Message-ID: <ZwUpT9HRdl33gv_G@shell.armlinux.org.uk>
Date: Tue, 8 Oct 2024 13:45:03 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Daniel Golle <daniel@...rotopia.org>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: realtek: check validity of 10GbE
 link-partner advertisement

On Tue, Oct 08, 2024 at 12:59:17PM +0100, Daniel Golle wrote:
> 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@...rotopia.org>
> > > > > ---
> > > > >  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.

Let's start checking what we're doing with regards to this register.

7.33.11 (Link Partner 10GBASE-T capability) states that this is only
valid when the Page received bit (7.1.6) has been set. This is the
BMSR_ANEGCOMPLETE / MDIO_AN_STAT1_COMPLETE bit.

Looking at rtl822x_read_status, which is called directly as a
.read_status() method, it reads some register that might be the
equivalent of MMD 7 Register 33 (if that's what 0xa5d, 0x13 is,
0xa5d, 0x12 seems to be MDIO_AN_10GBT_CTRL) whether or not the link
is up and whether or not AN has completed. It's only conditional on
Autoneg being enabled.

However, we don't look at 7.1.6, which is wrong according to 802.3.
So I think the first thing that's needed here is that needs fixing
- we should only be reading the LP ability registers when (a) we
have link, and (b) when the PHY indicates that config pages have
been received.

The next thing that needs fixing is to add support for checking
these LOCOK/REMOK bits - and if these are specific to the result of
the negotiation (there's some hints in 802.3 that's the case, as
there are other registers with similar bits in, but I haven't
looked deeply at it) then, since the resolution is done in core
PHY code, I think we need another method into drivers to check
these bits once resolution has occurred.

However, I need to do more reading to work this out.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists