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] [day] [month] [year] [list]

Date:   Mon, 5 Apr 2021 17:01:50 +0000
From:   Danielle Ratson <danieller@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "eric.dumazet@...il.com" <eric.dumazet@...il.com>,
        "mkubecek@...e.cz" <mkubecek@...e.cz>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "acardace@...hat.com" <acardace@...hat.com>,
        "irusskikh@...vell.com" <irusskikh@...vell.com>,
        "gustavo@...eddedor.com" <gustavo@...eddedor.com>,
        "magnus.karlsson@...el.com" <magnus.karlsson@...el.com>,
        "ecree@...arflare.com" <ecree@...arflare.com>,
        Ido Schimmel <idosch@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        mlxsw <mlxsw@...dia.com>
Subject: RE: [PATCH net v2 1/2] ethtool: Add link_mode parameter capability
 bit to ethtool_ops



> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Sunday, April 4, 2021 7:33 PM
> 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; Ido Schimmel <idosch@...dia.com>; Jiri Pirko <jiri@...dia.com>; mlxsw <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

Hi Andrew,

Please see my patch here: https://github.com/daniellerts/linux_mlxsw/commit/72ca614951418843aa87323630c354691d9e50d4
Since a lack of time until the window closes, please see if that is what you meant and you are ok with it, before I am sending another version.

Thanks,
Danielle