[PATCH net-next] ethtool: untangle the linkmode and ethtool headers

[PATCH net-next] ethtool: untangle the linkmode and ethtool headers

[Jakub Kicinski]

Commit 26c5334d344d ("ethtool: Add forced speed to supported link
modes maps") added a dependency between ethtool.h and linkmode.h.
The dependency in the opposite direction already exists so the
new code was inserted in an awkward place.

The reason for ethtool.h to include linkmode.h, is that
ethtool_forced_speed_maps_init() is a static inline helper.
That's not really necessary.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: paul.greenwalt@intel.com
CC: hkallweit1@gmail.com
CC: linux@armlinux.org.uk
CC: vladimir.oltean@nxp.com
CC: gal@nvidia.com
---
 include/linux/ethtool.h  | 22 ++--------------------
 include/linux/linkmode.h | 29 ++++++++++++++---------------
 net/ethtool/common.c     | 21 +++++++++++++++++++++
 3 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 8e91e8b8a693..226a36ed5aa1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -13,7 +13,6 @@
 #ifndef _LINUX_ETHTOOL_H
 #define _LINUX_ETHTOOL_H
 
-#include <linux/linkmode.h>
 #include <linux/bitmap.h>
 #include <linux/compat.h>
 #include <linux/if_ether.h>
@@ -1070,23 +1069,6 @@ struct ethtool_forced_speed_map {
 	.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.
- */
-static inline 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;
-	}
-}
+void
+ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size);
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
index cd38f89553e6..7303b4bc2ce0 100644
--- a/include/linux/linkmode.h
+++ b/include/linux/linkmode.h
@@ -2,21 +2,6 @@
 #define __LINKMODE_H
 
 #include <linux/bitmap.h>
-
-static inline void linkmode_set_bit(int nr, volatile unsigned long *addr)
-{
-	__set_bit(nr, addr);
-}
-
-static inline void linkmode_set_bit_array(const int *array, int array_size,
-					  unsigned long *addr)
-{
-	int i;
-
-	for (i = 0; i < array_size; i++)
-		linkmode_set_bit(array[i], addr);
-}
-
 #include <linux/ethtool.h>
 #include <uapi/linux/ethtool.h>
 
@@ -53,6 +38,11 @@ static inline int linkmode_andnot(unsigned long *dst, const unsigned long *src1,
 	return bitmap_andnot(dst, src1, src2,  __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
+static inline void linkmode_set_bit(int nr, volatile unsigned long *addr)
+{
+	__set_bit(nr, addr);
+}
+
 static inline void linkmode_clear_bit(int nr, volatile unsigned long *addr)
 {
 	__clear_bit(nr, addr);
@@ -72,6 +62,15 @@ static inline int linkmode_test_bit(int nr, const volatile unsigned long *addr)
 	return test_bit(nr, addr);
 }
 
+static inline void linkmode_set_bit_array(const int *array, int array_size,
+					  unsigned long *addr)
+{
+	int i;
+
+	for (i = 0; i < array_size; i++)
+		linkmode_set_bit(array[i], addr);
+}
+
 static inline int linkmode_equal(const unsigned long *src1,
 				 const unsigned long *src2)
 {
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index f5598c5f50de..b4419fb6df6a 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -685,3 +685,24 @@ ethtool_params_from_link_mode(struct ethtool_link_ksettings *link_ksettings,
 	link_ksettings->base.duplex = link_info->duplex;
 }
 EXPORT_SYMBOL_GPL(ethtool_params_from_link_mode);
+
+/**
+ * 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)
+{
+	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_GPL(ethtool_forced_speed_maps_init);
-- 
2.41.0

Re: [PATCH net-next] ethtool: untangle the linkmode and ethtool headers

[Greenwalt, Paul]

On 10/19/2023 8:28 AM, Jakub Kicinski wrote:
> Commit 26c5334d344d ("ethtool: Add forced speed to supported link
> modes maps") added a dependency between ethtool.h and linkmode.h.
> The dependency in the opposite direction already exists so the
> new code was inserted in an awkward place.
> 
> The reason for ethtool.h to include linkmode.h, is that
> ethtool_forced_speed_maps_init() is a static inline helper.
> That's not really necessary.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: andrew@lunn.ch
> CC: paul.greenwalt@intel.com
> CC: hkallweit1@gmail.com
> CC: linux@armlinux.org.uk
> CC: vladimir.oltean@nxp.com
> CC: gal@nvidia.com
> ---

Thanks Jakub, this is better.

Reviewed-by: Paul Greenwalt <paul.greenwalt@intel.com>

>  include/linux/ethtool.h  | 22 ++--------------------
>  include/linux/linkmode.h | 29 ++++++++++++++---------------
>  net/ethtool/common.c     | 21 +++++++++++++++++++++
>  3 files changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 8e91e8b8a693..226a36ed5aa1 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -13,7 +13,6 @@
>  #ifndef _LINUX_ETHTOOL_H
>  #define _LINUX_ETHTOOL_H
>  
> -#include <linux/linkmode.h>
>  #include <linux/bitmap.h>
>  #include <linux/compat.h>
>  #include <linux/if_ether.h>
> @@ -1070,23 +1069,6 @@ struct ethtool_forced_speed_map {
>  	.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.
> - */
> -static inline 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;
> -	}
> -}
> +void
> +ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size);
>  #endif /* _LINUX_ETHTOOL_H */
> diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
> index cd38f89553e6..7303b4bc2ce0 100644
> --- a/include/linux/linkmode.h
> +++ b/include/linux/linkmode.h
> @@ -2,21 +2,6 @@
>  #define __LINKMODE_H
>  
>  #include <linux/bitmap.h>
> -
> -static inline void linkmode_set_bit(int nr, volatile unsigned long *addr)
> -{
> -	__set_bit(nr, addr);
> -}
> -
> -static inline void linkmode_set_bit_array(const int *array, int array_size,
> -					  unsigned long *addr)
> -{
> -	int i;
> -
> -	for (i = 0; i < array_size; i++)
> -		linkmode_set_bit(array[i], addr);
> -}
> -
>  #include <linux/ethtool.h>
>  #include <uapi/linux/ethtool.h>
>  
> @@ -53,6 +38,11 @@ static inline int linkmode_andnot(unsigned long *dst, const unsigned long *src1,
>  	return bitmap_andnot(dst, src1, src2,  __ETHTOOL_LINK_MODE_MASK_NBITS);
>  }
>  
> +static inline void linkmode_set_bit(int nr, volatile unsigned long *addr)
> +{
> +	__set_bit(nr, addr);
> +}
> +
>  static inline void linkmode_clear_bit(int nr, volatile unsigned long *addr)
>  {
>  	__clear_bit(nr, addr);
> @@ -72,6 +62,15 @@ static inline int linkmode_test_bit(int nr, const volatile unsigned long *addr)
>  	return test_bit(nr, addr);
>  }
>  
> +static inline void linkmode_set_bit_array(const int *array, int array_size,
> +					  unsigned long *addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < array_size; i++)
> +		linkmode_set_bit(array[i], addr);
> +}
> +
>  static inline int linkmode_equal(const unsigned long *src1,
>  				 const unsigned long *src2)
>  {
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index f5598c5f50de..b4419fb6df6a 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -685,3 +685,24 @@ ethtool_params_from_link_mode(struct ethtool_link_ksettings *link_ksettings,
>  	link_ksettings->base.duplex = link_info->duplex;
>  }
>  EXPORT_SYMBOL_GPL(ethtool_params_from_link_mode);
> +
> +/**
> + * 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)
> +{
> +	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_GPL(ethtool_forced_speed_maps_init);

Re: [PATCH net-next] ethtool: untangle the linkmode and ethtool headers

[Russell King (Oracle)]

On Thu, Oct 19, 2023 at 08:28:15AM -0700, Jakub Kicinski wrote:
> Commit 26c5334d344d ("ethtool: Add forced speed to supported link
> modes maps") added a dependency between ethtool.h and linkmode.h.
> The dependency in the opposite direction already exists so the
> new code was inserted in an awkward place.
> 
> The reason for ethtool.h to include linkmode.h, is that
> ethtool_forced_speed_maps_init() is a static inline helper.
> That's not really necessary.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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] ethtool: untangle the linkmode and ethtool headers

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 08:28:15AM -0700, Jakub Kicinski wrote:
> +EXPORT_SYMBOL_GPL(ethtool_forced_speed_maps_init);

Is there a rule for EXPORT_SYMBOL() vs EXPORT_SYMBOL_GPL()? My rule of
thumb was that symbols used by drivers should get EXPORT_SYMBOL() for
maximum compatibility with their respective licenses, while symbols
exported for other core kernel modules should get EXPORT_SYMBOL_GPL().

Re: [PATCH net-next] ethtool: untangle the linkmode and ethtool headers

[Russell King (Oracle)]

On Fri, Oct 20, 2023 at 12:24:29PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 19, 2023 at 08:28:15AM -0700, Jakub Kicinski wrote:
> > +EXPORT_SYMBOL_GPL(ethtool_forced_speed_maps_init);
> 
> Is there a rule for EXPORT_SYMBOL() vs EXPORT_SYMBOL_GPL()? My rule of
> thumb was that symbols used by drivers should get EXPORT_SYMBOL() for
> maximum compatibility with their respective licenses, while symbols
> exported for other core kernel modules should get EXPORT_SYMBOL_GPL().

Author preference also comes into it - whether the author wants to
permit their code to be used by non-GPL compliant modules or not.

-- 
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] ethtool: untangle the linkmode and ethtool headers

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 08:28:15AM -0700, Jakub Kicinski wrote:
> Commit 26c5334d344d ("ethtool: Add forced speed to supported link
> modes maps") added a dependency between ethtool.h and linkmode.h.
> The dependency in the opposite direction already exists so the
> new code was inserted in an awkward place.
> 
> The reason for ethtool.h to include linkmode.h, is that
> ethtool_forced_speed_maps_init() is a static inline helper.
> That's not really necessary.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next] ethtool: untangle the linkmode and ethtool headers

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 19 Oct 2023 08:28:15 -0700 you wrote:
> Commit 26c5334d344d ("ethtool: Add forced speed to supported link
> modes maps") added a dependency between ethtool.h and linkmode.h.
> The dependency in the opposite direction already exists so the
> new code was inserted in an awkward place.
> 
> The reason for ethtool.h to include linkmode.h, is that
> ethtool_forced_speed_maps_init() is a static inline helper.
> That's not really necessary.
> 
> [...]

Here is the summary with links:
  - [net-next] ethtool: untangle the linkmode and ethtool headers
    https://git.kernel.org/netdev/net-next/c/20c6e05bd33d

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

Re: [PATCH net-next] ethtool: untangle the linkmode and ethtool headers

[Jakub Kicinski]

On Fri, 20 Oct 2023 12:24:29 +0300 Vladimir Oltean wrote:
> On Thu, Oct 19, 2023 at 08:28:15AM -0700, Jakub Kicinski wrote:
> > +EXPORT_SYMBOL_GPL(ethtool_forced_speed_maps_init);  
> 
> Is there a rule for EXPORT_SYMBOL() vs EXPORT_SYMBOL_GPL()? My rule of
> thumb was that symbols used by drivers should get EXPORT_SYMBOL() for
> maximum compatibility with their respective licenses, while symbols
> exported for other core kernel modules should get EXPORT_SYMBOL_GPL().

I think that Russell is right that it's author's preference. I don't
have a strong one so I just copy what's in the file. Luck would have 
it that the closest was EXPORT_SYMBOL_GPL(ethtool_set_ethtool_phy_ops);

We should perhaps have clearer guidance. You say drivers but even what
we put under drivers/net/ vs net/ is not crystal clear. With some
protocols leaving in one place and others in the other. Pretty core
things like netconsole are under drivers/ and qdiscs which have a
non-GPL API (IIRC) are under net/.

We'd need to consult with people who have more exposure to proprietary
code to figure out good rules. For better or worse I'm not one of them
:(