[PATCH net v3] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[PATCH net v3] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[Russell King (Oracle)]

When phy_ethtool_set_eee_noneg() detects a change in the LPI
parameters, it attempts to update phylib state and trigger the link
to cycle so the MAC sees the updated parameters.

However, in doing so, it sets phydev->enable_tx_lpi depending on
whether the EEE configuration allows the MAC to generate LPI without
taking into account the result of negotiation.

This can be demonstrated with a 1000base-T FD interface by:

 # ethtool --set-eee eno0 advertise 8   # cause EEE to be not negotiated
 # ethtool --set-eee eno0 tx-lpi off
 # ethtool --set-eee eno0 tx-lpi on

This results in being true, despite EEE not having been negotiated and:
 # ethtool --show-eee eno0
	EEE status: enabled - inactive
	Tx LPI: 250 (us)
	Supported EEE link modes:  100baseT/Full
	                           1000baseT/Full
	Advertised EEE link modes:  100baseT/Full
	                                         1000baseT/Full

Fix this by keeping track of whether EEE was negotiated via a new
eee_active member in struct phy_device, and include this state in
the decision whether phydev->enable_tx_lpi should be set.

Fixes: 3e43b903da04 ("net: phy: Immediately call adjust_link if only tx_lpi_enabled changes")
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
v3:
- fixed kernel-doc
- added fixes tag
- rebased on Heiner's patch: https://patchwork.kernel.org/project/netdevbpf/patch/a5efc274-ce58-49f3-ac8a-5384d9b41695@gmail.com/
As such, not added Andrew's r-b.

 drivers/net/phy/phy-c45.c |  2 +-
 drivers/net/phy/phy.c     | 30 ++++++++++++++++++------------
 include/linux/phy.h       |  2 ++
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 96d0b3a5a9d3..944ae98ad110 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1530,7 +1530,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 		return ret;
 
 	data->eee_enabled = is_enabled;
-	data->eee_active = ret;
+	data->eee_active = phydev->eee_active;
 	linkmode_copy(data->supported, phydev->supported_eee);
 
 	return 0;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a660a80f34b7..0d20b534122b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -990,14 +990,14 @@ static int phy_check_link_status(struct phy_device *phydev)
 		phydev->state = PHY_RUNNING;
 		err = genphy_c45_eee_is_active(phydev,
 					       NULL, NULL, NULL);
-		if (err <= 0)
-			phydev->enable_tx_lpi = false;
-		else
-			phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled;
+		phydev->eee_active = err > 0;
+		phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled &&
+					phydev->eee_active;
 
 		phy_link_up(phydev);
 	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
 		phydev->state = PHY_NOLINK;
+		phydev->eee_active = false;
 		phydev->enable_tx_lpi = false;
 		phy_link_down(phydev);
 	}
@@ -1685,15 +1685,21 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
 static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
 				      const struct eee_config *old_cfg)
 {
-	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
+	bool enable_tx_lpi;
+
+	if (!phydev->link)
+		return;
+
+	enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active;
+
+	if (phydev->enable_tx_lpi != enable_tx_lpi ||
 	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
-		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
-		if (phydev->link) {
-			phydev->link = false;
-			phy_link_down(phydev);
-			phydev->link = true;
-			phy_link_up(phydev);
-		}
+		phydev->enable_tx_lpi = false;
+		phydev->link = false;
+		phy_link_down(phydev);
+		phydev->enable_tx_lpi = enable_tx_lpi;
+		phydev->link = true;
+		phy_link_up(phydev);
 	}
 }
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 77c6d6451638..563c46205685 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -602,6 +602,7 @@ struct macsec_ops;
  * @supported_eee: supported PHY EEE linkmodes
  * @advertising_eee: Currently advertised EEE linkmodes
  * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
+ * @eee_active: phylib private state, indicating that EEE has been negotiated
  * @eee_cfg: User configuration of EEE
  * @lp_advertising: Current link partner advertised linkmodes
  * @host_interfaces: PHY interface modes supported by host
@@ -723,6 +724,7 @@ struct phy_device {
 	/* Energy efficient ethernet modes which should be prohibited */
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_broken_modes);
 	bool enable_tx_lpi;
+	bool eee_active;
 	struct eee_config eee_cfg;
 
 	/* Host supported PHY interface types. Should be ignored if empty. */
-- 
2.30.2

Re: [PATCH net v3] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[Paolo Abeni]

Hi,

On 11/23/24 15:50, Russell King (Oracle) wrote:
> When phy_ethtool_set_eee_noneg() detects a change in the LPI
> parameters, it attempts to update phylib state and trigger the link
> to cycle so the MAC sees the updated parameters.
> 
> However, in doing so, it sets phydev->enable_tx_lpi depending on
> whether the EEE configuration allows the MAC to generate LPI without
> taking into account the result of negotiation.
> 
> This can be demonstrated with a 1000base-T FD interface by:
> 
>  # ethtool --set-eee eno0 advertise 8   # cause EEE to be not negotiated
>  # ethtool --set-eee eno0 tx-lpi off
>  # ethtool --set-eee eno0 tx-lpi on
> 
> This results in being true, despite EEE not having been negotiated and:
>  # ethtool --show-eee eno0
> 	EEE status: enabled - inactive
> 	Tx LPI: 250 (us)
> 	Supported EEE link modes:  100baseT/Full
> 	                           1000baseT/Full
> 	Advertised EEE link modes:  100baseT/Full
> 	                                         1000baseT/Full
> 
> Fix this by keeping track of whether EEE was negotiated via a new
> eee_active member in struct phy_device, and include this state in
> the decision whether phydev->enable_tx_lpi should be set.
> 
> Fixes: 3e43b903da04 ("net: phy: Immediately call adjust_link if only tx_lpi_enabled changes")
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

This patch did not apply net cleanly to net tree when it was submitted,
due to its dependency. As a result it did not went through the CI tests.
Currently there is little material there phy specific - mostly builds
with different Kconfigs - but with time we hope to increase H/W coverage.

AFAICS this patch has no kconfig implication, so my local build should
be a safe-enough test, but please wait for the pre-reqs being merged for
future submissions.

Thanks,

Paolo

Re: [PATCH net v3] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Sat, 23 Nov 2024 14:50:12 +0000 you wrote:
> When phy_ethtool_set_eee_noneg() detects a change in the LPI
> parameters, it attempts to update phylib state and trigger the link
> to cycle so the MAC sees the updated parameters.
> 
> However, in doing so, it sets phydev->enable_tx_lpi depending on
> whether the EEE configuration allows the MAC to generate LPI without
> taking into account the result of negotiation.
> 
> [...]

Here is the summary with links:
  - [net,v3] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI
    https://git.kernel.org/netdev/net/c/e2668c34b7e1

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

Re: [PATCH net v3] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[Russell King (Oracle)]

On Thu, Nov 28, 2024 at 09:44:37AM +0100, Paolo Abeni wrote:
> Hi,
> 
> On 11/23/24 15:50, Russell King (Oracle) wrote:
> > When phy_ethtool_set_eee_noneg() detects a change in the LPI
> > parameters, it attempts to update phylib state and trigger the link
> > to cycle so the MAC sees the updated parameters.
> > 
> > However, in doing so, it sets phydev->enable_tx_lpi depending on
> > whether the EEE configuration allows the MAC to generate LPI without
> > taking into account the result of negotiation.
> > 
> > This can be demonstrated with a 1000base-T FD interface by:
> > 
> >  # ethtool --set-eee eno0 advertise 8   # cause EEE to be not negotiated
> >  # ethtool --set-eee eno0 tx-lpi off
> >  # ethtool --set-eee eno0 tx-lpi on
> > 
> > This results in being true, despite EEE not having been negotiated and:
> >  # ethtool --show-eee eno0
> > 	EEE status: enabled - inactive
> > 	Tx LPI: 250 (us)
> > 	Supported EEE link modes:  100baseT/Full
> > 	                           1000baseT/Full
> > 	Advertised EEE link modes:  100baseT/Full
> > 	                                         1000baseT/Full
> > 
> > Fix this by keeping track of whether EEE was negotiated via a new
> > eee_active member in struct phy_device, and include this state in
> > the decision whether phydev->enable_tx_lpi should be set.
> > 
> > Fixes: 3e43b903da04 ("net: phy: Immediately call adjust_link if only tx_lpi_enabled changes")
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> This patch did not apply net cleanly to net tree when it was submitted,
> due to its dependency. As a result it did not went through the CI tests.
> Currently there is little material there phy specific - mostly builds
> with different Kconfigs - but with time we hope to increase H/W coverage.
> 
> AFAICS this patch has no kconfig implication, so my local build should
> be a safe-enough test, but please wait for the pre-reqs being merged for
> future submissions.

I guess there's no way to tell the CI tests that another patch is
required? It would be useful if something like a message-id could
indicate to the CI tests that the patch in that message-id is
required.

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

Re: [PATCH net v3] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[Paolo Abeni]

On 11/28/24 12:13, Russell King (Oracle) wrote:
> On Thu, Nov 28, 2024 at 09:44:37AM +0100, Paolo Abeni wrote:
>> AFAICS this patch has no kconfig implication, so my local build should
>> be a safe-enough test, but please wait for the pre-reqs being merged for
>> future submissions.
> 
> I guess there's no way to tell the CI tests that another patch is
> required? It would be useful if something like a message-id could
> indicate to the CI tests that the patch in that message-id is
> required.

Not yet. I guess the main road-blocker is the limited time avail for
such developments (mostly Jakub's spare time). Feel free to contribute
to such feature to the CI infra (https://github.com/linux-netdev/nipa),
if time and will allow.

Thanks,

Paolo