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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d55e4a81-e4da-47c5-95ab-03132c1c5553@gmail.com>
Date: Wed, 20 Aug 2025 13:04:40 +0100
From: Pavel Begunkov <asml.silence@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, Willem de Bruijn <willemb@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, andrew+netdev@...n.ch, horms@...nel.org,
 davem@...emloft.net, sdf@...ichev.me, dw@...idwei.uk,
 michael.chan@...adcom.com, dtatulea@...dia.com, ap420073@...il.com,
 linux-kernel@...r.kernel.org, io-uring@...r.kernel.org
Subject: Re: [PATCH net-next v3 05/23] net: clarify the meaning of
 netdev_config members

On 8/19/25 02:46, Mina Almasry wrote:
> On Mon, Aug 18, 2025 at 6:56 AM Pavel Begunkov <asml.silence@...il.com> wrote:
>>
>> From: Jakub Kicinski <kuba@...nel.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