| Message ID | 20240405-link-mode-reconfiguration-fix-v1-1-c1480bc2346a@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: sparx5: fix reconfiguration of PCS on link mode change | expand |
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.
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!
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 --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,
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,