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

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

[Daniel Golle]

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;
+	}
 
 	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);
-- 
2.47.1

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

[Andrew Lunn]

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

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

[Daniel Golle]

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.

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

[Russell King (Oracle)]

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.

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