diff mbox series

[v2,RFC,4/5] ethtool: add linkmode bitmap support to struct ethtool_keee

Message ID 87a063ed-1b06-40b4-9c12-37658f36ea06@gmail.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series ethtool: switch EEE netlink interface to use EEE linkmode bitmaps | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Heiner Kallweit Jan. 6, 2024, 10:21 p.m. UTC 
Add linkmode bitmap members to struct ethtool_keee, but keep the legacy
u32 bitmaps for compatibility with existing drivers.
Use link_modes.supported not being empty as indicator that a user wants
to use the linkmode bitmap members instead of the legacy bitmaps.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/ethtool.h |  5 ++++
 net/ethtool/common.c    |  5 ++++
 net/ethtool/common.h    |  1 +
 net/ethtool/eee.c       | 51 ++++++++++++++++++++++++++++++-----------
 net/ethtool/ioctl.c     | 28 +++++++++++++++++++---
 5 files changed, 73 insertions(+), 17 deletions(-)

Comments

Andrew Lunn Jan. 7, 2024, 5:14 p.m. UTC | #1 
On Sat, Jan 06, 2024 at 11:21:31PM +0100, Heiner Kallweit wrote:
> Add linkmode bitmap members to struct ethtool_keee, but keep the legacy
> u32 bitmaps for compatibility with existing drivers.
> Use link_modes.supported not being empty as indicator that a user wants
> to use the linkmode bitmap members instead of the legacy bitmaps.

So my fear is, the legacy code will never get cleaned up.

How many MAC drivers are there which don't use phylib/phylink?

Maybe i can help out converting them.

> +++ b/include/linux/ethtool.h
> @@ -223,6 +223,11 @@ __ethtool_get_link_ksettings(struct net_device *dev,
>  			     struct ethtool_link_ksettings *link_ksettings);
>  
>  struct ethtool_keee {
> +	struct {
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
> +	} link_modes;
>  	u32	supported;
>  	u32	advertised;
>  	u32	lp_advertised;

I don't particularly like having the link_modes struct here. The end
goal should be that supported, advertised and lp_advertised become
link modes, and all the drivers are changed to use them.

Maybe we have one patch which does another global replace,
supported->supported_u32, advertised->advertised_32, etc, making space
for link mode symbols. phylib can directly use the new link modes so
there is no real change in that code. We can then convert the MAC
drivers not using phylib one by one, and then remove the _u32 members
at the end.

      Andrew
Heiner Kallweit Jan. 7, 2024, 5:50 p.m. UTC | #2 
On 07.01.2024 18:14, Andrew Lunn wrote:
> On Sat, Jan 06, 2024 at 11:21:31PM +0100, Heiner Kallweit wrote:
>> Add linkmode bitmap members to struct ethtool_keee, but keep the legacy
>> u32 bitmaps for compatibility with existing drivers.
>> Use link_modes.supported not being empty as indicator that a user wants
>> to use the linkmode bitmap members instead of the legacy bitmaps.
> 
> So my fear is, the legacy code will never get cleaned up.
> 
> How many MAC drivers are there which don't use phylib/phylink?
> 
A grep for lp_advertised gives the following, excluding lan78xx and lan743x,
for which I just submitted patches:
https://patchwork.kernel.org/project/netdevbpf/patch/3340ff84-8d7a-404b-8268-732c7f281164@gmail.com/
https://patchwork.kernel.org/project/netdevbpf/patch/b086b296-0a1b-42d4-8e2b-ef6682598185@gmail.com/

drivers/net/usb/r8152.c
drivers/net/usb/ax88179_178a.c
drivers/net/ethernet/qlogic/qede/qede_ethtool.c
drivers/net/ethernet/broadcom/tg3.c
drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
drivers/net/ethernet/broadcom/bnxt/bnxt.c
drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
drivers/net/ethernet/intel/igb/igb_ethtool.c
drivers/net/ethernet/intel/i40e/i40e_ethtool.c
drivers/net/ethernet/intel/igc/igc_ethtool.c
  This one is completely broken, we just talked about it.
drivers/net/ethernet/intel/e1000e/ethtool.c
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

> Maybe i can help out converting them.
> 
>> +++ b/include/linux/ethtool.h
>> @@ -223,6 +223,11 @@ __ethtool_get_link_ksettings(struct net_device *dev,
>>  			     struct ethtool_link_ksettings *link_ksettings);
>>  
>>  struct ethtool_keee {
>> +	struct {
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>> +		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
>> +	} link_modes;
>>  	u32	supported;
>>  	u32	advertised;
>>  	u32	lp_advertised;
> 
> I don't particularly like having the link_modes struct here. The end
> goal should be that supported, advertised and lp_advertised become
> link modes, and all the drivers are changed to use them.
> 
> Maybe we have one patch which does another global replace,
> supported->supported_u32, advertised->advertised_32, etc, making space
> for link mode symbols. phylib can directly use the new link modes so
> there is no real change in that code. We can then convert the MAC
> drivers not using phylib one by one, and then remove the _u32 members
> at the end.
> 
Agreed. I'll add that.

>       Andrew

Heiner
diff mbox series

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 25f82d23b..f46242033 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -223,6 +223,11 @@  __ethtool_get_link_ksettings(struct net_device *dev,
 			     struct ethtool_link_ksettings *link_ksettings);
 
 struct ethtool_keee {
+	struct {
+		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
+		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
+	} link_modes;
 	u32	supported;
 	u32	advertised;
 	u32	lp_advertised;
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 6b2a360dc..872849646 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -712,3 +712,8 @@  ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size)
 	}
 }
 EXPORT_SYMBOL_GPL(ethtool_forced_speed_maps_init);
+
+bool ethtool_eee_use_linkmodes(const struct ethtool_keee *eee)
+{
+	return !linkmode_empty(eee->link_modes.supported);
+}
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 28b8aaaf9..0f2b5f7ea 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -55,5 +55,6 @@  int ethtool_get_module_eeprom_call(struct net_device *dev,
 				   struct ethtool_eeprom *ee, u8 *data);
 
 bool __ethtool_dev_mm_supported(struct net_device *dev);
+bool ethtool_eee_use_linkmodes(const struct ethtool_keee *eee);
 
 #endif /* _ETHTOOL_COMMON_H */
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index fa7ee51b5..b2de845ca 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -30,6 +30,7 @@  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 *eee = &data->eee;
 	int ret;
 
 	if (!dev->ethtool_ops->get_eee)
@@ -37,9 +38,18 @@  static int eee_prepare_data(const struct ethnl_req_info *req_base,
 	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);
 
+	if (!ret && !ethtool_eee_use_linkmodes(eee)) {
+		ethtool_convert_legacy_u32_to_link_mode(eee->link_modes.supported,
+							eee->supported);
+		ethtool_convert_legacy_u32_to_link_mode(eee->link_modes.advertising,
+							eee->advertised);
+		ethtool_convert_legacy_u32_to_link_mode(eee->link_modes.lp_advertising,
+							eee->lp_advertised);
+	}
+
 	return ret;
 }
 
@@ -58,14 +68,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(eee->link_modes.advertising,
+				eee->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(eee->link_modes.lp_advertising, NULL,
+				__ETHTOOL_LINK_MODE_MASK_NBITS,
+				link_mode_names, compact);
 	if (ret < 0)
 		return ret;
 	len += ret;
@@ -87,14 +100,17 @@  static int eee_fill_reply(struct sk_buff *skb,
 	const struct ethtool_keee *eee = &data->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,
+			       eee->link_modes.advertising,
+			       eee->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,
+			       eee->link_modes.lp_advertising, NULL,
+			       __ETHTOOL_LINK_MODE_MASK_NBITS,
+			       link_mode_names, compact);
 	if (ret < 0)
 		return ret;
 
@@ -140,9 +156,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 (ethtool_eee_use_linkmodes(&eee)) {
+		ret = ethnl_update_bitset(eee.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_bool(&eee.eee_enabled, tb[ETHTOOL_A_EEE_ENABLED], &mod);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 9222fbeeb..0df3f183b 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1517,6 +1517,13 @@  static void eee_to_keee(struct ethtool_keee *keee,
 	keee->eee_enabled = eee->eee_enabled;
 	keee->tx_lpi_enabled = eee->tx_lpi_enabled;
 	keee->tx_lpi_timer = eee->tx_lpi_timer;
+
+	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);
 }
 
 static void keee_to_eee(struct ethtool_eee *eee,
@@ -1524,13 +1531,28 @@  static void keee_to_eee(struct ethtool_eee *eee,
 {
 	memset(eee, 0, sizeof(*eee));
 
-	eee->supported = keee->supported;
-	eee->advertised = keee->advertised;
-	eee->lp_advertised = keee->lp_advertised;
 	eee->eee_active = keee->eee_active;
 	eee->eee_enabled = keee->eee_enabled;
 	eee->tx_lpi_enabled = keee->tx_lpi_enabled;
 	eee->tx_lpi_timer = keee->tx_lpi_timer;
+
+	if (ethtool_eee_use_linkmodes(keee)) {
+		bool overflow;
+
+		overflow = !ethtool_convert_link_mode_to_legacy_u32(
+				&eee->supported,
+				keee->link_modes.supported);
+		ethtool_convert_link_mode_to_legacy_u32(&eee->advertised,
+				keee->link_modes.advertising);
+		ethtool_convert_link_mode_to_legacy_u32(&eee->lp_advertised,
+				keee->link_modes.lp_advertising);
+		if (overflow)
+			pr_warn("Ethtool ioctl interface doesn't support passing EEE linkmodes beyond bit 32\n");
+	} else {
+		eee->supported = keee->supported;
+		eee->advertised = keee->advertised;
+		eee->lp_advertised = keee->lp_advertised;
+	}
 }
 
 static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)