diff mbox series

[net] net: sparx5: fix reconfiguration of PCS on link mode change

Message ID 20240405-link-mode-reconfiguration-fix-v1-1-c1480bc2346a@microchip.com (mailing list archive)
State New, archived
Headers show
Series [net] net: sparx5: fix reconfiguration of PCS on link mode change | expand

Commit Message

Daniel Machon April 5, 2024, 9:53 a.m. UTC 
It was observed that the PCS would be misconfigured on link mode change,
if the negotiated link mode went from no-inband capabilities to in-band
capabilities. This bug appeared after the neg_mode change of phylink [1],
but is really due to the wrong config being used when reconfiguring the PCS.

Fix this by correctly using the new port configuration instead of the old
one.

[1] https://lore.kernel.org/netdev/ZIxQIBfO9dH5xFlg@shell.armlinux.org.uk/

Fixes: 946e7fd5053a ("net: sparx5: add port module support")
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_port.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


---
base-commit: d76c740b2eaaddc5fc3a8b21eaec5b6b11e8c3f5
change-id: 20240305-link-mode-reconfiguration-fix-df961fef5505

Best regards,

Comments

Russell King (Oracle) April 5, 2024, 10:20 a.m. UTC | #1 
On Fri, Apr 05, 2024 at 11:53:15AM +0200, Daniel Machon wrote:
> It was observed that the PCS would be misconfigured on link mode change,
> if the negotiated link mode went from no-inband capabilities to in-band
> capabilities. This bug appeared after the neg_mode change of phylink [1],
> but is really due to the wrong config being used when reconfiguring the PCS.

I don't see how the change you point to could have changed the
behaviour. Old code:

	conf.inband = phylink_autoneg_inband(mode);
	conf.autoneg = phylink_test(advertising, Autoneg);

New code:

	conf.inband = neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED ||
		      neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
	conf.autoneg = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;

where, for SGMII or 802.3z modes, neg_mode will be one of
PHYLINK_PCS_NEG_INBAND_DISABLED or PHYLINK_PCS_NEG_INBAND_ENABLED if
phylink_autoneg_inband(mode) is true, or PHYLINK_PCS_NEG_OUTBAND if
not.

It does change conf.autoneg slightly in that this will always be true
for SGMII, but will only be true for Autoneg + 802.3z modes.

As far as your code change goes, it looks correct to me, but I think
it's fixing a bug that goes back long before the commit you have
identified.

However, I think there's another issue here which is more relevant to
the problem you describe in your commit message. If you look at
port_conf_has_changed(), you will notice that it fails to compare
conf.inband, and thus fails to notice any change in the setting of
that configuration item. This will result in sparx5_port_pcs_set()
not even being called if only conf.inband changes state.

Thus, changing from in-band to out-of-band or vice versa won't have
any effect if this is the only change that occurs, and this also
exists prior to my change.
Daniel Machon April 5, 2024, 7:45 p.m. UTC | #2 
Hi Russel,

> > It was observed that the PCS would be misconfigured on link mode change,
> > if the negotiated link mode went from no-inband capabilities to in-band
> > capabilities. This bug appeared after the neg_mode change of phylink [1],
> > but is really due to the wrong config being used when reconfiguring the PCS.
> 
> I don't see how the change you point to could have changed the
> behaviour. Old code:
> 
>         conf.inband = phylink_autoneg_inband(mode);
>         conf.autoneg = phylink_test(advertising, Autoneg);
> 
> New code:
> 
>         conf.inband = neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED ||
>                       neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
>         conf.autoneg = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
> 
> where, for SGMII or 802.3z modes, neg_mode will be one of
> PHYLINK_PCS_NEG_INBAND_DISABLED or PHYLINK_PCS_NEG_INBAND_ENABLED if
> phylink_autoneg_inband(mode) is true, or PHYLINK_PCS_NEG_OUTBAND if
> not.

For inband/10GBase-R, conf.inband would be true, prior to the
phylink change. This is fine since conf.inband is only used when
configuring low speed devices.

After the change, conf.inband will be false when opting in through
phylink_pcs.net_mode = true. This causes the SGMII to be misconfigured,
since the inband parameter from the old config is used, and that
parameter is actually used for configuring SGMII..

TBH the commit description is inadequate at best. I will revise it in a
v2.

> 
> It does change conf.autoneg slightly in that this will always be true
> for SGMII, but will only be true for Autoneg + 802.3z modes.
> 
> As far as your code change goes, it looks correct to me, but I think
> it's fixing a bug that goes back long before the commit you have
> identified.

Just to be clear - by commit do you mean the phylink change or the commit
referenced in the fixes tag?

> 
> However, I think there's another issue here which is more relevant to
> the problem you describe in your commit message. If you look at
> port_conf_has_changed(), you will notice that it fails to compare
> conf.inband, and thus fails to notice any change in the setting of
> that configuration item. This will result in sparx5_port_pcs_set()
> not even being called if only conf.inband changes state.
> 
> Thus, changing from in-band to out-of-band or vice versa won't have
> any effect if this is the only change that occurs, and this also
> exists prior to my change.

Yes. I agree this is an issue that deserves a fix - separate from this
one I would think.

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Russell King (Oracle) April 5, 2024, 8:05 p.m. UTC | #3 
On Fri, Apr 05, 2024 at 07:45:56PM +0000, Daniel Machon wrote:
> Hi Russel,
> 
> > It does change conf.autoneg slightly in that this will always be true
> > for SGMII, but will only be true for Autoneg + 802.3z modes.
> > 
> > As far as your code change goes, it looks correct to me, but I think
> > it's fixing a bug that goes back long before the commit you have
> > identified.
> 
> Just to be clear - by commit do you mean the phylink change or the commit
> referenced in the fixes tag?

I think the bug technically exists prior to the phylink change if
there is any possibility that the link may switch between inband
and a non-inband mode. (e.g. as a result of inserting a SFP module.)

> > However, I think there's another issue here which is more relevant to
> > the problem you describe in your commit message. If you look at
> > port_conf_has_changed(), you will notice that it fails to compare
> > conf.inband, and thus fails to notice any change in the setting of
> > that configuration item. This will result in sparx5_port_pcs_set()
> > not even being called if only conf.inband changes state.
> > 
> > Thus, changing from in-band to out-of-band or vice versa won't have
> > any effect if this is the only change that occurs, and this also
> > exists prior to my change.
> 
> Yes. I agree this is an issue that deserves a fix - separate from this
> one I would think.

Agreed, thanks.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
index 3a1b1a1f5a19..60dd2fd603a8 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
@@ -731,7 +731,7 @@  static int sparx5_port_pcs_low_set(struct sparx5 *sparx5,
 	bool sgmii = false, inband_aneg = false;
 	int err;
 
-	if (port->conf.inband) {
+	if (conf->inband) {
 		if (conf->portmode == PHY_INTERFACE_MODE_SGMII ||
 		    conf->portmode == PHY_INTERFACE_MODE_QSGMII)
 			inband_aneg = true; /* Cisco-SGMII in-band-aneg */
@@ -948,7 +948,7 @@  int sparx5_port_pcs_set(struct sparx5 *sparx5,
 	if (err)
 		return -EINVAL;
 
-	if (port->conf.inband) {
+	if (conf->inband) {
 		/* Enable/disable 1G counters in ASM */
 		spx5_rmw(ASM_PORT_CFG_CSC_STAT_DIS_SET(high_speed_dev),
 			 ASM_PORT_CFG_CSC_STAT_DIS,