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

[Jakub Kicinski <kuba@kernel.org>]

On Wed,  6 Jan 2021 15:06:16 +0200 Danielle Ratson wrote:
> From: Danielle Ratson <danieller@nvidia.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;
> +}