netdev - Re: [RFC net-next 04/22] 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: <aAf0lyGclY42Vux-@LQ3V64L9R2>
Date: Tue, 22 Apr 2025 12:57:11 -0700
From: Joe Damato <jdamato@...tly.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	donald.hunter@...il.com, sdf@...ichev.me, almasrymina@...gle.com,
	dw@...idwei.uk, asml.silence@...il.com, ap420073@...il.com,
	dtatulea@...dia.com, michael.chan@...adcom.com
Subject: Re: [RFC net-next 04/22] net: clarify the meaning of netdev_config
 members

On Mon, Apr 21, 2025 at 03:28:09PM -0700, Jakub Kicinski wrote:
> 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@...nel.org>
> ---
>  include/net/netdev_queues.h | 19 +++++++++++++++++--
>  net/ethtool/common.c        |  3 ++-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index ea709b59d827..f4eab6fc64f4 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -6,11 +6,26 @@
>  
>  /**
>   * 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.
> +	 */
> +	/** @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;
>  };

Not your fault and not exactly related to this patch, but in general
I found it a bit confusing that the two fields are different widths.