On 2023-12-06 05:57, Alexander Lobakin wrote:
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
You are right, it is not needed or planned to be used in uAPI, I will
move this to include/linux/ethtool.h
Thanks.
