Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Paul Greenwalt <paul.greenwalt@intel.com>
Date: Sat, 19 Aug 2023 02:39:41 -0700

> The need to map Ethtool forced speeds to  Ethtool supported link modes is
> common among drivers. To support this move the supported link modes maps
> implementation from the qede driver. This is an efficient solution
> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes
> maps on module init") for qede driver.

[...]

> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 62b61527bcc4..245fd4a8d85b 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -1052,4 +1052,78 @@ 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;

Please re-layout this as follows:

1. caps;
2. cap_arr;
3. arr_size;
4. speed.

Or in any other way that wouldn't provoke holes on 64-bit systems.
I wasn't really familiar with that when initially adding these
definitions :D

> +};
> +
> +#define ETHTOOL_FORCED_SPEED_MAP(value)					\
> +{									\
> +	.speed		= SPEED_##value,				\
> +	.cap_arr	= ethtool_forced_speed_##value,			\
> +	.arr_size	= ARRAY_SIZE(ethtool_forced_speed_##value),	\
> +}
> +
> +static const u32 ethtool_forced_speed_1000[] __initconst = {

2 reasons I don't like this:

1. static const in a header file.
   This implies code duplication -- every object file will have its own
   copy. If we want to reuse them, they should be declared global in one
   place (somewhere in net/ethtool), without __initconst unfortunately.
2. __initconst in a header file.
   I can refer to that declaration somewhere not in the initialization
   code and catch modpost or section reference issues. If we want to
   make those global and accessible from the drivers, it should not have
   any non-standard section placement.

> +	ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +	ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
> +	ETHTOOL_LINK_MODE_1000baseX_Full_BIT,

BTW, can't we share the target bitmaps instead? I mean, why not
initialize those somewhere in net/ethtool and export them, instead of
exporting the source of initialization?

> +};
> +
> +static const u32 ethtool_forced_speed_10000[] __initconst = {
> +	ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseR_FEC_BIT,
> +	ETHTOOL_LINK_MODE_10000baseCR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT,
> +};
> +
> +static const u32 ethtool_forced_speed_20000[] __initconst = {
> +	ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT,
> +};
> +
> +static const u32 ethtool_forced_speed_25000[] __initconst = {
> +	ETHTOOL_LINK_MODE_25000baseKR_Full_BIT,
> +	ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
> +	ETHTOOL_LINK_MODE_25000baseSR_Full_BIT,
> +};
> +
> +static const u32 ethtool_forced_speed_40000[] __initconst = {
> +	ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT,
> +	ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
> +	ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
> +	ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT,
> +};
> +
> +static const u32 ethtool_forced_speed_50000[] __initconst = {
> +	ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT,
> +	ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT,
> +	ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT,
> +};
> +
> +static const u32 ethtool_forced_speed_100000[] __initconst = {
> +	ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT,
> +	ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT,
> +	ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT,
> +	ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT,
> +};
> +
> +/**
> + * 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..ac1fdd636bc1 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -3388,3 +3388,18 @@ 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)

Alternatively, to avoid passing size here, you can just terminate @maps
array with zeroed element and treat it as the array end.

> +{
> +	u32 i;
> +
> +	for (i = 0; i < size; i++) {

	for (u32 i = 0 ...

> +		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;

These two are needed only if your @map really refer to
__initdata/__initconst variables.

> +	}
> +}
> +EXPORT_SYMBOL(ethtool_forced_speed_maps_init);

Not sure ioctl.c in the best place for that.

Thanks,
Olek