Re: [PATCH net-next v4 06/24] net: clarify the meaning of netdev_config members

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/13/25 18:12, Randy Dunlap wrote:
> Hi,
> 
> On 10/13/25 7:54 AM, Pavel Begunkov wrote:
>> From: Jakub Kicinski <kuba@kernel.org>
>>
>> hds_thresh and hds_config are both inside struct netdev_config
>> but have quite different semantics. hds_config is the user config
>> with ternary semantics (on/off/unset). hds_thresh is a straight
>> up value, populated by the driver at init and only modified by
>> user space. We don't expect the drivers to have to pick a special
>> hds_thresh value based on other configuration.
>>
>> The two approaches have different advantages and downsides.
>> hds_thresh ("direct value") gives core easy access to current
>> device settings, but there's no way to express whether the value
>> comes from the user. It also requires the initialization by
>> the driver.
>>
>> hds_config ("user config values") tells us what user wanted, but
>> doesn't give us the current value in the core.
>>
>> Try to explain this a bit in the comments, so at we make a conscious
>> choice for new values which semantics we expect.
>>
>> Move the init inside ethtool_ringparam_get_cfg() to reflect the semantics.
>> Commit 216a61d33c07 ("net: ethtool: fix ethtool_ringparam_get_cfg()
>> returns a hds_thresh value always as 0.") added the setting for the
>> benefit of netdevsim which doesn't touch the value at all on get.
>> Again, this is just to clarify the intention, shouldn't cause any
>> functional change.
>>
>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> [pavel: applied clarification on relationship b/w HDS thresh and config]
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   include/net/netdev_queues.h | 20 ++++++++++++++++++--
>>   net/ethtool/common.c        |  3 ++-
>>   2 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
>> index cd00e0406cf4..9d5dde36c2e5 100644
>> --- a/include/net/netdev_queues.h
>> +++ b/include/net/netdev_queues.h
>> @@ -6,11 +6,27 @@
>>   
>>   /**
>>    * struct netdev_config - queue-related configuration for a netdev
>> - * @hds_thresh:		HDS Threshold value.
>> - * @hds_config:		HDS value from userspace.
>>    */
>>   struct netdev_config {
>> +	/* Direct value
>> +	 *
>> +	 * Driver default is expected to be fixed, and set in this struct
>> +	 * at init. From that point on user may change the value. There is
>> +	 * no explicit way to "unset" / restore driver default. Used only
>> +	 * when @hds_config is set.
>> +	 */
>> +	/** @hds_thresh: HDS Threshold value (ETHTOOL_A_RINGS_HDS_THRESH).
>> +	 */
>>   	u32	hds_thresh;
>> +
>> +	/* User config values
>> +	 *
>> +	 * Contain user configuration. If "set" driver must obey.
>> +	 * If "unset" driver is free to decide, and may change its choice
>> +	 * as other parameters change.
>> +	 */
>> +	/** @hds_config: HDS enabled (ETHTOOL_A_RINGS_TCP_DATA_SPLIT).
>> +	 */
>>   	u8	hds_config;
>>   };
> 
> kernel-doc comments should being with
> /**
> on a separate line. This will prevent warnings like these new ones:
> 
> Warning: include/net/netdev_queues.h:36 struct member 'hds_thresh' not described in 'netdev_config'
> Warning: include/net/netdev_queues.h:36 struct member 'hds_config' not described in 'netdev_config'
> Warning: include/net/netdev_queues.h:36 struct member 'rx_buf_len' not described in 'netdev_config'
> 
> (but there are 4 variables that this applies to. I don't know why kernel-doc.py
> only reported 3 of them.)

Thanks for taking a look! I was a bit reluctant to change it
too much since it's not authored by me. I'll be moving in direction
of removing the patch completely.

-- 
Pavel Begunkov