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

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]

Date:   Sun, 4 Apr 2021 18:32:40 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Danielle Ratson <danieller@...dia.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        eric.dumazet@...il.com, mkubecek@...e.cz, f.fainelli@...il.com,
        acardace@...hat.com, irusskikh@...vell.com, gustavo@...eddedor.com,
        magnus.karlsson@...el.com, ecree@...arflare.com, idosch@...dia.com,
        jiri@...dia.com, mlxsw@...dia.com
Subject: Re: [PATCH net v2 1/2] ethtool: Add link_mode parameter capability
 bit to ethtool_ops

> @@ -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