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

[Pavel Begunkov]

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