Re: [PATCH net] net: phy: realtek: clear status if link is down

[Daniel Golle <daniel@makrotopia.org>]

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.