[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <63d43712-2bcd-4b1b-a4bc-f97d898e4d00@intel.com>
Date: Wed, 6 Dec 2023 06:03:20 -0700
From: Ahmed Zaki <ahmed.zaki@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: <netdev@...r.kernel.org>, <mkubecek@...e.cz>, <andrew@...n.ch>,
<willemdebruijn.kernel@...il.com>, <corbet@....net>,
<vladimir.oltean@....com>, <gal@...dia.com>, <linux-doc@...r.kernel.org>,
<jesse.brandeburg@...el.com>, <ecree.xilinx@...il.com>,
<edumazet@...gle.com>, <anthony.l.nguyen@...el.com>, <horms@...nel.org>,
<kuba@...nel.org>, Jacob Keller <jacob.e.keller@...el.com>,
<intel-wired-lan@...ts.osuosl.org>, <pabeni@...hat.com>,
<davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH net-next v7 1/8] net: ethtool: pass a
pointer to parameters to get/set_rxfh ethtool ops
On 2023-12-06 05:57, Alexander Lobakin wrote:
> From: Ahmed Zaki <ahmed.zaki@...el.com>
> 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.
Powered by blists - more mailing lists