[PATCH net] net: phy: fix phylib's dual eee_enabled

[PATCH net] net: phy: fix phylib's dual eee_enabled

[Russell King (Oracle)]

phylib has two eee_enabled members. Some parts of the code are using
phydev->eee_enabled, other parts are using phydev->eee_cfg.eee_enabled.
This leads to incorrect behaviour as their state goes out of sync.
ethtool --show-eee shows incorrect information, and --set-eee sometimes
doesn't take effect.

Fix this by only having one eee_enabled member - that in eee_cfg.

Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE")
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy-c45.c    | 4 +---
 drivers/net/phy/phy_device.c | 4 ++--
 include/linux/phy.h          | 2 --
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 5695935fdce9..ac987e5e82dc 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -942,7 +942,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
  */
 int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
 {
-	if (!phydev->eee_enabled) {
+	if (!phydev->eee_cfg.eee_enabled) {
 		__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
 
 		return genphy_c45_write_eee_adv(phydev, adv);
@@ -1575,8 +1575,6 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 		linkmode_copy(phydev->advertising_eee, adv);
 	}
 
-	phydev->eee_enabled = data->eee_enabled;
-
 	ret = genphy_c45_an_config_eee_aneg(phydev);
 	if (ret > 0) {
 		ret = phy_restart_aneg(phydev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 499797646580..5dfa2aa53c90 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3595,12 +3595,12 @@ static int phy_probe(struct device *dev)
 	/* There is no "enabled" flag. If PHY is advertising, assume it is
 	 * kind of enabled.
 	 */
-	phydev->eee_enabled = !linkmode_empty(phydev->advertising_eee);
+	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
 
 	/* Some PHYs may advertise, by default, not support EEE modes. So,
 	 * we need to clean them.
 	 */
-	if (phydev->eee_enabled)
+	if (phydev->eee_cfg.eee_enabled)
 		linkmode_and(phydev->advertising_eee, phydev->supported_eee,
 			     phydev->advertising_eee);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a98bc91a0cde..44890cdf40a2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -601,7 +601,6 @@ struct macsec_ops;
  * @adv_old: Saved advertised while power saving for WoL
  * @supported_eee: supported PHY EEE linkmodes
  * @advertising_eee: Currently advertised EEE linkmodes
- * @eee_enabled: Flag indicating whether the EEE feature is enabled
  * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
  * @eee_cfg: User configuration of EEE
  * @lp_advertising: Current link partner advertised linkmodes
@@ -721,7 +720,6 @@ struct phy_device {
 	/* used for eee validation and configuration*/
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee);
-	bool eee_enabled;
 
 	/* Host supported PHY interface types. Should be ignored if empty. */
 	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
-- 
2.30.2

Re: [PATCH net] net: phy: fix phylib's dual eee_enabled

[Heiner Kallweit]

On 14.11.2024 11:33, Russell King (Oracle) wrote:
> phylib has two eee_enabled members. Some parts of the code are using
> phydev->eee_enabled, other parts are using phydev->eee_cfg.eee_enabled.
> This leads to incorrect behaviour as their state goes out of sync.
> ethtool --show-eee shows incorrect information, and --set-eee sometimes
> doesn't take effect.
> 
> Fix this by only having one eee_enabled member - that in eee_cfg.
> 
> Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE")
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH net] net: phy: fix phylib's dual eee_enabled

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 14 Nov 2024 10:33:27 +0000 you wrote:
> phylib has two eee_enabled members. Some parts of the code are using
> phydev->eee_enabled, other parts are using phydev->eee_cfg.eee_enabled.
> This leads to incorrect behaviour as their state goes out of sync.
> ethtool --show-eee shows incorrect information, and --set-eee sometimes
> doesn't take effect.
> 
> Fix this by only having one eee_enabled member - that in eee_cfg.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: fix phylib's dual eee_enabled
    https://git.kernel.org/netdev/net/c/41ffcd95015f

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

Re: [PATCH net] net: phy: fix phylib's dual eee_enabled

[Herve Codina]

Hi,

On Thu, 14 Nov 2024 10:33:27 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> phylib has two eee_enabled members. Some parts of the code are using
> phydev->eee_enabled, other parts are using phydev->eee_cfg.eee_enabled.
> This leads to incorrect behaviour as their state goes out of sync.
> ethtool --show-eee shows incorrect information, and --set-eee sometimes
> doesn't take effect.
> 
> Fix this by only having one eee_enabled member - that in eee_cfg.
> 
> Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE")
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phy-c45.c    | 4 +---
>  drivers/net/phy/phy_device.c | 4 ++--
>  include/linux/phy.h          | 2 --
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 

I observed a regression with this patch applied.

My system is based on a i.MX8MP soc with a TI DP83867 ethernet PHY and was
working with the kernel v6.12 release.

Using the v6.13-rc6 kernel leads to a low ethernet bandwidth.

I used to perform SCP transfers at around 6MB/s on my setup and after moving
to the last v6.13-rc6 kernel, the bandwidth dropped to 70KB/s.

A git bisect identified the commit 41ffcd95015f ("net: phy: fix phylib's dual
eee_enabled").

With this patch applied, the issue is present. Without the patch, the issue
is not present. Also if I add the 'eee-broken-100tx' device-tree property in
the PHY node, the issue is not present anymore.

Didn't investigated more the issue but the patch introduced a regression on
my system.

Best regards,
Hervé

Re: [PATCH net] net: phy: fix phylib's dual eee_enabled

[Oleksij Rempel]

On Tue, Jan 07, 2025 at 02:40:48PM +0100, Herve Codina wrote:
> Hi,
> 
> On Thu, 14 Nov 2024 10:33:27 +0000
> "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> 
> > phylib has two eee_enabled members. Some parts of the code are using
> > phydev->eee_enabled, other parts are using phydev->eee_cfg.eee_enabled.
> > This leads to incorrect behaviour as their state goes out of sync.
> > ethtool --show-eee shows incorrect information, and --set-eee sometimes
> > doesn't take effect.
> > 
> > Fix this by only having one eee_enabled member - that in eee_cfg.
> > 
> > Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE")
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phy-c45.c    | 4 +---
> >  drivers/net/phy/phy_device.c | 4 ++--
> >  include/linux/phy.h          | 2 --
> >  3 files changed, 3 insertions(+), 7 deletions(-)
> > 
> 
> I observed a regression with this patch applied.
> 
> My system is based on a i.MX8MP soc with a TI DP83867 ethernet PHY and was
> working with the kernel v6.12 release.

Which ethernet interface is used on this system? FEC or stmmac?

Is it the correct PHY?
https://www.ti.com/product/de-de/DP83867E

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net] net: phy: fix phylib's dual eee_enabled

[Herve Codina]

On Tue, 7 Jan 2025 15:13:52 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Tue, Jan 07, 2025 at 02:40:48PM +0100, Herve Codina wrote:
> > Hi,
> > 
> > On Thu, 14 Nov 2024 10:33:27 +0000
> > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> >   
> > > phylib has two eee_enabled members. Some parts of the code are using
> > > phydev->eee_enabled, other parts are using phydev->eee_cfg.eee_enabled.
> > > This leads to incorrect behaviour as their state goes out of sync.
> > > ethtool --show-eee shows incorrect information, and --set-eee sometimes
> > > doesn't take effect.
> > > 
> > > Fix this by only having one eee_enabled member - that in eee_cfg.
> > > 
> > > Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE")
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  drivers/net/phy/phy-c45.c    | 4 +---
> > >  drivers/net/phy/phy_device.c | 4 ++--
> > >  include/linux/phy.h          | 2 --
> > >  3 files changed, 3 insertions(+), 7 deletions(-)
> > >   
> > 
> > I observed a regression with this patch applied.
> > 
> > My system is based on a i.MX8MP soc with a TI DP83867 ethernet PHY and was
> > working with the kernel v6.12 release.  
> 
> Which ethernet interface is used on this system? FEC or stmmac?

It is the FEC (ethernet@30be0000).

> 
> Is it the correct PHY?
> https://www.ti.com/product/de-de/DP83867E

Yes, it is this PHY.

Best regards,
Hervé

Re: [PATCH net] net: phy: fix phylib's dual eee_enabled

[Oleksij Rempel]

On Tue, Jan 07, 2025 at 03:23:45PM +0100, Herve Codina wrote:
> On Tue, 7 Jan 2025 15:13:52 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > On Tue, Jan 07, 2025 at 02:40:48PM +0100, Herve Codina wrote:
> > > Hi,
> > > 
> > > On Thu, 14 Nov 2024 10:33:27 +0000
> > > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> > >   
> > > > phylib has two eee_enabled members. Some parts of the code are using
> > > > phydev->eee_enabled, other parts are using phydev->eee_cfg.eee_enabled.
> > > > This leads to incorrect behaviour as their state goes out of sync.
> > > > ethtool --show-eee shows incorrect information, and --set-eee sometimes
> > > > doesn't take effect.
> > > > 
> > > > Fix this by only having one eee_enabled member - that in eee_cfg.
> > > > 
> > > > Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE")
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  drivers/net/phy/phy-c45.c    | 4 +---
> > > >  drivers/net/phy/phy_device.c | 4 ++--
> > > >  include/linux/phy.h          | 2 --
> > > >  3 files changed, 3 insertions(+), 7 deletions(-)
> > > >   
> > > 
> > > I observed a regression with this patch applied.
> > > 
> > > My system is based on a i.MX8MP soc with a TI DP83867 ethernet PHY and was
> > > working with the kernel v6.12 release.  
> > 
> > Which ethernet interface is used on this system? FEC or stmmac?
> 
> It is the FEC (ethernet@30be0000).

FEC driver's EEE support is still broken, i assume it is configuring
wrong timer. But, I can't fix without access to HW with proper PHY.

> > 
> > Is it the correct PHY?
> > https://www.ti.com/product/de-de/DP83867E
> 
> Yes, it is this PHY.

EEE support is not listed in this documentation and there is no errata.
I assume, there is still a register claiming EEE support. Otherwise it
would not be activated.

https://e2e.ti.com/support/interface-group/interface/f/interface-forum/658638/dp83867ir-eee-energy-efficient-ethernet
https://e2e.ti.com/support/interface-group/interface/f/interface-forum/556456/dp83867-mdi-auto-negotiation-with-eee-energy-efficient-ethernet-router?DP83867-MDI-Auto-Negotiation-with-EEE-Energy-Efficient-Ethernet-router
https://e2e.ti.com/support/interface-group/interface/f/interface-forum/716392/dp83867ir-eee-energy-efficient-ethernet-with-dp83867

Since chip vendor recommends to actively disable EEE support, something like
this will be needed.
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -947,6 +947,8 @@ static int dp83867_config_init(struct phy_device *phydev)
                               mask, val);
        }
 
+       phy_disable_eee(phydev);
+
        return 0;
 }

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |