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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <412e63a7-278e-369d-1ed4-9d930770d9e0@intel.com>
Date: Thu, 19 Oct 2023 08:47:41 -0700
From: "Greenwalt, Paul" <paul.greenwalt@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
	<vladimir.oltean@....com>, <gal@...dia.com>
Subject: Re: [PATCH net-next] ethtool: untangle the linkmode and ethtool
 headers



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@...nel.org>
> ---
> CC: andrew@...n.ch
> CC: paul.greenwalt@...el.com
> CC: hkallweit1@...il.com
> CC: linux@...linux.org.uk
> CC: vladimir.oltean@....com
> CC: gal@...dia.com
> ---

Thanks Jakub, this is better.

Reviewed-by: Paul Greenwalt <paul.greenwalt@...el.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);