| Message ID | 229e077bad31d1a9086426f60c3a4f4ac20d2c1a.1736901813.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: realtek: clear status if link is down | expand |
On Wed, Jan 15, 2025 at 12:46:11AM +0000, Daniel Golle wrote: > Clear speed, duplex and master/slave status in case the link is down > to avoid reporting bogus link(-partner) properties. > > Fixes: 5cb409b3960e ("net: phy: realtek: clear 1000Base-T link partner advertisement") > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > drivers/net/phy/realtek.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index f65d7f1f348e..3f0e03e2abce 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -720,8 +720,12 @@ static int rtlgen_read_status(struct phy_device *phydev) > if (ret < 0) > return ret; > > - if (!phydev->link) > + if (!phydev->link) { > + phydev->duplex = DUPLEX_UNKNOWN; > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; > + phydev->speed = SPEED_UNKNOWN; > return 0; > + } > I must be missing something here... rtlgen_read_status() first calls genphy_read_status(phydev); int genphy_read_status(struct phy_device *phydev) { int err, old_link = phydev->link; /* Update the link, but return if there was an error */ err = genphy_update_link(phydev); if (err) return err; /* why bother the PHY if nothing can have changed */ if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link) return 0; phydev->master_slave_get = MASTER_SLAVE_CFG_UNSUPPORTED; phydev->master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED; phydev->speed = SPEED_UNKNOWN; phydev->duplex = DUPLEX_UNKNOWN; phydev->pause = 0; phydev->asym_pause = 0; Why is that not sufficient ? > val = phy_read_paged(phydev, 0xa43, 0x12); > if (val < 0) > @@ -1028,11 +1032,11 @@ static int rtl822x_c45_read_status(struct phy_device *phydev) > return ret; > > if (phydev->autoneg == AUTONEG_DISABLE || > - !genphy_c45_aneg_done(phydev)) > + !genphy_c45_aneg_done(phydev) || > + !phydev->link) { > mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, 0); > - > - /* Vendor register as C45 has no standardized support for 1000BaseT */ > - if (phydev->autoneg == AUTONEG_ENABLE) { > + } else { > + /* Vendor register as C45 has no standardized support for 1000BaseT */ > val = phy_read_mmd(phydev, MDIO_MMD_VEND2, > RTL822X_VND2_GANLPAR); > if (val < 0) > @@ -1041,8 +1045,12 @@ static int rtl822x_c45_read_status(struct phy_device *phydev) > mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val); > } > > - if (!phydev->link) > + if (!phydev->link) { > + phydev->duplex = DUPLEX_UNKNOWN; > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; > + phydev->speed = SPEED_UNKNOWN; > return 0; > + } rtl822x_c45_read_status() calls genphy_c45_read_link() which again clears state from phydev. Andrew
Hi Andrew, On Wed, Jan 15, 2025 at 03:50:33AM +0100, Andrew Lunn wrote: > On Wed, Jan 15, 2025 at 12:46:11AM +0000, Daniel Golle wrote: > > Clear speed, duplex and master/slave status in case the link is down > > to avoid reporting bogus link(-partner) properties. > > > > Fixes: 5cb409b3960e ("net: phy: realtek: clear 1000Base-T link partner advertisement") > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > drivers/net/phy/realtek.c | 20 ++++++++++++++------ > > 1 file changed, 14 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > > index f65d7f1f348e..3f0e03e2abce 100644 > > --- a/drivers/net/phy/realtek.c > > +++ b/drivers/net/phy/realtek.c > > @@ -720,8 +720,12 @@ static int rtlgen_read_status(struct phy_device *phydev) > > if (ret < 0) > > return ret; > > > > - if (!phydev->link) > > + if (!phydev->link) { > > + phydev->duplex = DUPLEX_UNKNOWN; > > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; > > + phydev->speed = SPEED_UNKNOWN; > > return 0; > > + } > > > > I must be missing something here... > > > rtlgen_read_status() first calls genphy_read_status(phydev); > [...] > Why is that not sufficient ? The problem are the stale NBase-T link-partner advertisement bits and the subsequent call to phy_resolve_aneg_linkmode(), which results in bogus speed and duplex, based on previously connected link partner advertising 2500Base-T, 5GBase-T or 10GBase-T modes. The more elegant solution I found by now is to just always call mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0); before calling rtlgen_read_status(). In case the link is up, rtlgen_decode_physr() will anyway set speed and duplex. > > @@ -1041,8 +1045,12 @@ static int rtl822x_c45_read_status(struct phy_device *phydev) > > mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val); > > } > > > > - if (!phydev->link) > > + if (!phydev->link) { > > + phydev->duplex = DUPLEX_UNKNOWN; > > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; > > + phydev->speed = SPEED_UNKNOWN; > > return 0; > > + } > > > rtl822x_c45_read_status() calls genphy_c45_read_link() which again > clears state from phydev. rtl822x_c45_read_status() calls genphy_c45_read_status(), which calls genphy_c45_read_lpa(), and that doesn't clear either ETHTOOL_LINK_MODE_1000baseT_Half_BIT nor ETHTOOL_LINK_MODE_1000baseT_Full_BIT as there is no generic handling for 1000Base-T in Clause-45. So also in the Clause-45 case, the subsequent call to phy_resolve_aneg_linkmode() may then wrongly populate speed and duplex, this time according to the stale 1000baseT bits. Moving the call to rtl822x_c45_read_status() in rtl822x_c45_read_status() to after the 1000baseT lpa bits have been taken care of fixes that part of the issue. Clearing master_slave_state in the C45 case is still necessary because it isn't done by genphy_c45_read_status(). I will post a series replacing this patch for all 3 described changes.
On Wed, Jan 15, 2025 at 05:07:22AM +0000, Daniel Golle wrote: > Hi Andrew, > > On Wed, Jan 15, 2025 at 03:50:33AM +0100, Andrew Lunn wrote: > > On Wed, Jan 15, 2025 at 12:46:11AM +0000, Daniel Golle wrote: > > > Clear speed, duplex and master/slave status in case the link is down > > > to avoid reporting bogus link(-partner) properties. > > > > > > Fixes: 5cb409b3960e ("net: phy: realtek: clear 1000Base-T link partner advertisement") > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > > --- > > > drivers/net/phy/realtek.c | 20 ++++++++++++++------ > > > 1 file changed, 14 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > > > index f65d7f1f348e..3f0e03e2abce 100644 > > > --- a/drivers/net/phy/realtek.c > > > +++ b/drivers/net/phy/realtek.c > > > @@ -720,8 +720,12 @@ static int rtlgen_read_status(struct phy_device *phydev) > > > if (ret < 0) > > > return ret; > > > > > > - if (!phydev->link) > > > + if (!phydev->link) { > > > + phydev->duplex = DUPLEX_UNKNOWN; > > > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; > > > + phydev->speed = SPEED_UNKNOWN; > > > return 0; > > > + } > > > > > > > I must be missing something here... > > > > > > rtlgen_read_status() first calls genphy_read_status(phydev); > > [...] > > Why is that not sufficient ? > > The problem are the stale NBase-T link-partner advertisement bits and the > subsequent call to phy_resolve_aneg_linkmode(), which results in bogus > speed and duplex, based on previously connected link partner advertising > 2500Base-T, 5GBase-T or 10GBase-T modes. This means you're also populating the link-partner advertisement bits with bogus data. It would be better not to read the link status. Does it leave the MDIO_AN_STAT1_COMPLETE bit set as well, thus causing genphy_c45_baset1_read_lpa() to read the advertisement rather than clearing it? Or is it because this is buggy: /* Vendor register as C45 has no standardized support for 1000BaseT */ if (phydev->autoneg == AUTONEG_ENABLE) { val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_GANLPAR); if (val < 0) return val; mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val); } This should be clearing the bits that mii_stat1000_mod_linkmode_lpa_t() sets if autoneg is not complete. > rtl822x_c45_read_status() calls genphy_c45_read_status(), which calls > genphy_c45_read_lpa(), and that doesn't clear either > ETHTOOL_LINK_MODE_1000baseT_Half_BIT nor ETHTOOL_LINK_MODE_1000baseT_Full_BIT > as there is no generic handling for 1000Base-T in Clause-45. > > So also in the Clause-45 case, the subsequent call to > phy_resolve_aneg_linkmode() may then wrongly populate speed and duplex, this > time according to the stale 1000baseT bits. > > Moving the call to rtl822x_c45_read_status() in rtl822x_c45_read_status() to > after the 1000baseT lpa bits have been taken care of fixes that part of the > issue. > > Clearing master_slave_state in the C45 case is still necessary because it isn't > done by genphy_c45_read_status(). So that's a yes then. That's because the functions only set/clear the advertisement bits that they control. If the PHY driver manages any other advertisement bits, it has to _fully_ manage them. So with 1000baseT bits, as the generic functions don't manage them, the PHY driver has to do _full_ management of them. That includes clearing the bits when autoneg is not complete.
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index f65d7f1f348e..3f0e03e2abce 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -720,8 +720,12 @@ static int rtlgen_read_status(struct phy_device *phydev) if (ret < 0) return ret; - if (!phydev->link) + if (!phydev->link) { + phydev->duplex = DUPLEX_UNKNOWN; + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; + phydev->speed = SPEED_UNKNOWN; return 0; + } val = phy_read_paged(phydev, 0xa43, 0x12); if (val < 0) @@ -1028,11 +1032,11 @@ static int rtl822x_c45_read_status(struct phy_device *phydev) return ret; if (phydev->autoneg == AUTONEG_DISABLE || - !genphy_c45_aneg_done(phydev)) + !genphy_c45_aneg_done(phydev) || + !phydev->link) { mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, 0); - - /* Vendor register as C45 has no standardized support for 1000BaseT */ - if (phydev->autoneg == AUTONEG_ENABLE) { + } else { + /* Vendor register as C45 has no standardized support for 1000BaseT */ val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_GANLPAR); if (val < 0) @@ -1041,8 +1045,12 @@ static int rtl822x_c45_read_status(struct phy_device *phydev) mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val); } - if (!phydev->link) + if (!phydev->link) { + phydev->duplex = DUPLEX_UNKNOWN; + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; + phydev->speed = SPEED_UNKNOWN; return 0; + } /* Read actual speed from vendor register. */ val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL_VND2_PHYSR);
Clear speed, duplex and master/slave status in case the link is down to avoid reporting bogus link(-partner) properties. Fixes: 5cb409b3960e ("net: phy: realtek: clear 1000Base-T link partner advertisement") Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/net/phy/realtek.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)