[PATCH net-next 0/6] net: phy: improve and simplify EEE handling in phylib

[PATCH net-next 0/6] net: phy: improve and simplify EEE handling in phylib

[Heiner Kallweit]

This series improves and simplifies phylib's EEE handling.

Heiner Kallweit (6):
  net: phy: move definition of phy_is_started before
    phy_disable_eee_mode
  net: phy: improve phy_disable_eee_mode
  net: phy: remove disabled EEE modes from advertising in phy_probe
  net: phy: c45: Don't silently remove disabled EEE modes any longer
    when writing advertisement register
  net: phy: c45: use cached EEE advertisement in
    genphy_c45_ethtool_get_eee
  net: phy: c45: remove local advertisement parameter from
    genphy_c45_eee_is_active

 drivers/net/phy/phy-c45.c    | 40 +++++++++++-------------------------
 drivers/net/phy/phy.c        |  4 ++--
 drivers/net/phy/phy_device.c | 21 +++++++++----------
 include/linux/phy.h          | 20 ++++++++++--------
 4 files changed, 35 insertions(+), 50 deletions(-)

-- 
2.48.1

[PATCH net-next 1/6] net: phy: move definition of phy_is_started before phy_disable_eee_mode

[Heiner Kallweit]

In preparation of a follow-up patch, move phy_is_started() to before
phy_disable_eee_mode().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/phy.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 8efbf62d8..481f8e21f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1330,22 +1330,22 @@ void of_set_phy_timing_role(struct phy_device *phydev);
 int phy_speed_down_core(struct phy_device *phydev);
 
 /**
- * phy_disable_eee_mode - Don't advertise an EEE mode.
+ * phy_is_started - Convenience function to check whether PHY is started
  * @phydev: The phy_device struct
- * @link_mode: The EEE mode to be disabled
  */
-static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
+static inline bool phy_is_started(struct phy_device *phydev)
 {
-	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
+	return phydev->state >= PHY_UP;
 }
 
 /**
- * phy_is_started - Convenience function to check whether PHY is started
+ * phy_disable_eee_mode - Don't advertise an EEE mode.
  * @phydev: The phy_device struct
+ * @link_mode: The EEE mode to be disabled
  */
-static inline bool phy_is_started(struct phy_device *phydev)
+static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
 {
-	return phydev->state >= PHY_UP;
+	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
 }
 
 void phy_resolve_aneg_pause(struct phy_device *phydev);
-- 
2.48.1

[PATCH net-next 2/6] net: phy: improve phy_disable_eee_mode

[Heiner Kallweit]

If a mode is to be disabled, remove it from advertising_eee.
Disabling EEE modes shall be done before calling phy_start(),
warn if that's not the case.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/phy.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 481f8e21f..26a11a0c7 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1345,7 +1345,10 @@ static inline bool phy_is_started(struct phy_device *phydev)
  */
 static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
 {
+	WARN_ON(phy_is_started(phydev));
+
 	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
+	linkmode_clear_bit(link_mode, phydev->advertising_eee);
 }
 
 void phy_resolve_aneg_pause(struct phy_device *phydev);
-- 
2.48.1

[PATCH net-next 3/6] net: phy: remove disabled EEE modes from advertising_eee in phy_probe

[Heiner Kallweit]

A PHY driver may populate eee_disabled_modes in its probe or get_features
callback, therefore filter the EEE advertisement read from the PHY.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 35ec99b4d..103a4d102 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3563,22 +3563,21 @@ static int phy_probe(struct device *dev)
 	if (err)
 		goto out;
 
-	/* There is no "enabled" flag. If PHY is advertising, assume it is
-	 * kind of enabled.
-	 */
-	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
+	/* Get the EEE modes we want to prohibit. */
+	of_set_phy_eee_broken(phydev);
 
 	/* Some PHYs may advertise, by default, not support EEE modes. So,
-	 * we need to clean them.
+	 * we need to clean them. In addition remove all disabled EEE modes.
 	 */
-	if (phydev->eee_cfg.eee_enabled)
-		linkmode_and(phydev->advertising_eee, phydev->supported_eee,
-			     phydev->advertising_eee);
+	linkmode_and(phydev->advertising_eee, phydev->supported_eee,
+		     phydev->advertising_eee);
+	linkmode_andnot(phydev->advertising_eee, phydev->advertising_eee,
+			phydev->eee_disabled_modes);
 
-	/* Get the EEE modes we want to prohibit. We will ask
-	 * the PHY stop advertising these mode later on
+	/* There is no "enabled" flag. If PHY is advertising, assume it is
+	 * kind of enabled.
 	 */
-	of_set_phy_eee_broken(phydev);
+	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
 
 	/* Get master/slave strap overrides */
 	of_set_phy_timing_role(phydev);
-- 
2.48.1

[PATCH net-next 4/6] net: phy: c45: Don't silently remove disabled EEE modes any longer when writing advertisement register

[Heiner Kallweit]

advertising_eee is adjusted now whenever an EEE mode gets disabled.
Therefore we can remove the silent removal of disabled EEE modes here.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index dedbdcab8..b19055cfc 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -683,13 +683,10 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
 static int genphy_c45_write_eee_adv(struct phy_device *phydev,
 				    unsigned long *adv)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp);
 	int val, changed = 0;
 
-	linkmode_andnot(tmp, adv, phydev->eee_disabled_modes);
-
 	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP1_FEATURES)) {
-		val = linkmode_to_mii_eee_cap1_t(tmp);
+		val = linkmode_to_mii_eee_cap1_t(adv);
 
 		/* IEEE 802.3-2018 45.2.7.13 EEE advertisement 1
 		 * (Register 7.60)
@@ -707,7 +704,7 @@ static int genphy_c45_write_eee_adv(struct phy_device *phydev,
 	}
 
 	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES)) {
-		val = linkmode_to_mii_eee_cap2_t(tmp);
+		val = linkmode_to_mii_eee_cap2_t(adv);
 
 		/* IEEE 802.3-2022 45.2.7.16 EEE advertisement 2
 		 * (Register 7.62)
-- 
2.48.1

[PATCH net-next 5/6] net: phy: c45: use cached EEE advertisement in genphy_c45_ethtool_get_eee

[Heiner Kallweit]

Now that disabled EEE modes are considered when populating
advertising_eee, we can use this bitmap here instead of reading
the PHY register.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index b19055cfc..fed5fb0ef 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1516,14 +1516,14 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 {
 	int ret;
 
-	ret = genphy_c45_eee_is_active(phydev, data->advertised,
-				       data->lp_advertised);
+	ret = genphy_c45_eee_is_active(phydev, NULL, data->lp_advertised);
 	if (ret < 0)
 		return ret;
 
 	data->eee_active = phydev->eee_active;
 	linkmode_andnot(data->supported, phydev->supported_eee,
 			phydev->eee_disabled_modes);
+	linkmode_copy(data->advertised, phydev->advertising_eee);
 	return 0;
 }
 EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
-- 
2.48.1

[PATCH net-next 6/6] net: phy: c45: remove local advertisement parameter from genphy_c45_eee_is_active

[Heiner Kallweit]

After the last user has gone, we can remove the local advertisement
parameter from genphy_c45_eee_is_active.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 31 +++++++++----------------------
 drivers/net/phy/phy.c     |  4 ++--
 include/linux/phy.h       |  3 +--
 3 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index fed5fb0ef..37c9a344b 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1464,42 +1464,29 @@ EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status);
 /**
  * genphy_c45_eee_is_active - get EEE status
  * @phydev: target phy_device struct
- * @adv: variable to store advertised linkmodes
  * @lp: variable to store LP advertised linkmodes
  *
- * Description: this function will read local and link partner PHY
- * advertisements. Compare them return current EEE state.
+ * Description: this function will read link partner PHY advertisement
+ * and compare it to local advertisement to return current EEE state.
  */
-int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
-			     unsigned long *lp)
+int genphy_c45_eee_is_active(struct phy_device *phydev, 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_active;
 	int ret;
 
-	ret = genphy_c45_read_eee_adv(phydev, tmp_adv);
-	if (ret)
-		return ret;
-
 	ret = genphy_c45_read_eee_lpa(phydev, tmp_lp);
 	if (ret)
 		return ret;
 
-	linkmode_and(common, tmp_adv, tmp_lp);
-	if (!linkmode_empty(tmp_adv) && !linkmode_empty(common))
-		eee_active = phy_check_valid(phydev->speed, phydev->duplex,
-					     common);
-	else
-		eee_active = false;
-
-	if (adv)
-		linkmode_copy(adv, tmp_adv);
 	if (lp)
 		linkmode_copy(lp, tmp_lp);
 
-	return eee_active;
+	linkmode_and(common, phydev->advertising_eee, tmp_lp);
+	if (linkmode_empty(common))
+		return 0;
+
+	return phy_check_valid(phydev->speed, phydev->duplex, common);
 }
 EXPORT_SYMBOL(genphy_c45_eee_is_active);
 
@@ -1516,7 +1503,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 {
 	int ret;
 
-	ret = genphy_c45_eee_is_active(phydev, NULL, data->lp_advertised);
+	ret = genphy_c45_eee_is_active(phydev, data->lp_advertised);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 77b3fb843..b454e31d4 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1031,7 +1031,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);
+		err = genphy_c45_eee_is_active(phydev, NULL);
 		phydev->eee_active = err > 0;
 		phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled &&
 					phydev->eee_active;
@@ -1761,7 +1761,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);
+	ret = genphy_c45_eee_is_active(phydev, NULL);
 	if (ret < 0)
 		return ret;
 	if (!ret)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 26a11a0c7..c0f524579 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2022,8 +2022,7 @@ int genphy_c45_plca_set_cfg(struct phy_device *phydev,
 			    const struct phy_plca_cfg *plca_cfg);
 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);
+int genphy_c45_eee_is_active(struct phy_device *phydev, 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,
-- 
2.48.1

Re: [PATCH net-next 1/6] net: phy: move definition of phy_is_started before phy_disable_eee_mode

[Andrew Lunn]

On Sun, Feb 16, 2025 at 10:15:42PM +0100, Heiner Kallweit wrote:
> In preparation of a follow-up patch, move phy_is_started() to before
> phy_disable_eee_mode().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 2/6] net: phy: improve phy_disable_eee_mode

[Andrew Lunn]

On Sun, Feb 16, 2025 at 10:16:34PM +0100, Heiner Kallweit wrote:
> If a mode is to be disabled, remove it from advertising_eee.
> Disabling EEE modes shall be done before calling phy_start(),
> warn if that's not the case.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 3/6] net: phy: remove disabled EEE modes from advertising_eee in phy_probe

[Andrew Lunn]

On Sun, Feb 16, 2025 at 10:17:48PM +0100, Heiner Kallweit wrote:
> A PHY driver may populate eee_disabled_modes in its probe or get_features
> callback, therefore filter the EEE advertisement read from the PHY.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 5/6] net: phy: c45: use cached EEE advertisement in genphy_c45_ethtool_get_eee

[Andrew Lunn]

On Sun, Feb 16, 2025 at 10:19:23PM +0100, Heiner Kallweit wrote:
> Now that disabled EEE modes are considered when populating
> advertising_eee, we can use this bitmap here instead of reading
> the PHY register.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 6/6] net: phy: c45: remove local advertisement parameter from genphy_c45_eee_is_active

[Andrew Lunn]

On Sun, Feb 16, 2025 at 10:20:07PM +0100, Heiner Kallweit wrote:
> After the last user has gone, we can remove the local advertisement
> parameter from genphy_c45_eee_is_active.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/6] net: phy: move definition of phy_is_started before phy_disable_eee_mode

[Russell King (Oracle)]

On Sun, Feb 16, 2025 at 10:15:42PM +0100, Heiner Kallweit wrote:
> In preparation of a follow-up patch, move phy_is_started() to before
> phy_disable_eee_mode().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
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-next 2/6] net: phy: improve phy_disable_eee_mode

[Russell King (Oracle)]

On Sun, Feb 16, 2025 at 10:16:34PM +0100, Heiner Kallweit wrote:
> If a mode is to be disabled, remove it from advertising_eee.
> Disabling EEE modes shall be done before calling phy_start(),
> warn if that's not the case.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
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-next 3/6] net: phy: remove disabled EEE modes from advertising_eee in phy_probe

[Russell King (Oracle)]

On Sun, Feb 16, 2025 at 10:17:48PM +0100, Heiner Kallweit wrote:
> A PHY driver may populate eee_disabled_modes in its probe or get_features
> callback, therefore filter the EEE advertisement read from the PHY.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
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-next 4/6] net: phy: c45: Don't silently remove disabled EEE modes any longer when writing advertisement register

[Russell King (Oracle)]

On Sun, Feb 16, 2025 at 10:18:42PM +0100, Heiner Kallweit wrote:
> advertising_eee is adjusted now whenever an EEE mode gets disabled.
> Therefore we can remove the silent removal of disabled EEE modes here.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
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-next 5/6] net: phy: c45: use cached EEE advertisement in genphy_c45_ethtool_get_eee

[Russell King (Oracle)]

On Sun, Feb 16, 2025 at 10:19:23PM +0100, Heiner Kallweit wrote:
> Now that disabled EEE modes are considered when populating
> advertising_eee, we can use this bitmap here instead of reading
> the PHY register.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
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-next 6/6] net: phy: c45: remove local advertisement parameter from genphy_c45_eee_is_active

[Russell King (Oracle)]

On Sun, Feb 16, 2025 at 10:20:07PM +0100, Heiner Kallweit wrote:
> After the last user has gone, we can remove the local advertisement
> parameter from genphy_c45_eee_is_active.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
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-next 0/6] net: phy: improve and simplify EEE handling in phylib

[patchwork-bot+netdevbpf]

Hello:

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

On Sun, 16 Feb 2025 22:14:56 +0100 you wrote:
> This series improves and simplifies phylib's EEE handling.
> 
> Heiner Kallweit (6):
>   net: phy: move definition of phy_is_started before
>     phy_disable_eee_mode
>   net: phy: improve phy_disable_eee_mode
>   net: phy: remove disabled EEE modes from advertising in phy_probe
>   net: phy: c45: Don't silently remove disabled EEE modes any longer
>     when writing advertisement register
>   net: phy: c45: use cached EEE advertisement in
>     genphy_c45_ethtool_get_eee
>   net: phy: c45: remove local advertisement parameter from
>     genphy_c45_eee_is_active
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] net: phy: move definition of phy_is_started before phy_disable_eee_mode
    https://git.kernel.org/netdev/net-next/c/8a6a77bb5a41
  - [net-next,2/6] net: phy: improve phy_disable_eee_mode
    https://git.kernel.org/netdev/net-next/c/a9b6a860d778
  - [net-next,3/6] net: phy: remove disabled EEE modes from advertising_eee in phy_probe
    https://git.kernel.org/netdev/net-next/c/7f33fea6bb53
  - [net-next,4/6] net: phy: c45: Don't silently remove disabled EEE modes any longer when writing advertisement register
    https://git.kernel.org/netdev/net-next/c/aa951feb5426
  - [net-next,5/6] net: phy: c45: use cached EEE advertisement in genphy_c45_ethtool_get_eee
    https://git.kernel.org/netdev/net-next/c/199d0ce385ad
  - [net-next,6/6] net: phy: c45: remove local advertisement parameter from genphy_c45_eee_is_active
    https://git.kernel.org/netdev/net-next/c/809265fe96fe

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