Re: [PATCH net-next] net: phy: realtek: check validity of 10GbE link-partner advertisement

[Daniel Golle]

On Tue, Oct 08, 2024 at 12:10:07PM +0100, Russell King (Oracle) wrote:
> On Fri, Oct 04, 2024 at 11:06:01PM +0100, Daniel Golle wrote:
> > On Fri, Oct 04, 2024 at 11:17:28PM +0200, Andrew Lunn wrote:
> > > On Fri, Oct 04, 2024 at 04:50:36PM +0100, Daniel Golle wrote:
> > > > Only use link-partner advertisement bits for 10GbE modes if they are
> > > > actually valid. Check LOCALOK and REMOTEOK bits and clear 10GbE modes
> > > > unless both of them are set.
> > > > This prevents misinterpreting the stale 2500M link-partner advertisement
> > > > bit in case a subsequent linkpartner doesn't do any NBase-T
> > > > advertisement at all.
> > > > 
> > > > Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/net/phy/realtek.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > > > index c4d0d93523ad..d276477cf511 100644
> > > > --- a/drivers/net/phy/realtek.c
> > > > +++ b/drivers/net/phy/realtek.c
> > > > @@ -927,6 +927,10 @@ static int rtl822x_read_status(struct phy_device *phydev)
> > > >  		if (lpadv < 0)
> > > >  			return lpadv;
> > > >  
> > > > +		if (!(lpadv & MDIO_AN_10GBT_STAT_REMOK) ||
> > > > +		    !(lpadv & MDIO_AN_10GBT_STAT_LOCOK))
> > > > +			lpadv = 0;
> > > > +
> > > >  		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising,
> > > >  						  lpadv);
> > > 
> > > I know lpadv is coming from a vendor register, but does
> > > MDIO_AN_10GBT_STAT_LOCOK and MDIO_AN_10GBT_STAT_REMOK apply if it was
> > > also from the register defined in 802.3? I'm just wondering if this
> > > test should be inside mii_10gbt_stat_mod_linkmode_lpa_t()?
> > 
> > Yes, it does apply and I thought the same, but as
> > mii_10gbt_stat_mod_linkmode_lpa_t is used in various places without
> > checking those two bits we may break other PHYs which may not use
> > them (and apparently this is mostly a problem on RealTek PHYs where
> > all the other bits in the register persist in case of a non-NBase-T-
> > capable subsequent link-partner after initially being connected to
> > an NBase-T-capable one).
> > 
> > Maybe we could introduce a new function
> > mii_10gbt_stat_mod_linkmode_lpa_validate_t()
> > which calls mii_10gbt_stat_mod_linkmode_lpa_t() but checks LOCOK and
> > REMOK as a precondition?
> 
> Isn't the link status supposed to indicate link down of LOCOK
> is clear?
> 
> Maybe checking these bits should be included in the link status
> check, and if not set, then phydev->link should be cleared?

At least in case of those RealTek PHYs the situation is a bit different:
The AN_10GBT bits do get set according to the link partner advertisement
in case the link partner does any 10GBT advertisement at all.
Now, if after being connected to a link partner which does 10GBT
advertisement you subsequently connect to a link partner which doesn't
do any 10GBT advertisement at all, the previously advertised bits
remain in the register, just REMOK and LOCOK aren't set.
That obviously doesn't imply that the link is down.
I noticed it because I would see a downshift warning in kernel logs
even though the new link partner was not capable of connecting with
speeds higher than 1000MBit/s.
Note that some that this doesn't happen with all 1GBE NICs, because
some seem to carry out 10GBT advertisement but just all empty while
others just don't do any 10GBT advertisement at all.