netdev - Re: [PATCH net-next repost v2 1/7] ethtool: Extend link modes settings uAPI with lanes

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: <20210107163504.55018c73@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 7 Jan 2021 16:35:04 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Danielle Ratson <danieller@...lanox.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jiri@...dia.com,
        andrew@...n.ch, f.fainelli@...il.com, mkubecek@...e.cz,
        mlxsw@...dia.com, idosch@...dia.com,
        Danielle Ratson <danieller@...dia.com>
Subject: Re: [PATCH net-next repost v2 1/7] ethtool: Extend link modes
 settings uAPI with lanes

On Wed,  6 Jan 2021 15:06:16 +0200 Danielle Ratson wrote:
> From: Danielle Ratson <danieller@...dia.com>
> 
> Currently, when auto negotiation is on, the user can advertise all the
> linkmodes which correspond to a specific speed, but does not have a
> similar selector for the number of lanes. This is significant when a
> specific speed can be achieved using different number of lanes.  For
> example, 2x50 or 4x25.
> 
> Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct
> ethtool_link_settings' with lanes field in order to implement a new
> lanes-selector that will enable the user to advertise a specific number
> of lanes as well.
> 
> When auto negotiation is off, lanes parameter can be forced only if the
> driver supports it. Add a capability bit in 'struct ethtool_ops' that
> allows ethtool know if the driver can handle the lanes parameter when
> auto negotiation is off, so if it does not, an error message will be
> returned when trying to set lanes.

> @@ -420,6 +423,7 @@ struct ethtool_pause_stats {
>   * of the generic netdev features interface.
>   */
>  struct ethtool_ops {
> +	u32     capabilities;

An appropriately named bitfield seems better. Alternatively maybe let
the driver specify which lane counts it can accept?

And please remember to add the kdoc.

>  	u32	supported_coalesce_params;
>  	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
>  	int	(*get_regs_len)(struct net_device *);

> @@ -274,16 +277,17 @@ const struct nla_policy ethnl_linkmodes_set_policy[] = {
>  	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_U32 },
>  	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_U8 },
>  	[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG]	= { .type = NLA_U8 },
> +	[ETHTOOL_A_LINKMODES_LANES]		= { .type = NLA_U32 },

Please set the min and max for the policy, so userspace can at least
see that part.

> +static bool ethnl_validate_lanes_cfg(u32 cfg)
> +{
> +	switch (cfg) {
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		return true;

And with the policy checking min and max this can be turned into
a simple is_power_of_2() call.

> +	}
> +
> +	return false;
> +}