Re: [PATCH net-next v3 05/23] net: clarify the meaning of netdev_config members

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

On 8/19/25 02:46, Mina Almasry wrote:
> On Mon, Aug 18, 2025 at 6:56 AM Pavel Begunkov <asml.silence@gmail.com> 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.
>>
> 
> TBH I can't say that moving the init to before
> dev->ethtool_ops->get_ringparam(dev, param, kparam, extack) made me
> understand semantics better. 

I agree, it didn't do it for me either ...

> If you do a respin, maybe a comment above
> the kparam->hds_thresh to say what you mean would help the next reader
> understand.

... and since the move doesn't have a strong semantical meaning, I
can't think of a good comment to put on top of the assignment.
hds_thresh is already described in struct netdev_config and it
seems like a better place for such stuff. Thoughts?

-- 
Pavel Begunkov