Re: [PATCH net v2 1/2] ethtool: Add link_mode parameter capability bit to ethtool_ops

[Andrew Lunn <andrew@lunn.ch>]

> @@ -436,12 +436,16 @@ int __ethtool_get_link_ksettings(struct net_device *dev,
>  
>  	memset(link_ksettings, 0, sizeof(*link_ksettings));
>  
> -	link_ksettings->link_mode = -1;
>  	err = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
>  	if (err)
>  		return err;
>  
> -	if (link_ksettings->link_mode != -1) {
> +	if (dev->ethtool_ops->cap_link_mode_supported &&
> +	    link_ksettings->link_mode != -1) {

Is this -1 behaviour documented anywhere? It seems like you just
changed its meaning. It used to mean, this field has not been set,
ignore it. Adding the cap_link_mode_supported it now means, we have
field has been set, but we have no idea what link mode is being used.
So you should probably add something to the documentation of struct
ethtool_link_ksettings.

I wonder if we should actually add ETHTOOL_LINK_MODE_UNKNOWN to enum
ethtool_link_mode_bit_indices?

> +		if (WARN_ON_ONCE(link_ksettings->link_mode >=
> +				 __ETHTOOL_LINK_MODE_MASK_NBITS))
> +			return -EINVAL;
> +
>  		link_info = &link_mode_params[link_ksettings->link_mode];
>  		link_ksettings->base.speed = link_info->speed;
>  		link_ksettings->lanes = link_info->lanes;

If dev->ethtool_ops->cap_link_mode_supported &&
link_ksettings->link_mode == -1 should you be setting speed to
SPEED_UNKNOWN, and lanes to LANE_UNKNOWN? Or is that already the
default?

But over all, this API between the core and the driver seems
messy. Why not just add a helper in common.c which translates link
mode to speed/duplex/lanes and call it in the driver. Then you don't
need this capability flags, which i doubt any other driver will ever
use. And you don't need to worry about drivers returning random
values. As far as i can see, the link_mode returned by the driver is
not used for anything other than for this translation. So i don't see
a need for it outside of the driver. Or maybe i'm missing something?

	Andrew