[PATCH net 0/3] net: phy: realtek: fix status when link is down

[PATCH net 0/3] net: phy: realtek: fix status when link is down

[Daniel Golle]

The .read_status method for RealTek RTL822x PHYs (both C22 and C45) has
multilpe issues which result in reporting bogus link partner advertised
modes as well as speed and duplex while the link is down and no cable
is plugged in.

Example: ethtool after disconnecting a 1000M/Full capable link partner,
now with no wire plugged:

Settings for lan1:
    Supported ports: [ TP ]
    Supported link modes:   10baseT/Half 10baseT/Full
                            100baseT/Half 100baseT/Full
                            1000baseT/Full
                            2500baseT/Full
    Supported pause frame use: Symmetric Receive-only
    Supports auto-negotiation: Yes
    Supported FEC modes: Not reported
    Advertised link modes:  10baseT/Half 10baseT/Full
                            100baseT/Half 100baseT/Full
                            1000baseT/Full
                            2500baseT/Full
    Advertised pause frame use: Symmetric Receive-only
    Advertised auto-negotiation: Yes
    Advertised FEC modes: Not reported
    Link partner advertised link modes:  1000baseT/Full
    Link partner advertised pause frame use: No
    Link partner advertised auto-negotiation: No
    Link partner advertised FEC modes: Not reported
    Speed: 1000Mb/s
    Duplex: Full
    Auto-negotiation: on
    Port: Twisted Pair
    PHYAD: 7
    Transceiver: external
    MDI-X: Unknown
    Supports Wake-on: d
    Wake-on: d
    Link detected: no

Fix this by making sure all of the fields populated by
rtl822x_c45_read_status() or rtl822x_read_status() get reset, also in
case the link is down.

Daniel Golle (3):
  net: phy: realtek: clear 1000Base-T lpa bits if link is down
  net: phy: realtek: clear master_slave_state if link is down
  net: phy: realtek: always clear 10gbase-t link partner advertisement

 drivers/net/phy/realtek.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

-- 
2.47.1

[PATCH net 1/3] net: phy: realtek: clear 1000Base-T lpa if link is down

[Daniel Golle]

Only read 1000Base-T link partner advertisement if autonegotiation has
completed and otherwise 1000Base-T link partner advertisement bits.

This fixes bogus 1000Base-T link partner advertisement after link goes
down (eg. by disconnecting the wire).
Fixes: 5cb409b3960e ("net: phy: realtek: clear 1000Base-T link partner advertisement")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/realtek.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f65d7f1f348e..26b324ab0f90 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -1023,23 +1023,20 @@ static int rtl822x_c45_read_status(struct phy_device *phydev)
 {
 	int ret, val;
 
-	ret = genphy_c45_read_status(phydev);
-	if (ret < 0)
-		return ret;
-
-	if (phydev->autoneg == AUTONEG_DISABLE ||
-	    !genphy_c45_aneg_done(phydev))
-		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) {
+	if (phydev->autoneg == AUTONEG_ENABLE && genphy_c45_aneg_done(phydev)) {
 		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);
+	} else {
+		val = 0;
 	}
+	mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val);
+
+	ret = genphy_c45_read_status(phydev);
+	if (ret < 0)
+		return ret;
 
 	if (!phydev->link)
 		return 0;
-- 
2.47.1

[PATCH net 2/3] net: phy: realtek: clear master_slave_state if link is down

[Daniel Golle]

rtlgen_decode_physr() which sets master_slave_state isn't called in case
the link is down and other than rtlgen_read_status(),
rtl822x_c45_read_status() doesn't implicitely clear master_slave_state.

Avoid stale master_slave_state by always setting it to
MASTER_SLAVE_STATE_UNKNOWN in rtl822x_c45_read_status() in case the link
is down.

Fixes: 081c9c0265c9 ("net: phy: realtek: read duplex and gbit master from PHYSR register")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/realtek.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 26b324ab0f90..93704abb6787 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -1038,8 +1038,10 @@ static int rtl822x_c45_read_status(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	if (!phydev->link)
+	if (!phydev->link) {
+		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
 		return 0;
+	}
 
 	/* Read actual speed from vendor register. */
 	val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL_VND2_PHYSR);
-- 
2.47.1

[PATCH net 3/3] net: phy: realtek: always clear NBase-T lpa

[Daniel Golle]

Clear NBase-T link partner advertisement before calling
rtlgen_read_status() to avoid phy_resolve_aneg_linkmode() wrongly
setting speed and duplex.

This fixes bogus 2.5G/5G/10G link partner advertisement and thus
speed and duplex being set by phy_resolve_aneg_linkmode() due to stale
NBase-T lpa.

Fixes: 68d5cd09e891 ("net: phy: realtek: change order of calls in C22 read_status()")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/realtek.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 93704abb6787..9cefca1aefa1 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -952,15 +952,15 @@ static int rtl822x_read_status(struct phy_device *phydev)
 {
 	int lpadv, ret;
 
+	mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
+
 	ret = rtlgen_read_status(phydev);
 	if (ret < 0)
 		return ret;
 
 	if (phydev->autoneg == AUTONEG_DISABLE ||
-	    !phydev->autoneg_complete) {
-		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
+	    !phydev->autoneg_complete)
 		return 0;
-	}
 
 	lpadv = phy_read_paged(phydev, 0xa5d, 0x13);
 	if (lpadv < 0)
-- 
2.47.1

Re: [PATCH net 0/3] net: phy: realtek: fix status when link is down

[Heiner Kallweit]

On 15.01.2025 15:43, Daniel Golle wrote:
> The .read_status method for RealTek RTL822x PHYs (both C22 and C45) has
> multilpe issues which result in reporting bogus link partner advertised
> modes as well as speed and duplex while the link is down and no cable
> is plugged in.
> 
> Example: ethtool after disconnecting a 1000M/Full capable link partner,
> now with no wire plugged:
> 
> Settings for lan1:
>     Supported ports: [ TP ]
>     Supported link modes:   10baseT/Half 10baseT/Full
>                             100baseT/Half 100baseT/Full
>                             1000baseT/Full
>                             2500baseT/Full
>     Supported pause frame use: Symmetric Receive-only
>     Supports auto-negotiation: Yes
>     Supported FEC modes: Not reported
>     Advertised link modes:  10baseT/Half 10baseT/Full
>                             100baseT/Half 100baseT/Full
>                             1000baseT/Full
>                             2500baseT/Full
>     Advertised pause frame use: Symmetric Receive-only
>     Advertised auto-negotiation: Yes
>     Advertised FEC modes: Not reported
>     Link partner advertised link modes:  1000baseT/Full
>     Link partner advertised pause frame use: No
>     Link partner advertised auto-negotiation: No
>     Link partner advertised FEC modes: Not reported
>     Speed: 1000Mb/s
>     Duplex: Full
>     Auto-negotiation: on
>     Port: Twisted Pair
>     PHYAD: 7
>     Transceiver: external
>     MDI-X: Unknown
>     Supports Wake-on: d
>     Wake-on: d
>     Link detected: no
> 
> Fix this by making sure all of the fields populated by
> rtl822x_c45_read_status() or rtl822x_read_status() get reset, also in
> case the link is down.
> 
> Daniel Golle (3):
>   net: phy: realtek: clear 1000Base-T lpa bits if link is down
>   net: phy: realtek: clear master_slave_state if link is down
>   net: phy: realtek: always clear 10gbase-t link partner advertisement
> 
>  drivers/net/phy/realtek.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
Note that the Realtek PHY driver has just been moved to its own subdirectory.
So you have to rebase your patch set.

Re: [PATCH net 0/3] net: phy: realtek: fix status when link is down

[Jakub Kicinski]

On Wed, 15 Jan 2025 18:19:44 +0100 Heiner Kallweit wrote:
> Note that the Realtek PHY driver has just been moved to its own subdirectory.
> So you have to rebase your patch set.

Maybe not just yet. This is for net, and the rename was in net-next.
But as you may be alluding to 6.13 is likely to get released this
weekend, so the distinction will stop being relevant.

Let's keep rebasing on net for now, without anticipating the merge.

nit: subject should have said v2

Re: [PATCH net 1/3] net: phy: realtek: clear 1000Base-T lpa if link is down

[Michal Swiatkowski]

On Wed, Jan 15, 2025 at 02:43:35PM +0000, Daniel Golle wrote:
> Only read 1000Base-T link partner advertisement if autonegotiation has
> completed and otherwise 1000Base-T link partner advertisement bits.
> 
> This fixes bogus 1000Base-T link partner advertisement after link goes
> down (eg. by disconnecting the wire).
> Fixes: 5cb409b3960e ("net: phy: realtek: clear 1000Base-T link partner advertisement")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/phy/realtek.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index f65d7f1f348e..26b324ab0f90 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -1023,23 +1023,20 @@ static int rtl822x_c45_read_status(struct phy_device *phydev)
>  {
>  	int ret, val;
>  
> -	ret = genphy_c45_read_status(phydev);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (phydev->autoneg == AUTONEG_DISABLE ||
> -	    !genphy_c45_aneg_done(phydev))
> -		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) {
> +	if (phydev->autoneg == AUTONEG_ENABLE && genphy_c45_aneg_done(phydev)) {
>  		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);
> +	} else {
> +		val = 0;
>  	}
> +	mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val);
> +
> +	ret = genphy_c45_read_status(phydev);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (!phydev->link)
>  		return 0;

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> -- 
> 2.47.1

Re: [PATCH net 2/3] net: phy: realtek: clear master_slave_state if link is down

[Michal Swiatkowski]

On Wed, Jan 15, 2025 at 02:43:43PM +0000, Daniel Golle wrote:
> rtlgen_decode_physr() which sets master_slave_state isn't called in case
> the link is down and other than rtlgen_read_status(),
> rtl822x_c45_read_status() doesn't implicitely clear master_slave_state.
> 
> Avoid stale master_slave_state by always setting it to
> MASTER_SLAVE_STATE_UNKNOWN in rtl822x_c45_read_status() in case the link
> is down.
> 
> Fixes: 081c9c0265c9 ("net: phy: realtek: read duplex and gbit master from PHYSR register")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/phy/realtek.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 26b324ab0f90..93704abb6787 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -1038,8 +1038,10 @@ static int rtl822x_c45_read_status(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (!phydev->link)
> +	if (!phydev->link) {
> +		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
>  		return 0;
> +	}
>  
>  	/* Read actual speed from vendor register. */
>  	val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL_VND2_PHYSR);

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> -- 
> 2.47.1

Re: [PATCH net 3/3] net: phy: realtek: always clear NBase-T lpa

[Michal Swiatkowski]

On Wed, Jan 15, 2025 at 02:45:00PM +0000, Daniel Golle wrote:
> Clear NBase-T link partner advertisement before calling
> rtlgen_read_status() to avoid phy_resolve_aneg_linkmode() wrongly
> setting speed and duplex.
> 
> This fixes bogus 2.5G/5G/10G link partner advertisement and thus
> speed and duplex being set by phy_resolve_aneg_linkmode() due to stale
> NBase-T lpa.
> 
> Fixes: 68d5cd09e891 ("net: phy: realtek: change order of calls in C22 read_status()")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/phy/realtek.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 93704abb6787..9cefca1aefa1 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -952,15 +952,15 @@ static int rtl822x_read_status(struct phy_device *phydev)
>  {
>  	int lpadv, ret;
>  
> +	mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
> +
>  	ret = rtlgen_read_status(phydev);
>  	if (ret < 0)
>  		return ret;
>  
>  	if (phydev->autoneg == AUTONEG_DISABLE ||
> -	    !phydev->autoneg_complete) {
> -		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
> +	    !phydev->autoneg_complete)
>  		return 0;
> -	}
>  
>  	lpadv = phy_read_paged(phydev, 0xa5d, 0x13);
>  	if (lpadv < 0)

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> -- 
> 2.47.1

Re: [PATCH net 0/3] net: phy: realtek: fix status when link is down

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 15 Jan 2025 14:43:24 +0000 you wrote:
> The .read_status method for RealTek RTL822x PHYs (both C22 and C45) has
> multilpe issues which result in reporting bogus link partner advertised
> modes as well as speed and duplex while the link is down and no cable
> is plugged in.
> 
> Example: ethtool after disconnecting a 1000M/Full capable link partner,
> now with no wire plugged:
> 
> [...]

Here is the summary with links:
  - [net,1/3] net: phy: realtek: clear 1000Base-T lpa if link is down
    https://git.kernel.org/netdev/net/c/34d5a86ff7bb
  - [net,2/3] net: phy: realtek: clear master_slave_state if link is down
    https://git.kernel.org/netdev/net/c/ea8318cb33e5
  - [net,3/3] net: phy: realtek: always clear NBase-T lpa
    https://git.kernel.org/netdev/net/c/d3eb58549842

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html