| 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.
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(-)