[PATCH net-next v3 00/10] net: phy: improve phylib EEE handling

[PATCH net-next v3 00/10] net: phy: improve phylib EEE handling

[Heiner Kallweit]

This series includes improvements for the EEE handling in phylib.

v2:
- add patch 3
- patch 4:
  - silently filter out disabled EEE modes
  - add extack user hint if requested EEE advertisement includes
    disabled modes

v3:
- patch4:
  - remove trailing newline from NL_SET_ERR_MSG message

Heiner Kallweit (10):
  net: phy: rename eee_broken_modes to eee_disabled_modes
  net: phy: rename phy_set_eee_broken to phy_disable_eee_mode
  ethtool: allow ethtool op set_eee to set an NL extack message
  net: phy: c45: improve handling of disabled EEE modes in ethtool
    functions
  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/ethernet/realtek/r8169_main.c |  6 +--
 drivers/net/phy/phy-c45.c                 | 51 +++++++++--------------
 drivers/net/phy/phy-core.c                |  2 +-
 drivers/net/phy/phy.c                     |  4 +-
 drivers/net/phy/phy_device.c              | 23 +++++-----
 include/linux/ethtool.h                   |  1 +
 include/linux/phy.h                       | 24 ++++++-----
 net/ethtool/eee.c                         |  2 +-
 8 files changed, 52 insertions(+), 61 deletions(-)

-- 
2.47.1

[PATCH net-next v3 01/10] net: phy: rename eee_broken_modes to eee_disabled_modes

[Heiner Kallweit]

This bitmap is used also if the MAC doesn't support an EEE mode.
So the mode isn't necessarily broken in the PHY. Therefore rename
the bitmap.

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

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 0dac08e85..468d24611 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -686,7 +686,7 @@ static int genphy_c45_write_eee_adv(struct phy_device *phydev,
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp);
 	int val, changed = 0;
 
-	linkmode_andnot(tmp, adv, phydev->eee_broken_modes);
+	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);
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 6bf3ec985..beeb0ef2f 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -388,7 +388,7 @@ void of_set_phy_supported(struct phy_device *phydev)
 void of_set_phy_eee_broken(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
-	unsigned long *modes = phydev->eee_broken_modes;
+	unsigned long *modes = phydev->eee_disabled_modes;
 
 	if (!IS_ENABLED(CONFIG_OF_MDIO) || !node)
 		return;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bdc997f59..f6a5f986f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3016,7 +3016,7 @@ void phy_disable_eee(struct phy_device *phydev)
 	phydev->eee_cfg.tx_lpi_enabled = false;
 	phydev->eee_cfg.eee_enabled = false;
 	/* don't let userspace re-enable EEE advertisement */
-	linkmode_fill(phydev->eee_broken_modes);
+	linkmode_fill(phydev->eee_disabled_modes);
 }
 EXPORT_SYMBOL_GPL(phy_disable_eee);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5bc71d599..c5dc2dbf0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -629,7 +629,7 @@ struct macsec_ops;
  * @eee_cfg: User configuration of EEE
  * @lp_advertising: Current link partner advertised linkmodes
  * @host_interfaces: PHY interface modes supported by host
- * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
+ * @eee_disabled_modes: Energy efficient ethernet modes not to be advertised
  * @autoneg: Flag autoneg being used
  * @rate_matching: Current rate matching mode
  * @link: Current link state
@@ -745,7 +745,7 @@ struct phy_device {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee);
 	/* Energy efficient ethernet modes which should be prohibited */
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_broken_modes);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_disabled_modes);
 	bool enable_tx_lpi;
 	bool eee_active;
 	struct eee_config eee_cfg;
@@ -1324,7 +1324,7 @@ int phy_speed_down_core(struct phy_device *phydev);
  */
 static inline void phy_set_eee_broken(struct phy_device *phydev, u32 link_mode)
 {
-	linkmode_set_bit(link_mode, phydev->eee_broken_modes);
+	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
 }
 
 /**
-- 
2.47.1

[PATCH net-next v3 02/10] net: phy: rename phy_set_eee_broken to phy_disable_eee_mode

[Heiner Kallweit]

Consider that an EEE mode may not be broken but simply not supported
by the MAC, and rename function phy_set_eee_broken().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 6 +++---
 include/linux/phy.h                       | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5724f650f..bf368b32c 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5222,9 +5222,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
 
 	/* mimic behavior of r8125/r8126 vendor drivers */
 	if (tp->mac_version == RTL_GIGA_MAC_VER_61)
-		phy_set_eee_broken(tp->phydev,
-				   ETHTOOL_LINK_MODE_2500baseT_Full_BIT);
-	phy_set_eee_broken(tp->phydev, ETHTOOL_LINK_MODE_5000baseT_Full_BIT);
+		phy_disable_eee_mode(tp->phydev,
+				     ETHTOOL_LINK_MODE_2500baseT_Full_BIT);
+	phy_disable_eee_mode(tp->phydev, ETHTOOL_LINK_MODE_5000baseT_Full_BIT);
 
 	/* PHY will be woken up in rtl_open() */
 	phy_suspend(tp->phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c5dc2dbf0..7138bb074 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1318,11 +1318,11 @@ void of_set_phy_timing_role(struct phy_device *phydev);
 int phy_speed_down_core(struct phy_device *phydev);
 
 /**
- * phy_set_eee_broken - Mark an EEE mode as broken so that it isn't advertised.
+ * phy_disable_eee_mode - Don't advertise an EEE mode.
  * @phydev: The phy_device struct
- * @link_mode: The broken EEE mode
+ * @link_mode: The EEE mode to be disabled
  */
-static inline void phy_set_eee_broken(struct phy_device *phydev, u32 link_mode)
+static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
 {
 	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
 }
-- 
2.47.1

[PATCH net-next v3 03/10] ethtool: allow ethtool op set_eee to set an NL extack message

[Heiner Kallweit]

Disabled EEE modes (e.g. because not supported by the MAC) are silently
filtered out by phylib's set_eee implementation. For being able to
present a hint to the user, expose extack as part of struct ethtool_keee.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/ethtool.h | 1 +
 net/ethtool/eee.c       | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f711bfd75..8ee047747 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -270,6 +270,7 @@ struct ethtool_keee {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertised);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised);
+	struct netlink_ext_ack *extack;
 	u32	tx_lpi_timer;
 	bool	tx_lpi_enabled;
 	bool	eee_active;
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index bf398973e..6546d7290 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -129,7 +129,7 @@ ethnl_set_eee(struct ethnl_req_info *req_info, struct genl_info *info)
 {
 	struct net_device *dev = req_info->dev;
 	struct nlattr **tb = info->attrs;
-	struct ethtool_keee eee = {};
+	struct ethtool_keee eee = { .extack = info->extack };
 	bool mod = false;
 	int ret;
 
-- 
2.47.1

[PATCH net-next v3 04/10] net: phy: c45: improve handling of disabled EEE modes in ethtool functions

[Heiner Kallweit]

Currently disabled EEE modes are shown as supported in ethtool.
Change this by filtering them out when populating data->supported
in genphy_c45_ethtool_get_eee.
Disabled EEE modes are filtered out by genphy_c45_write_eee_adv.
This is planned to be removed, therefore ensure in
genphy_c45_ethtool_set_eee that disabled EEE modes are silently
removed from the user spaces provided EEE advertisement. Add a
hint to the user so that it is done not that silently any longer.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- silently filter out disabled EEE modes
- add extack user hint if requested EEE advertisement includes
  disabled modes
v3:
- remove trailing newline from NL_SET_ERR_MSG message
---
 drivers/net/phy/phy-c45.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 468d24611..2335f4ad1 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1525,8 +1525,8 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 		return ret;
 
 	data->eee_active = phydev->eee_active;
-	linkmode_copy(data->supported, phydev->supported_eee);
-
+	linkmode_andnot(data->supported, phydev->supported_eee,
+			phydev->eee_disabled_modes);
 	return 0;
 }
 EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
@@ -1559,7 +1559,12 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 				phydev_warn(phydev, "At least some EEE link modes are not supported.\n");
 				return -EINVAL;
 			}
-			linkmode_copy(phydev->advertising_eee, adv);
+
+			linkmode_andnot(phydev->advertising_eee, adv,
+					phydev->eee_disabled_modes);
+			if (!linkmode_equal(phydev->advertising_eee, adv))
+				NL_SET_ERR_MSG(data->extack,
+					       "Requested EEE advertisement includes disabled modes");
 		} else if (linkmode_empty(phydev->advertising_eee)) {
 			phy_advertise_eee_all(phydev);
 		}
-- 
2.47.1

[PATCH net-next v3 05/10] 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 7138bb074..ad71d3a3b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1318,22 +1318,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.47.1

[PATCH net-next v3 06/10] 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 ad71d3a3b..fce29aaa9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1333,7 +1333,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.47.1

[PATCH net-next v3 07/10] net: phy: remove disabled EEE modes from advertising 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 f6a5f986f..ff4b4d42b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3613,22 +3613,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.47.1

[PATCH net-next v3 08/10] 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 2335f4ad1..904a10c02 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.47.1

[PATCH net-next v3 09/10] 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 904a10c02..0fb5e20f3 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.47.1

[PATCH net-next v3 10/10] 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 0fb5e20f3..9f2d8987c 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 e4b04cdaa..5812a3f12 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -988,7 +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);
+		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;
@@ -1657,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);
+	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 fce29aaa9..5a6dcbd8e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2016,8 +2016,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.47.1

Re: [PATCH net-next v3 03/10] ethtool: allow ethtool op set_eee to set an NL extack message

[Jakub Kicinski]

On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote:
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index f711bfd75..8ee047747 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -270,6 +270,7 @@ struct ethtool_keee {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertised);
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised);
> +	struct netlink_ext_ack *extack;
>  	u32	tx_lpi_timer;
>  	bool	tx_lpi_enabled;
>  	bool	eee_active;

:S I don't think we have a precedent for passing extack inside 
the paramter struct. I see 25 .set_eee callbacks, not crazy many.
Could you plumb this thru as a separate argument, please?
-- 
pw-bot: cr

Re: [PATCH net-next v3 03/10] ethtool: allow ethtool op set_eee to set an NL extack message

[Russell King (Oracle)]

On Tue, Jan 14, 2025 at 03:00:43PM -0800, Jakub Kicinski wrote:
> On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote:
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index f711bfd75..8ee047747 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -270,6 +270,7 @@ struct ethtool_keee {
> >  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> >  	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertised);
> >  	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised);
> > +	struct netlink_ext_ack *extack;
> >  	u32	tx_lpi_timer;
> >  	bool	tx_lpi_enabled;
> >  	bool	eee_active;
> 
> :S I don't think we have a precedent for passing extack inside 
> the paramter struct. I see 25 .set_eee callbacks, not crazy many.
> Could you plumb this thru as a separate argument, please?

I was going to wait for this before reworking the phylink based EEE
support, but as it looks like this is going to be a while before it's
merged, I'll rework my series based on this not being merged and
post it non-RFC this afternoon.

-- 
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 v3 03/10] ethtool: allow ethtool op set_eee to set an NL extack message

[Heiner Kallweit]

On 15.01.2025 00:00, Jakub Kicinski wrote:
> On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote:
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index f711bfd75..8ee047747 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -270,6 +270,7 @@ struct ethtool_keee {
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertised);
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised);
>> +	struct netlink_ext_ack *extack;
>>  	u32	tx_lpi_timer;
>>  	bool	tx_lpi_enabled;
>>  	bool	eee_active;
> 
> :S I don't think we have a precedent for passing extack inside 
> the paramter struct. I see 25 .set_eee callbacks, not crazy many.
> Could you plumb this thru as a separate argument, please?

I see your point regarding calling convention consistency.
Drawback of passing extack as a separate argument is that we would
have to do the same extension also to functions in phylib.
Affected are phy_ethtool_set_eee and genphy_c45_ethtool_set_eee,
because extack is to be used in the latter.
Passing extack within struct ethtool_keee we don't have to change
the functions in the call chain. So passing extack separately
comes at a cost. Is it worth it?

Re: [PATCH net-next v3 03/10] ethtool: allow ethtool op set_eee to set an NL extack message

[Jakub Kicinski]

On Wed, 15 Jan 2025 18:46:35 +0100 Heiner Kallweit wrote:
> On 15.01.2025 00:00, Jakub Kicinski wrote:
> > On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote:  
> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> >> index f711bfd75..8ee047747 100644
> >> --- a/include/linux/ethtool.h
> >> +++ b/include/linux/ethtool.h
> >> @@ -270,6 +270,7 @@ struct ethtool_keee {
> >>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> >>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertised);
> >>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised);
> >> +	struct netlink_ext_ack *extack;
> >>  	u32	tx_lpi_timer;
> >>  	bool	tx_lpi_enabled;
> >>  	bool	eee_active;  
> > 
> > :S I don't think we have a precedent for passing extack inside 
> > the paramter struct. I see 25 .set_eee callbacks, not crazy many.
> > Could you plumb this thru as a separate argument, please?  
> 
> I see your point regarding calling convention consistency.
> Drawback of passing extack as a separate argument is that we would
> have to do the same extension also to functions in phylib.
> Affected are phy_ethtool_set_eee and genphy_c45_ethtool_set_eee,
> because extack is to be used in the latter.
> Passing extack within struct ethtool_keee we don't have to change
> the functions in the call chain. So passing extack separately
> comes at a cost. Is it worth it?

I doubt it will be uglier than stuffing transient pointers into a config
struct. But we will only know for sure once the code is written..

Re: [PATCH net-next v3 03/10] ethtool: allow ethtool op set_eee to set an NL extack message

[Heiner Kallweit]

On 15.01.2025 00:00, Jakub Kicinski wrote:
> On Sun, 12 Jan 2025 14:28:22 +0100 Heiner Kallweit wrote:
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index f711bfd75..8ee047747 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -270,6 +270,7 @@ struct ethtool_keee {
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertised);
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised);
>> +	struct netlink_ext_ack *extack;
>>  	u32	tx_lpi_timer;
>>  	bool	tx_lpi_enabled;
>>  	bool	eee_active;
> 
> :S I don't think we have a precedent for passing extack inside 
> the paramter struct. I see 25 .set_eee callbacks, not crazy many.
> Could you plumb this thru as a separate argument, please?

Thought about alternatives:
struct ethtool_netdev_state may be a good candidate for passing
extack to ethtool ops. Code below does this for all "set" ops,
as a starting point. This approach may even allow us to remove
the extack argument from a number of existing ethtool ops,
incl. static functions used within these ops.
Would this approach be acceptable?

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 870994cc3..28acb9224 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1171,12 +1171,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
  * @rss_ctx:           XArray of custom RSS contexts
  * @rss_lock:          Protects entries in @rss_ctx.  May be taken from
  *                     within RTNL.
+ * @extack:            For passing netlink error messages
  * @wol_enabled:       Wake-on-LAN is enabled
  * @module_fw_flash_in_progress: Module firmware flashing is in progress.
  */
 struct ethtool_netdev_state {
        struct xarray           rss_ctx;
        struct mutex            rss_lock;
+       struct netlink_ext_ack  *extack;
        unsigned                wol_enabled:1;
        unsigned                module_fw_flash_in_progress:1;
 };
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index b4c45207f..0cc22c482 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -704,7 +704,10 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
        if (ret < 0)
                goto out_free_cfg;

+       dev->ethtool->extack = info->extack;
        ret = ops->set(&req_info, info);
+       dev->ethtool->extack = NULL;
+
        if (ret < 0)
                goto out_ops;

Re: [PATCH net-next v3 03/10] ethtool: allow ethtool op set_eee to set an NL extack message

[Jakub Kicinski]

On Sun, 9 Feb 2025 00:22:08 +0100 Heiner Kallweit wrote:
> Would this approach be acceptable?

I don't think so. We considered this for ndos, IIRC, global state can
lead to bugs.