Skip to contentSkip to conversation

net: phylib EEE cleanups

PATCH Applied Russell King (Oracle) 4 participants 10 messages Dec 5, 2024 – Dec 7, 2024

Subsystems (3)
Networking Network drivers Ethernet PHY
lore
Share & tools
lore

Russell King (Oracle)

author reviewer
#0 lore raw

cover net: phylib EEE cleanups

Hi,

Clean up phylib's EEE support. Patches previously posted as RFC as part of the phylink EEE series.

Patch 1 changes the Marvell driver to use the state we store in struct phy_device, rather than manually calling phydev->eee_cfg.eee_enabled.

Patch 2 avoids genphy_c45_ethtool_get_eee() setting ->eee_enabled, as we copy that from phydev->eee_cfg.eee_enabled later, and after patch 3 mo one uses this after calling genphy_c45_ethtool_get_eee(). In fact, the only caller of this function now is phy_ethtool_get_eee().

As all callers to genphy_c45_eee_is_active() now pass NULL as its is_enabled flag, this is no longer useful. Remove the argument in patch 3.

Patch 4 updates the phylib documentation to make it absolutely clear that phy_ethtool_get_eee() now fills in all members of struct ethtool_keee, which is why we now have so many buggy network drivers.

 drivers/net/phy/marvell.c |  4 +---
 drivers/net/phy/phy-c45.c | 14 ++++----------
 drivers/net/phy/phy.c     |  9 ++++-----
 include/linux/phy.h       |  2 +-
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#1 lore raw

PATCH 1/4 net: phy: marvell: use phydev->eee_cfg.eee_enabled

Rather than calling genphy_c45_ethtool_get_eee() to retrieve whether EEE is enabled, use the value stored in the phy_device eee_cfg structure.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/marvell.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/net/phy/marvell.c +1 -3
index b885bc0fe6e0..ffe223ad9e5f 100644
@@ -1550,7 +1550,6 @@ static int m88e1540_get_fld(struct phy_device *phydev, u8 *msecs)
 
 static int m88e1540_set_fld(struct phy_device *phydev, const u8 *msecs)
 {
-	struct ethtool_keee eee;
 	int val, ret;
 
 	if (*msecs == ETHTOOL_PHY_FAST_LINK_DOWN_OFF)

@@ -1560,8 +1559,7 @@ static int m88e1540_set_fld(struct phy_device *phydev, const u8 *msecs)
 	/* According to the Marvell data sheet EEE must be disabled for
 	 * Fast Link Down detection to work properly
 	 */
-	ret = genphy_c45_ethtool_get_eee(phydev, &eee);
-	if (!ret && eee.eee_enabled) {
+	if (phydev->eee_cfg.eee_enabled) {
 		phydev_warn(phydev, "Fast Link Down detection requires EEE to be disabled!\n");
 		return -EBUSY;
 	}
Signature · 1 line
2.30.2
#3 lore raw

PATCH 2/4 net: phy: avoid genphy_c45_ethtool_get_eee() setting eee_enabled

In reply to Russell King (Oracle)

genphy_c45_ethtool_get_eee() is only called from phy_ethtool_get_eee(), which then calls eeecfg_to_eee(). eeecfg_to_eee() will overwrite keee.eee_enabled, so there's no point setting keee.eee_enabled in genphy_c45_ethtool_get_eee(). Remove this assignment.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phy-c45.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/net/phy/phy-c45.c +1 -3
index 944ae98ad110..d162f78bc68d 100644
@@ -1521,15 +1521,13 @@ EXPORT_SYMBOL(genphy_c45_eee_is_active);
 int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 			       struct ethtool_keee *data)
 {
-	bool is_enabled;
 	int ret;
 
 	ret = genphy_c45_eee_is_active(phydev, data->advertised,
-				       data->lp_advertised, &is_enabled);
+				       data->lp_advertised, NULL);
 	if (ret < 0)
 		return ret;
 
-	data->eee_enabled = is_enabled;
 	data->eee_active = phydev->eee_active;
 	linkmode_copy(data->supported, phydev->supported_eee);
Signature · 1 line
2.30.2
#5 lore raw

PATCH 3/4 net: phy: remove genphy_c45_eee_is_active()'s is_enabled arg

In reply to Russell King (Oracle)

All callers to genphy_c45_eee_is_active() now pass NULL as the is_enabled argument, which means we never use the value computed in this function. Remove the argument and clean up this function.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phy-c45.c | 12 ++++--------
 drivers/net/phy/phy.c     |  5 ++---
 include/linux/phy.h       |  2 +-
 3 files changed, 7 insertions(+), 12 deletions(-)
drivers/net/phy/phy-c45.c +4 -8
index d162f78bc68d..0dac08e85304 100644
@@ -1469,18 +1469,17 @@ EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status);
  * @phydev: target phy_device struct
  * @adv: variable to store advertised linkmodes
  * @lp: variable to store LP advertised linkmodes
- * @is_enabled: variable to store EEE enabled/disabled configuration value
  *
  * Description: this function will read local and link partner PHY
  * advertisements. Compare them return current EEE state.
  */
 int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
-			     unsigned long *lp, bool *is_enabled)
+			     unsigned long *lp)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp_adv) = {};
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp_lp) = {};
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
-	bool eee_enabled, eee_active;
+	bool eee_active;
 	int ret;
 
 	ret = genphy_c45_read_eee_adv(phydev, tmp_adv);

@@ -1491,9 +1490,8 @@ int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
 	if (ret)
 		return ret;
 
-	eee_enabled = !linkmode_empty(tmp_adv);
 	linkmode_and(common, tmp_adv, tmp_lp);
-	if (eee_enabled && !linkmode_empty(common))
+	if (!linkmode_empty(tmp_adv) && !linkmode_empty(common))
 		eee_active = phy_check_valid(phydev->speed, phydev->duplex,
 					     common);
 	else

@@ -1503,8 +1501,6 @@ int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
 		linkmode_copy(adv, tmp_adv);
 	if (lp)
 		linkmode_copy(lp, tmp_lp);
-	if (is_enabled)
-		*is_enabled = eee_enabled;
 
 	return eee_active;
 }

@@ -1524,7 +1520,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 	int ret;
 
 	ret = genphy_c45_eee_is_active(phydev, data->advertised,
-				       data->lp_advertised, NULL);
+				       data->lp_advertised);
 	if (ret < 0)
 		return ret;
drivers/net/phy/phy.c +2 -3
index 0c228aa18019..4cf344254237 100644
@@ -988,8 +988,7 @@ static int phy_check_link_status(struct phy_device *phydev)
 	if (phydev->link && phydev->state != PHY_RUNNING) {
 		phy_check_downshift(phydev);
 		phydev->state = PHY_RUNNING;
-		err = genphy_c45_eee_is_active(phydev,
-					       NULL, NULL, NULL);
+		err = genphy_c45_eee_is_active(phydev, NULL, NULL);
 		phydev->eee_active = err > 0;
 		phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled &&
 					phydev->eee_active;

@@ -1658,7 +1657,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 	if (!phydev->drv)
 		return -EIO;
 
-	ret = genphy_c45_eee_is_active(phydev, NULL, NULL, NULL);
+	ret = genphy_c45_eee_is_active(phydev, NULL, NULL);
 	if (ret < 0)
 		return ret;
 	if (!ret)
include/linux/phy.h +1 -1
index 61a1bc81f597..bb157136351e 100644
@@ -1991,7 +1991,7 @@ int genphy_c45_plca_set_cfg(struct phy_device *phydev,
 int genphy_c45_plca_get_status(struct phy_device *phydev,
 			       struct phy_plca_status *plca_st);
 int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
-			     unsigned long *lp, bool *is_enabled);
+			     unsigned long *lp);
 int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 			       struct ethtool_keee *data);
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
Signature · 1 line
2.30.2
#7 lore raw

PATCH 4/4 net: phy: update phy_ethtool_get_eee() documentation

In reply to Russell King (Oracle)

Update the phy_ethtool_get_eee() documentation to make it clear that all members of struct ethtool_keee are written by this function.

keee.supported, keee.advertised, keee.lp_advertised and keee.eee_active are all written by genphy_c45_ethtool_get_eee().

keee.tx_lpi_timer, keee.tx_lpi_enabled and keee.eee_enabled are all written by eeecfg_to_eee().

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/phy/phy.c +2 -2
index 4cf344254237..e4b04cdaa995 100644
@@ -1701,8 +1701,8 @@ EXPORT_SYMBOL(phy_get_eee_err);
  * @phydev: target phy_device struct
  * @data: ethtool_keee data
  *
- * Description: reports the Supported/Advertisement/LP Advertisement
- * capabilities, etc.
+ * Description: get the current EEE settings, filling in all members of
+ * @data.
  */
 int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
 {
Signature · 1 line
2.30.2
author reviewer Reviewed-by Heiner Kallweit on v1 1/4
#2 lore raw

Re: [PATCH net-next 1/4] net: phy: marvell: use phydev->eee_cfg.eee_enabled

In reply to Russell King (Oracle)
Quoted 5 lines · Russell King (Oracle)

Rather than calling genphy_c45_ethtool_get_eee() to retrieve whether EEE is enabled, use the value stored in the phy_device eee_cfg structure.

Signed-off-by Russell King (Oracle) <[email protected]>

I seem to have missed adding Heiner's r-b given in the RFC posting:

Reviewed-by Heiner Kallweit <[email protected]>
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
author reviewer Reviewed-by Heiner Kallweit on v1 2/4
#4 lore raw

Re: [PATCH net-next 2/4] net: phy: avoid genphy_c45_ethtool_get_eee() setting eee_enabled

In reply to Russell King (Oracle)
Quoted 6 lines · Russell King (Oracle)

genphy_c45_ethtool_get_eee() is only called from phy_ethtool_get_eee(), which then calls eeecfg_to_eee(). eeecfg_to_eee() will overwrite keee.eee_enabled, so there's no point setting keee.eee_enabled in genphy_c45_ethtool_get_eee(). Remove this assignment.

Signed-off-by Russell King (Oracle) <[email protected]>

I seem to have missed adding Heiner's r-b given in the RFC posting:

Reviewed-by Heiner Kallweit <[email protected]>
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
author reviewer Reviewed-by Heiner Kallweit on v1 3/4
#6 lore raw

Re: [PATCH net-next 3/4] net: phy: remove genphy_c45_eee_is_active()'s is_enabled arg

In reply to Russell King (Oracle)
Quoted 5 lines · Russell King (Oracle)

All callers to genphy_c45_eee_is_active() now pass NULL as the is_enabled argument, which means we never use the value computed in this function. Remove the argument and clean up this function.

Signed-off-by Russell King (Oracle) <[email protected]>

I seem to have missed adding Heiner's r-b given in the RFC posting:

Reviewed-by Heiner Kallweit <[email protected]>
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
maintainer Reviewed-by on v1 4/4
#8 lore raw

Re: [PATCH net-next 4/4] net: phy: update phy_ethtool_get_eee() documentation

In reply to Russell King (Oracle)
Quoted 10 lines · Russell King (Oracle)

Update the phy_ethtool_get_eee() documentation to make it clear that all members of struct ethtool_keee are written by this function.

keee.supported, keee.advertised, keee.lp_advertised and keee.eee_active are all written by genphy_c45_ethtool_get_eee().

keee.tx_lpi_timer, keee.tx_lpi_enabled and keee.eee_enabled are all written by eeecfg_to_eee().

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

CI Applied
#9 lore raw
In reply to Russell King (Oracle)
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 10 lines · On Thu, 5 Dec 2024 10:41:42 +0000 you wrote:

Hi,

Clean up phylib's EEE support. Patches previously posted as RFC as part of the phylink EEE series.

Patch 1 changes the Marvell driver to use the state we store in struct phy_device, rather than manually calling phydev->eee_cfg.eee_enabled.

[...]

Here is the summary with links:
- [net-next,1/4] net: phy: marvell: use phydev->eee_cfg.eee_enabled

    https://git.kernel.org/netdev/net-next/c/bac3d0f21c5a
  - [net-next,2/4] net: phy: avoid genphy_c45_ethtool_get_eee() setting eee_enabled
    https://git.kernel.org/netdev/net-next/c/92f7acb825ec
  - [net-next,3/4] net: phy: remove genphy_c45_eee_is_active()'s is_enabled arg
    https://git.kernel.org/netdev/net-next/c/8f1c716090a7
  - [net-next,4/4] net: phy: update phy_ethtool_get_eee() documentation
    https://git.kernel.org/netdev/net-next/c/f899c594e138

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html