[PATCH net-next 0/5] ethtool: add support for EEE linkmodes beyond bit 32

[PATCH net-next 0/5] ethtool: add support for EEE linkmodes beyond bit 32

[Heiner Kallweit]

So far only 32bit legacy bitmaps are passed to userspace. This makes
it impossible to manage EEE linkmodes beyond bit 32, e.g. manage EEE
for 2500BaseT and 5000BaseT. This series adds support for passing
full linkmode bitmaps between kernel and userspace.

Fortunately the netlink-based part of ethtool is quite smart and no
changes are needed in ethtool. However this applies to the netlink
interface only, the ioctl interface for now remains restricted to
legacy bitmaps.

Next step will be adding support for the c45 EEE2 standard registers
(3.21, 7.62, 7.63) to the genphy_c45 functions dealing with EEE.
I have a follow-up series for this ready to be submitted.

Heiner Kallweit (5):
  ethtool: add struct ethtool_keee and extend struct ethtool_eee
  ethtool: add basic handling of struct ethtool_keee
  ethtool: send EEE linkmode bitmaps to userspace
  net: phy: c45: prepare genphy_c45_ethtool_set_eee for follow-up
    extension
  net: phy: c45: extend genphy_c45_ethtool_[set|get]_eee

 drivers/net/phy/phy-c45.c    | 57 +++++++++++++++++----------
 include/linux/ethtool.h      | 18 +++++++++
 include/uapi/linux/ethtool.h |  4 +-
 net/ethtool/eee.c            | 75 +++++++++++++++++++++++++-----------
 4 files changed, 109 insertions(+), 45 deletions(-)

-- 
2.43.0

[PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Heiner Kallweit]

In order to pass EEE link modes beyond bit 32 to userspace we have to
complement the 32 bit bitmaps in struct ethtool_eee with linkmode
bitmaps. Therefore, similar to ethtool_link_settings and
ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of
the reserved fields in struct ethtool_eee as flag that an instance
of struct ethtool_eee is embedded in a struct ethtool_keee, thus the
linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor.

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

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index cfcd952a1..3b46405dd 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
 #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
 	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
 
+struct ethtool_keee {
+	struct ethtool_eee eee;
+	struct {
+		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
+		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
+	} link_modes;
+	bool use_link_modes;
+};
+
+static inline struct ethtool_keee *ethtool_eee2keee(struct ethtool_eee *eee)
+{
+	if (!eee->is_member_of_keee)
+		return NULL;
+
+	return container_of(eee, struct ethtool_keee, eee);
+}
+
 /* drivers must ignore base.cmd and base.link_mode_masks_nwords
  * fields, but they are allowed to overwrite them (will be ignored).
  */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 0787d561a..ffc5ab130 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -365,6 +365,7 @@ struct ethtool_eeprom {
  * @tx_lpi_timer: Time in microseconds the interface delays prior to asserting
  *	its tx lpi (after reaching 'idle' state). Effective only when eee
  *	was negotiated and tx_lpi_enabled was set.
+ * @is_member_of_keee: struct is member of a struct ethtool_keee
  * @reserved: Reserved for future use; see the note on reserved space.
  */
 struct ethtool_eee {
@@ -376,7 +377,8 @@ struct ethtool_eee {
 	__u32	eee_enabled;
 	__u32	tx_lpi_enabled;
 	__u32	tx_lpi_timer;
-	__u32	reserved[2];
+	__u8    is_member_of_keee;
+	__u8	reserved[7];
 };
 
 /**
-- 
2.43.0

[PATCH net-next 2/5] ethtool: add basic handling of struct ethtool_keee

[Heiner Kallweit]

This is in preparation of follow-up functional changes, and it adds
basic handling of struct ethtool_keee. No functional change intended.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 net/ethtool/eee.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index 2853394d0..9b34d3310 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -13,7 +13,7 @@ struct eee_req_info {
 
 struct eee_reply_data {
 	struct ethnl_reply_data		base;
-	struct ethtool_eee		eee;
+	struct ethtool_keee		keee;
 };
 
 #define EEE_REPDATA(__reply_base) \
@@ -30,14 +30,17 @@ static int eee_prepare_data(const struct ethnl_req_info *req_base,
 {
 	struct eee_reply_data *data = EEE_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
+	struct ethtool_keee *keee = &data->keee;
+	struct ethtool_eee *eee = &keee->eee;
 	int ret;
 
+	eee->is_member_of_keee = 1;
 	if (!dev->ethtool_ops->get_eee)
 		return -EOPNOTSUPP;
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
-	ret = dev->ethtool_ops->get_eee(dev, &data->eee);
+	ret = dev->ethtool_ops->get_eee(dev, eee);
 	ethnl_ops_complete(dev);
 
 	return ret;
@@ -48,7 +51,8 @@ static int eee_reply_size(const struct ethnl_req_info *req_base,
 {
 	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
 	const struct eee_reply_data *data = EEE_REPDATA(reply_base);
-	const struct ethtool_eee *eee = &data->eee;
+	const struct ethtool_keee *keee = &data->keee;
+	const struct ethtool_eee *eee = &keee->eee;
 	int len = 0;
 	int ret;
 
@@ -84,7 +88,8 @@ static int eee_fill_reply(struct sk_buff *skb,
 {
 	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
 	const struct eee_reply_data *data = EEE_REPDATA(reply_base);
-	const struct ethtool_eee *eee = &data->eee;
+	const struct ethtool_keee *keee = &data->keee;
+	const struct ethtool_eee *eee = &keee->eee;
 	int ret;
 
 	ret = ethnl_put_bitset32(skb, ETHTOOL_A_EEE_MODES_OURS,
@@ -132,28 +137,30 @@ 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_eee eee = {};
+	struct ethtool_keee keee = {};
+	struct ethtool_eee *eee = &keee.eee;
 	bool mod = false;
 	int ret;
 
-	ret = dev->ethtool_ops->get_eee(dev, &eee);
+	eee->is_member_of_keee = 1;
+	ret = dev->ethtool_ops->get_eee(dev, eee);
 	if (ret < 0)
 		return ret;
 
-	ret = ethnl_update_bitset32(&eee.advertised, EEE_MODES_COUNT,
+	ret = ethnl_update_bitset32(&eee->advertised, EEE_MODES_COUNT,
 				    tb[ETHTOOL_A_EEE_MODES_OURS],
 				    link_mode_names, info->extack, &mod);
 	if (ret < 0)
 		return ret;
-	ethnl_update_bool32(&eee.eee_enabled, tb[ETHTOOL_A_EEE_ENABLED], &mod);
-	ethnl_update_bool32(&eee.tx_lpi_enabled,
+	ethnl_update_bool32(&eee->eee_enabled, tb[ETHTOOL_A_EEE_ENABLED], &mod);
+	ethnl_update_bool32(&eee->tx_lpi_enabled,
 			    tb[ETHTOOL_A_EEE_TX_LPI_ENABLED], &mod);
-	ethnl_update_u32(&eee.tx_lpi_timer, tb[ETHTOOL_A_EEE_TX_LPI_TIMER],
+	ethnl_update_u32(&eee->tx_lpi_timer, tb[ETHTOOL_A_EEE_TX_LPI_TIMER],
 			 &mod);
 	if (!mod)
 		return 0;
 
-	ret = dev->ethtool_ops->set_eee(dev, &eee);
+	ret = dev->ethtool_ops->set_eee(dev, eee);
 	return ret < 0 ? ret : 1;
 }
 
-- 
2.43.0

[PATCH net-next 3/5] ethtool: send EEE linkmode bitmaps to userspace

[Heiner Kallweit]

Fortunately, for sending EEE linkmode bitmaps to userspace, no changes
to the netlink attributes are needed. We just increase the size of the
sent bitmaps. For backward compatibility, if keee->use_link_modes is
false, copy the legacy bitmaps to keee->link_modes before sending
the linkmode bitmaps to userspace.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 net/ethtool/eee.c | 48 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index 9b34d3310..38b151146 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -43,6 +43,15 @@ static int eee_prepare_data(const struct ethnl_req_info *req_base,
 	ret = dev->ethtool_ops->get_eee(dev, eee);
 	ethnl_ops_complete(dev);
 
+	if (!ret && !keee->use_link_modes) {
+		ethtool_convert_legacy_u32_to_link_mode(keee->link_modes.supported,
+							eee->supported);
+		ethtool_convert_legacy_u32_to_link_mode(keee->link_modes.advertising,
+							eee->advertised);
+		ethtool_convert_legacy_u32_to_link_mode(keee->link_modes.lp_advertising,
+							eee->lp_advertised);
+	}
+
 	return ret;
 }
 
@@ -62,14 +71,17 @@ static int eee_reply_size(const struct ethnl_req_info *req_base,
 		     EEE_MODES_COUNT);
 
 	/* MODES_OURS */
-	ret = ethnl_bitset32_size(&eee->advertised, &eee->supported,
-				  EEE_MODES_COUNT, link_mode_names, compact);
+	ret = ethnl_bitset_size(keee->link_modes.advertising,
+				keee->link_modes.supported,
+				__ETHTOOL_LINK_MODE_MASK_NBITS,
+				link_mode_names, compact);
 	if (ret < 0)
 		return ret;
 	len += ret;
 	/* MODES_PEERS */
-	ret = ethnl_bitset32_size(&eee->lp_advertised, NULL,
-				  EEE_MODES_COUNT, link_mode_names, compact);
+	ret = ethnl_bitset_size(keee->link_modes.lp_advertising, NULL,
+				__ETHTOOL_LINK_MODE_MASK_NBITS,
+				link_mode_names, compact);
 	if (ret < 0)
 		return ret;
 	len += ret;
@@ -92,14 +104,17 @@ static int eee_fill_reply(struct sk_buff *skb,
 	const struct ethtool_eee *eee = &keee->eee;
 	int ret;
 
-	ret = ethnl_put_bitset32(skb, ETHTOOL_A_EEE_MODES_OURS,
-				 &eee->advertised, &eee->supported,
-				 EEE_MODES_COUNT, link_mode_names, compact);
+	ret = ethnl_put_bitset(skb, ETHTOOL_A_EEE_MODES_OURS,
+			       keee->link_modes.advertising,
+			       keee->link_modes.supported,
+			       __ETHTOOL_LINK_MODE_MASK_NBITS,
+			       link_mode_names, compact);
 	if (ret < 0)
 		return ret;
-	ret = ethnl_put_bitset32(skb, ETHTOOL_A_EEE_MODES_PEER,
-				 &eee->lp_advertised, NULL, EEE_MODES_COUNT,
-				 link_mode_names, compact);
+	ret = ethnl_put_bitset(skb, ETHTOOL_A_EEE_MODES_PEER,
+			       keee->link_modes.lp_advertising, NULL,
+			       __ETHTOOL_LINK_MODE_MASK_NBITS,
+			       link_mode_names, compact);
 	if (ret < 0)
 		return ret;
 
@@ -147,9 +162,16 @@ ethnl_set_eee(struct ethnl_req_info *req_info, struct genl_info *info)
 	if (ret < 0)
 		return ret;
 
-	ret = ethnl_update_bitset32(&eee->advertised, EEE_MODES_COUNT,
-				    tb[ETHTOOL_A_EEE_MODES_OURS],
-				    link_mode_names, info->extack, &mod);
+	if (keee.use_link_modes) {
+		ret = ethnl_update_bitset(keee.link_modes.advertising,
+					  __ETHTOOL_LINK_MODE_MASK_NBITS,
+					  tb[ETHTOOL_A_EEE_MODES_OURS],
+					  link_mode_names, info->extack, &mod);
+	} else {
+		ret = ethnl_update_bitset32(&eee->advertised, EEE_MODES_COUNT,
+					    tb[ETHTOOL_A_EEE_MODES_OURS],
+					    link_mode_names, info->extack, &mod);
+	}
 	if (ret < 0)
 		return ret;
 	ethnl_update_bool32(&eee->eee_enabled, tb[ETHTOOL_A_EEE_ENABLED], &mod);
-- 
2.43.0

[PATCH net-next 4/5] net: phy: c45: prepare genphy_c45_ethtool_set_eee for follow-up extension

[Heiner Kallweit]

This prepares genphy_c45_ethtool_set_eee() for functional changes in a
follow-up patch. No functional change intended.

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

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 747d14bf1..9d8b2b5eb 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1492,32 +1492,31 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 			       struct ethtool_eee *data)
 {
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp);
+	bool unsupported;
 	int ret;
 
-	if (data->eee_enabled) {
-		if (data->advertised) {
-			__ETHTOOL_DECLARE_LINK_MODE_MASK(adv);
-
-			ethtool_convert_legacy_u32_to_link_mode(adv,
-								data->advertised);
-			linkmode_andnot(adv, adv, phydev->supported_eee);
-			if (!linkmode_empty(adv)) {
-				phydev_warn(phydev, "At least some EEE link modes are not supported.\n");
-				return -EINVAL;
-			}
-
-			ethtool_convert_legacy_u32_to_link_mode(phydev->advertising_eee,
-								data->advertised);
-		} else {
-			linkmode_copy(phydev->advertising_eee,
-				      phydev->supported_eee);
-		}
+	phydev->eee_enabled = data->eee_enabled;
+	if (!data->eee_enabled)
+		goto eee_aneg;
 
-		phydev->eee_enabled = true;
-	} else {
-		phydev->eee_enabled = false;
+	ethtool_convert_legacy_u32_to_link_mode(adv, data->advertised);
+
+	if (linkmode_empty(adv)) {
+		linkmode_copy(phydev->advertising_eee, phydev->supported_eee);
+		goto eee_aneg;
 	}
 
+	unsupported = linkmode_andnot(tmp, adv, phydev->supported_eee);
+	if (unsupported) {
+		phydev_warn(phydev, "At least some EEE link modes are not supported.\n");
+		return -EINVAL;
+	}
+
+	linkmode_copy(phydev->advertising_eee, adv);
+
+eee_aneg:
 	ret = genphy_c45_an_config_eee_aneg(phydev);
 	if (ret < 0)
 		return ret;
-- 
2.43.0

[PATCH net-next 5/5] net: phy: c45: extend genphy_c45_ethtool_[set|get]_eee

[Heiner Kallweit]

Extend both functions to use the linkmode bitmap extension if available.

Note: The linkmode extension for now is supported only if ethtool uses
netlink. It's not supported for ioctl.

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

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 9d8b2b5eb..e276dba19 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1454,6 +1454,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp) = {};
 	bool overflow = false, is_enabled;
+	struct ethtool_keee *keee;
 	int ret;
 
 	ret = genphy_c45_eee_is_active(phydev, adv, lp, &is_enabled);
@@ -1463,6 +1464,16 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 	data->eee_enabled = is_enabled;
 	data->eee_active = ret;
 
+	keee = ethtool_eee2keee(data);
+	if (keee) {
+		linkmode_copy(keee->link_modes.supported,
+			      phydev->supported_eee);
+		linkmode_copy(keee->link_modes.advertising, adv);
+		linkmode_copy(keee->link_modes.lp_advertising, lp);
+		keee->use_link_modes = 1;
+		return 0;
+	}
+
 	if (!ethtool_convert_link_mode_to_legacy_u32(&data->supported,
 						     phydev->supported_eee))
 		overflow = true;
@@ -1494,6 +1505,7 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp);
+	struct ethtool_keee *keee;
 	bool unsupported;
 	int ret;
 
@@ -1501,7 +1513,11 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 	if (!data->eee_enabled)
 		goto eee_aneg;
 
-	ethtool_convert_legacy_u32_to_link_mode(adv, data->advertised);
+	keee = ethtool_eee2keee(data);
+	if (keee && keee->use_link_modes)
+		linkmode_copy(adv, keee->link_modes.advertising);
+	else
+		ethtool_convert_legacy_u32_to_link_mode(adv, data->advertised);
 
 	if (linkmode_empty(adv)) {
 		linkmode_copy(phydev->advertising_eee, phydev->supported_eee);
-- 
2.43.0

Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Marek Behún]

On Mon, 1 Jan 2024 22:23:15 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> + * @is_member_of_keee: struct is member of a struct ethtool_keee

I don't like how the name of a field in a UAPI structure refers to
kernel internals.

Can we rename it somehow?

Marek

Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Heiner Kallweit]

On 04.01.2024 17:27, Marek Behún wrote:
> On Mon, 1 Jan 2024 22:23:15 +0100
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>> + * @is_member_of_keee: struct is member of a struct ethtool_keee
> 
> I don't like how the name of a field in a UAPI structure refers to
> kernel internals.
> 
Actually this new member of struct ethtool_eee is irrelevant to
userspace. It just has to be member of struct ethtool_eee because
that's what we pass to the kernel EEE ethtool callbacks.
OK, in theory we could also pass the new flag as a new member of
struct net_device, but this would be hacky IMO.

> Can we rename it somehow?
> 
I'm open for suggestions.

> Marek

Heiner

Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Andrew Lunn]

On Mon, Jan 01, 2024 at 10:23:15PM +0100, Heiner Kallweit wrote:
> In order to pass EEE link modes beyond bit 32 to userspace we have to
> complement the 32 bit bitmaps in struct ethtool_eee with linkmode
> bitmaps. Therefore, similar to ethtool_link_settings and
> ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of
> the reserved fields in struct ethtool_eee as flag that an instance
> of struct ethtool_eee is embedded in a struct ethtool_keee, thus the
> linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/linux/ethtool.h      | 18 ++++++++++++++++++
>  include/uapi/linux/ethtool.h |  4 +++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index cfcd952a1..3b46405dd 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
>  
> +struct ethtool_keee {
> +	struct ethtool_eee eee;
> +	struct {
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
> +	} link_modes;
> +	bool use_link_modes;
> +};

I know its a lot more work, but its not how i would do it.

1) Add struct ethtool_keee which is a straight copy of ethtool_eee.

2) Then modify every in kernel MAC driver using ethtool_eee to
actually take ethtool_keee. Since its identical, its just a function
prototype change.

3) Then i would add some helpers to get and set eee bits. The initial
version would be limited to 32 bits, and expect to be passed a pointer
to a u32. Them modify all the MAC drivers which manipulate the
supported, advertising and lp_advertising to use these helpers.

4) Lastly, flip supported, advertising and lp_advertising to
ETHTOOL_DECLARE_LINK_MODE_MASK, modify the helpers, and fixup the
IOCTL API to convert to legacy u32 etc.

The first 2 steps are a patch each. Step 3 is a lot of patches, one
per MAC driver, but the changes should be simple and easy to
review. And then 4 is probably a single patch.

Doing it like this, we have a clean internal API.

      Andrew

Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Heiner Kallweit]

On 04.01.2024 18:16, Andrew Lunn wrote:
> On Mon, Jan 01, 2024 at 10:23:15PM +0100, Heiner Kallweit wrote:
>> In order to pass EEE link modes beyond bit 32 to userspace we have to
>> complement the 32 bit bitmaps in struct ethtool_eee with linkmode
>> bitmaps. Therefore, similar to ethtool_link_settings and
>> ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of
>> the reserved fields in struct ethtool_eee as flag that an instance
>> of struct ethtool_eee is embedded in a struct ethtool_keee, thus the
>> linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  include/linux/ethtool.h      | 18 ++++++++++++++++++
>>  include/uapi/linux/ethtool.h |  4 +++-
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index cfcd952a1..3b46405dd 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
>>  
>> +struct ethtool_keee {
>> +	struct ethtool_eee eee;
>> +	struct {
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
>> +	} link_modes;
>> +	bool use_link_modes;
>> +};
> 
> I know its a lot more work, but its not how i would do it.
> 
> 1) Add struct ethtool_keee which is a straight copy of ethtool_eee.
> 
I think I would make some (compatible) changes.
Member cmd isn't needed, and the type of the boolean values that is
__u32 currently I'd change to bool.

> 2) Then modify every in kernel MAC driver using ethtool_eee to
> actually take ethtool_keee. Since its identical, its just a function
> prototype change.
> 
> 3) Then i would add some helpers to get and set eee bits. The initial
> version would be limited to 32 bits, and expect to be passed a pointer
> to a u32. Them modify all the MAC drivers which manipulate the
> supported, advertising and lp_advertising to use these helpers.
> 
> 4) Lastly, flip supported, advertising and lp_advertising to
> ETHTOOL_DECLARE_LINK_MODE_MASK, modify the helpers, and fixup the
> IOCTL API to convert to legacy u32 etc.
> 
> The first 2 steps are a patch each. Step 3 is a lot of patches, one
> per MAC driver, but the changes should be simple and easy to
> review. And then 4 is probably a single patch.
> 
I hope we get away with step 2 being a single patch, as it is a pure
mechanical change. Typically a series is needed with one patch per
module, then having to wait for the ACK from the respective maintainers.
Can we get an OK upfront for the single patch approach?

> Doing it like this, we have a clean internal API.
> 
This indeed looks like the cleanest solution approach to me.
One benefit would be that we can almost completely get rid of the
strict data structure based coupling between userspace and kernel.
However this applies only if we keep the ioctl interface out of scope.

Is it consensus that the ioctl interface is considered legacy and
extended functionality may be implemented for the netlink interface only?
An upfront maintainer OK would be helpful.

I don't like the link settings ioctl handshake too much, which is needed
to communicate the number of bits in a linkmode bitmap to userspace.
Presumably we had to reuse this approach for EEE linkmode bitmaps if
we want to support linkmode bitmaps over ioctl.

>       Andrew
> 
> 
Heiner

Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Heiner Kallweit]

On 04.01.2024 18:16, Andrew Lunn wrote:
> On Mon, Jan 01, 2024 at 10:23:15PM +0100, Heiner Kallweit wrote:
>> In order to pass EEE link modes beyond bit 32 to userspace we have to
>> complement the 32 bit bitmaps in struct ethtool_eee with linkmode
>> bitmaps. Therefore, similar to ethtool_link_settings and
>> ethtool_link_kesettings, add a struct ethtool_keee. Use one byte of
>> the reserved fields in struct ethtool_eee as flag that an instance
>> of struct ethtool_eee is embedded in a struct ethtool_keee, thus the
>> linkmode bitmaps being accessible. Add ethtool_eee2keee() as accessor.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  include/linux/ethtool.h      | 18 ++++++++++++++++++
>>  include/uapi/linux/ethtool.h |  4 +++-
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index cfcd952a1..3b46405dd 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -163,6 +163,24 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
>>  
>> +struct ethtool_keee {
>> +	struct ethtool_eee eee;
>> +	struct {
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
>> +	} link_modes;
>> +	bool use_link_modes;
>> +};
> 
> I know its a lot more work, but its not how i would do it.
> 
> 1) Add struct ethtool_keee which is a straight copy of ethtool_eee.
> 
> 2) Then modify every in kernel MAC driver using ethtool_eee to
> actually take ethtool_keee. Since its identical, its just a function
> prototype change.
> 
> 3) Then i would add some helpers to get and set eee bits. The initial
> version would be limited to 32 bits, and expect to be passed a pointer
> to a u32. Them modify all the MAC drivers which manipulate the
> supported, advertising and lp_advertising to use these helpers.
> 
Looking at the ethtool EEE ops implementation of the relevant 16 drivers i see
quite some fixing/refactoring need. Just look at igc_ethtool_get_eee():

        edata->supported = SUPPORTED_Autoneg;
        edata->advertised = SUPPORTED_Autoneg;
        edata->lp_advertised = SUPPORTED_Autoneg;

This doesn't make sense at all, this function never worked and apparently
nobody ever noticed this. Maybe the author meant
edata->supported |= SUPPORTED_Autoneg, but even this wouldn't make sense
for an EEE mode bitmap.
I'd prefer to separate the needed refactoring/fixing from the EEE linkmode
bitmap extension, therefore omit step 3.

Steps 1 and 2 are good, they allow to decouple struct ethtool_keee from
ethtool_eee, so we can simplify struct ethtool_keee and reduce it to what's
needed on kernel side.

> 4) Lastly, flip supported, advertising and lp_advertising to
> ETHTOOL_DECLARE_LINK_MODE_MASK, modify the helpers, and fixup the
> IOCTL API to convert to legacy u32 etc.
> 
> The first 2 steps are a patch each. Step 3 is a lot of patches, one
> per MAC driver, but the changes should be simple and easy to
> review. And then 4 is probably a single patch.
> 
> Doing it like this, we have a clean internal API.
> 
>       Andrew
> 
> 

Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Andrew Lunn]

> Looking at the ethtool EEE ops implementation of the relevant 16 drivers i see
> quite some fixing/refactoring need. Just look at igc_ethtool_get_eee():
> 
>         edata->supported = SUPPORTED_Autoneg;
>         edata->advertised = SUPPORTED_Autoneg;
>         edata->lp_advertised = SUPPORTED_Autoneg;
> 
> This doesn't make sense at all, this function never worked and apparently
> nobody ever noticed this. Maybe the author meant
> edata->supported |= SUPPORTED_Autoneg, but even this wouldn't make sense
> for an EEE mode bitmap.

Yes, i noticed this as well. EEE is not too well defined, but this is
wrong. Since it never worked, just deleting this is fine, and leave it
to Intel engineers to set actual bitmaps.

> I'd prefer to separate the needed refactoring/fixing from the EEE linkmode
> bitmap extension, therefore omit step 3.
> 
> Steps 1 and 2 are good, they allow to decouple struct ethtool_keee from
> ethtool_eee, so we can simplify struct ethtool_keee and reduce it to what's
> needed on kernel side.

I would not do too much refactoring. I have a big patchset which
refactors most of the phylib driven drivers code for EEE, removing a
lot of it and pushing it into phylib. Its been sat in my repo a while
and i need to find the time/energy to post it and get it merged.

    Andrew

Re: [PATCH net-next 0/5] ethtool: add support for EEE linkmodes beyond bit 32

[Heiner Kallweit]

On 01.01.2024 22:22, Heiner Kallweit wrote:
> So far only 32bit legacy bitmaps are passed to userspace. This makes
> it impossible to manage EEE linkmodes beyond bit 32, e.g. manage EEE
> for 2500BaseT and 5000BaseT. This series adds support for passing
> full linkmode bitmaps between kernel and userspace.
> 
> Fortunately the netlink-based part of ethtool is quite smart and no
> changes are needed in ethtool. However this applies to the netlink
> interface only, the ioctl interface for now remains restricted to
> legacy bitmaps.
> 
> Next step will be adding support for the c45 EEE2 standard registers
> (3.21, 7.62, 7.63) to the genphy_c45 functions dealing with EEE.
> I have a follow-up series for this ready to be submitted.
> 
> Heiner Kallweit (5):
>   ethtool: add struct ethtool_keee and extend struct ethtool_eee
>   ethtool: add basic handling of struct ethtool_keee
>   ethtool: send EEE linkmode bitmaps to userspace
>   net: phy: c45: prepare genphy_c45_ethtool_set_eee for follow-up
>     extension
>   net: phy: c45: extend genphy_c45_ethtool_[set|get]_eee
> 
>  drivers/net/phy/phy-c45.c    | 57 +++++++++++++++++----------
>  include/linux/ethtool.h      | 18 +++++++++
>  include/uapi/linux/ethtool.h |  4 +-
>  net/ethtool/eee.c            | 75 +++++++++++++++++++++++++-----------
>  4 files changed, 109 insertions(+), 45 deletions(-)
> 
This version of the series can be set to "changes requested".
I'll submit a follow-up RFC series as basis for further discussion.

Re: [PATCH net-next 1/5] ethtool: add struct ethtool_keee and extend struct ethtool_eee

[Russell King (Oracle)]

On Sat, Jan 06, 2024 at 12:15:18AM +0100, Andrew Lunn wrote:
> I would not do too much refactoring. I have a big patchset which
> refactors most of the phylib driven drivers code for EEE, removing a
> lot of it and pushing it into phylib. Its been sat in my repo a while
> and i need to find the time/energy to post it and get it merged.

Strangely, I have similar for phylink... I was thinking about sending it
out during the last cycle, but I guess next cycle will do.

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