From: Ahmed Zaki <ahmed.zaki@xxxxxxxxx>
Date: Tue, 5 Dec 2023 16:00:42 -0700
Duplicating my comment from the internal review:
> The get/set_rxfh ethtool ops currently takes the rxfh (RSS) parameters
> as direct function arguments. This will force us to change the API (and
> all drivers' functions) every time some new parameters are added.
[...]
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f7fba0dc87e5..f6e229e465b1 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1229,6 +1229,27 @@ struct ethtool_rxnfc {
> __u32 rule_locs[];
> };
>
> +/**
> + * struct ethtool_rxfh_param - RXFH (RSS) parameters
> + * @hfunc: Defines the current RSS hash function used by HW (or to be set to).
> + * Valid values are one of the %ETH_RSS_HASH_*.
> + * @indir_size: On SET, the array size of the user buffer for the
> + * indirection table, which may be zero, or
> + * %ETH_RXFH_INDIR_NO_CHANGE. On GET (read from the driver),
> + * the array size of the hardware indirection table.
> + * @indir: The indirection table of size @indir_size entries.
> + * @key_size: On SET, the array size of the user buffer for the hash key,
> + * which may be zero. On GET (read from the driver), the size of the
> + * hardware hash key.
> + * @key: The hash key of size @key_size bytes.
> + */
> +struct ethtool_rxfh_param {
> + __u8 hfunc;
> + __u32 indir_size;
> + __u32 *indir;
> + __u32 key_size;
> + __u8 *key;
> +};
1. Why is this structure needed in UAPI? Do you plan to use it somewhere
in userspace?
2. Kernel and userspace can't share pointers (as well as unsigned longs,
size_ts, and so on) as you may run a 32-bit application on a 64-bit
kernel.
3. Please never pass UAPI structures directly to the drivers, it's a bad
idea and you may end up converting all those drivers once again when
you'd need to to e.g. change the type of a field there. You won't be
able to change the type in a UAPI structure.
Thanks,
Olek
