diff mbox series

[net-next,v4,1/2] ethtool: Add forced speed to supported link modes maps

Message ID 20231011131348.435353-2-pawel.chmielewski@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series ethtool: Add link mode maps for forced speeds | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3237 this patch: 3237
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang success Errors and warnings before: 1571 this patch: 1571
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3479 this patch: 3479
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 116 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pawel Chmielewski Oct. 11, 2023, 1:13 p.m. UTC 
From: Paul Greenwalt <paul.greenwalt@intel.com>

The need to map Ethtool forced speeds to Ethtool supported link modes is
common among drivers. To support this, add a common structure for forced
speed maps and a function to init them.  This is solution was originally
introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes
maps on module init") for qede driver.

ethtool_forced_speed_maps_init() should be called during driver init
with an array of struct ethtool_forced_speed_map to populate the mapping.

Definitions for maps themselves are left in the driver code, as the sets
of supported link modes may vary between the devices.

The qede driver was compile tested only.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Pawel Chmielewski <pawel.chmielewski@intel.com>
---
 .../net/ethernet/qlogic/qede/qede_ethtool.c   | 46 +++++--------------
 include/linux/ethtool.h                       | 27 +++++++++++
 net/ethtool/ioctl.c                           | 13 ++++++
 3 files changed, 52 insertions(+), 34 deletions(-)

Comments

Jiri Pirko Oct. 11, 2023, 3:12 p.m. UTC | #1 
Wed, Oct 11, 2023 at 03:13:47PM CEST, pawel.chmielewski@intel.com wrote:
>From: Paul Greenwalt <paul.greenwalt@intel.com>
>
>The need to map Ethtool forced speeds to Ethtool supported link modes is
>common among drivers. To support this, add a common structure for forced
>speed maps and a function to init them.  This is solution was originally
>introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes
>maps on module init") for qede driver.
>
>ethtool_forced_speed_maps_init() should be called during driver init
>with an array of struct ethtool_forced_speed_map to populate the mapping.
>
>Definitions for maps themselves are left in the driver code, as the sets
>of supported link modes may vary between the devices.
>
>The qede driver was compile tested only.
>
>Suggested-by: Andrew Lunn <andrew@lunn.ch>
>Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
>Signed-off-by: Pawel Chmielewski <pawel.chmielewski@intel.com>
>---
> .../net/ethernet/qlogic/qede/qede_ethtool.c   | 46 +++++--------------
> include/linux/ethtool.h                       | 27 +++++++++++
> net/ethtool/ioctl.c                           | 13 ++++++

Would be probably better to this this in 2 patches. 1) Introduce ethtool
infra, 2) convert qede to use it


> 3 files changed, 52 insertions(+), 34 deletions(-)
>
>diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
>index 95820cf1cd6c..d4f2cd13d308 100644
>--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
>+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
>@@ -201,21 +201,6 @@ static const char qede_tests_str_arr[QEDE_ETHTOOL_TEST_MAX][ETH_GSTRING_LEN] = {
> 
> /* Forced speed capabilities maps */
> 
>-struct qede_forced_speed_map {
>-	u32		speed;
>-	__ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
>-
>-	const u32	*cap_arr;
>-	u32		arr_size;
>-};
>-
>-#define QEDE_FORCED_SPEED_MAP(value)					\
>-{									\
>-	.speed		= SPEED_##value,				\
>-	.cap_arr	= qede_forced_speed_##value,			\
>-	.arr_size	= ARRAY_SIZE(qede_forced_speed_##value),	\
>-}
>-
> static const u32 qede_forced_speed_1000[] __initconst = {
> 	ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> 	ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
>@@ -263,28 +248,21 @@ static const u32 qede_forced_speed_100000[] __initconst = {
> 	ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT,
> };
> 
>-static struct qede_forced_speed_map qede_forced_speed_maps[] __ro_after_init = {
>-	QEDE_FORCED_SPEED_MAP(1000),
>-	QEDE_FORCED_SPEED_MAP(10000),
>-	QEDE_FORCED_SPEED_MAP(20000),
>-	QEDE_FORCED_SPEED_MAP(25000),
>-	QEDE_FORCED_SPEED_MAP(40000),
>-	QEDE_FORCED_SPEED_MAP(50000),
>-	QEDE_FORCED_SPEED_MAP(100000),
>+static struct ethtool_forced_speed_map
>+	qede_forced_speed_maps[] __ro_after_init = {
>+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 1000),

This is confusing indentation. What about:
static struct ethtool_forced_speed_map
qede_forced_speed_maps[] __ro_after_init = {
	ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 1000),
?


>+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 10000),
>+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 20000),
>+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 25000),
>+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 40000),
>+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 50000),
>+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 100000),
> };
> 
> void __init qede_forced_speed_maps_init(void)
> {
>-	struct qede_forced_speed_map *map;
>-	u32 i;
>-
>-	for (i = 0; i < ARRAY_SIZE(qede_forced_speed_maps); i++) {
>-		map = qede_forced_speed_maps + i;
>-
>-		linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
>-		map->cap_arr = NULL;
>-		map->arr_size = 0;
>-	}
>+	ethtool_forced_speed_maps_init(qede_forced_speed_maps,
>+				       ARRAY_SIZE(qede_forced_speed_maps));
> }
> 
> /* Ethtool callbacks */
>@@ -564,8 +542,8 @@ static int qede_set_link_ksettings(struct net_device *dev,
> 				   const struct ethtool_link_ksettings *cmd)
> {
> 	const struct ethtool_link_settings *base = &cmd->base;
>+	const struct ethtool_forced_speed_map *map;
> 	struct qede_dev *edev = netdev_priv(dev);
>-	const struct qede_forced_speed_map *map;
> 	struct qed_link_output current_link;
> 	struct qed_link_params params;
> 	u32 i;
>diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>index 62b61527bcc4..68d682012e9d 100644
>--- a/include/linux/ethtool.h
>+++ b/include/linux/ethtool.h
>@@ -1052,4 +1052,31 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
>  * next string.
>  */
> extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
>+
>+/* Link mode to forced speed capabilities maps */
>+struct ethtool_forced_speed_map {
>+	u32		speed;
>+	__ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
>+
>+	const u32	*cap_arr;
>+	u32		arr_size;
>+};
>+
>+#define ETHTOOL_FORCED_SPEED_MAP(prefix, value)				\
>+{									\
>+	.speed		= SPEED_##value,				\
>+	.cap_arr	= prefix##_##value,				\
>+	.arr_size	= ARRAY_SIZE(prefix##_##value),			\
>+}
>+
>+/**
>+ * ethtool_forced_speed_maps_init
>+ * @maps: Pointer to an array of Ethtool forced speed map
>+ * @size: Array size
>+ *
>+ * Initialize an array of Ethtool forced speed map to Ethtool link modes. This
>+ * should be called during driver module init.
>+ */
>+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
>+				    u32 size);
> #endif /* _LINUX_ETHTOOL_H */
>diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c

Why you put this into ioctl.c?

Can't this be put into include/linux/linkmode.h as a static helper as
well?


>index 0b0ce4f81c01..34507691fc9d 100644
>--- a/net/ethtool/ioctl.c
>+++ b/net/ethtool/ioctl.c
>@@ -3388,3 +3388,16 @@ void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *flow)
> 	kfree(flow);
> }
> EXPORT_SYMBOL(ethtool_rx_flow_rule_destroy);
>+
>+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
>+				    u32 size)
>+{
>+	for (u32 i = 0; i < size; i++) {
>+		struct ethtool_forced_speed_map *map = &maps[i];
>+
>+		linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
>+		map->cap_arr = NULL;
>+		map->arr_size = 0;
>+	}
>+}
>+EXPORT_SYMBOL(ethtool_forced_speed_maps_init);
>-- 
>2.37.3
>
>
Pawel Chmielewski Oct. 12, 2023, 1:33 p.m. UTC | #2 
> >diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> 
> Why you put this into ioctl.c?
> 
> Can't this be put into include/linux/linkmode.h as a static helper as
> well?

I'm a little bit confused, include/linux/linkmode.h doesn't contain
similar ethtool helpers.. Did you maybe meant ethtool.h?
 
> 
> >index 0b0ce4f81c01..34507691fc9d 100644
> >--- a/net/ethtool/ioctl.c
> >+++ b/net/ethtool/ioctl.c
> >@@ -3388,3 +3388,16 @@ void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *flow)
> > 	kfree(flow);
> > }
> > EXPORT_SYMBOL(ethtool_rx_flow_rule_destroy);
> >+
> >+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
> >+				    u32 size)
> >+{
> >+	for (u32 i = 0; i < size; i++) {
> >+		struct ethtool_forced_speed_map *map = &maps[i];
> >+
> >+		linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
> >+		map->cap_arr = NULL;
> >+		map->arr_size = 0;
> >+	}
> >+}
> >+EXPORT_SYMBOL(ethtool_forced_speed_maps_init);
> >-- 
> >2.37.3
> >
> >
Jiri Pirko Oct. 12, 2023, 1:42 p.m. UTC | #3 
Thu, Oct 12, 2023 at 03:33:15PM CEST, pawel.chmielewski@intel.com wrote:
>> >diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>> 
>> Why you put this into ioctl.c?
>> 
>> Can't this be put into include/linux/linkmode.h as a static helper as
>> well?
>
>I'm a little bit confused, include/linux/linkmode.h doesn't contain
>similar ethtool helpers.. Did you maybe meant ethtool.h?

I just looked there linkmode_set_bit_array is. ethtool.h might be the
place.


> 
>> 
>> >index 0b0ce4f81c01..34507691fc9d 100644
>> >--- a/net/ethtool/ioctl.c
>> >+++ b/net/ethtool/ioctl.c
>> >@@ -3388,3 +3388,16 @@ void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *flow)
>> > 	kfree(flow);
>> > }
>> > EXPORT_SYMBOL(ethtool_rx_flow_rule_destroy);
>> >+
>> >+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
>> >+				    u32 size)
>> >+{
>> >+	for (u32 i = 0; i < size; i++) {
>> >+		struct ethtool_forced_speed_map *map = &maps[i];
>> >+
>> >+		linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
>> >+		map->cap_arr = NULL;
>> >+		map->arr_size = 0;
>> >+	}
>> >+}
>> >+EXPORT_SYMBOL(ethtool_forced_speed_maps_init);
>> >-- 
>> >2.37.3
>> >
>> >
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index 95820cf1cd6c..d4f2cd13d308 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -201,21 +201,6 @@  static const char qede_tests_str_arr[QEDE_ETHTOOL_TEST_MAX][ETH_GSTRING_LEN] = {
 
 /* Forced speed capabilities maps */
 
-struct qede_forced_speed_map {
-	u32		speed;
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
-
-	const u32	*cap_arr;
-	u32		arr_size;
-};
-
-#define QEDE_FORCED_SPEED_MAP(value)					\
-{									\
-	.speed		= SPEED_##value,				\
-	.cap_arr	= qede_forced_speed_##value,			\
-	.arr_size	= ARRAY_SIZE(qede_forced_speed_##value),	\
-}
-
 static const u32 qede_forced_speed_1000[] __initconst = {
 	ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
 	ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
@@ -263,28 +248,21 @@  static const u32 qede_forced_speed_100000[] __initconst = {
 	ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT,
 };
 
-static struct qede_forced_speed_map qede_forced_speed_maps[] __ro_after_init = {
-	QEDE_FORCED_SPEED_MAP(1000),
-	QEDE_FORCED_SPEED_MAP(10000),
-	QEDE_FORCED_SPEED_MAP(20000),
-	QEDE_FORCED_SPEED_MAP(25000),
-	QEDE_FORCED_SPEED_MAP(40000),
-	QEDE_FORCED_SPEED_MAP(50000),
-	QEDE_FORCED_SPEED_MAP(100000),
+static struct ethtool_forced_speed_map
+	qede_forced_speed_maps[] __ro_after_init = {
+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 1000),
+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 10000),
+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 20000),
+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 25000),
+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 40000),
+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 50000),
+		ETHTOOL_FORCED_SPEED_MAP(qede_forced_speed, 100000),
 };
 
 void __init qede_forced_speed_maps_init(void)
 {
-	struct qede_forced_speed_map *map;
-	u32 i;
-
-	for (i = 0; i < ARRAY_SIZE(qede_forced_speed_maps); i++) {
-		map = qede_forced_speed_maps + i;
-
-		linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
-		map->cap_arr = NULL;
-		map->arr_size = 0;
-	}
+	ethtool_forced_speed_maps_init(qede_forced_speed_maps,
+				       ARRAY_SIZE(qede_forced_speed_maps));
 }
 
 /* Ethtool callbacks */
@@ -564,8 +542,8 @@  static int qede_set_link_ksettings(struct net_device *dev,
 				   const struct ethtool_link_ksettings *cmd)
 {
 	const struct ethtool_link_settings *base = &cmd->base;
+	const struct ethtool_forced_speed_map *map;
 	struct qede_dev *edev = netdev_priv(dev);
-	const struct qede_forced_speed_map *map;
 	struct qed_link_output current_link;
 	struct qed_link_params params;
 	u32 i;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 62b61527bcc4..68d682012e9d 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1052,4 +1052,31 @@  static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add,
  * next string.
  */
 extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
+
+/* Link mode to forced speed capabilities maps */
+struct ethtool_forced_speed_map {
+	u32		speed;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
+
+	const u32	*cap_arr;
+	u32		arr_size;
+};
+
+#define ETHTOOL_FORCED_SPEED_MAP(prefix, value)				\
+{									\
+	.speed		= SPEED_##value,				\
+	.cap_arr	= prefix##_##value,				\
+	.arr_size	= ARRAY_SIZE(prefix##_##value),			\
+}
+
+/**
+ * ethtool_forced_speed_maps_init
+ * @maps: Pointer to an array of Ethtool forced speed map
+ * @size: Array size
+ *
+ * Initialize an array of Ethtool forced speed map to Ethtool link modes. This
+ * should be called during driver module init.
+ */
+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
+				    u32 size);
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0b0ce4f81c01..34507691fc9d 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -3388,3 +3388,16 @@  void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *flow)
 	kfree(flow);
 }
 EXPORT_SYMBOL(ethtool_rx_flow_rule_destroy);
+
+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
+				    u32 size)
+{
+	for (u32 i = 0; i < size; i++) {
+		struct ethtool_forced_speed_map *map = &maps[i];
+
+		linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
+		map->cap_arr = NULL;
+		map->arr_size = 0;
+	}
+}
+EXPORT_SYMBOL(ethtool_forced_speed_maps_init);