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

[Michal Kubecek <mkubecek@suse.cz>]

On Mon, Nov 23, 2020 at 09:47:53AM +0000, Danielle Ratson wrote:
> 
> 
> > -----Original Message-----
> > From: Michal Kubecek <mkubecek@suse.cz>
> > Sent: Thursday, October 22, 2020 7:28 PM
> > To: Danielle Ratson <danieller@nvidia.com>
> > Cc: Jiri Pirko <jiri@resnulli.us>; Andrew Lunn <andrew@lunn.ch>; Jakub Kicinski <kuba@kernel.org>; Ido Schimmel
> > <idosch@idosch.org>; netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko <jiri@nvidia.com>; f.fainelli@gmail.com; mlxsw
> > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>; johannes@sipsolutions.net
> > Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes
> > 
> > On Thu, Oct 22, 2020 at 06:15:48AM +0000, Danielle Ratson wrote:
> > > > -----Original Message-----
> > > > From: Michal Kubecek <mkubecek@suse.cz>
> > > > Sent: Wednesday, October 21, 2020 11:48 AM
> > > >
> > > > Ah, right, it does. But as you extend struct ethtool_link_ksettings
> > > > and drivers will need to be updated to provide this information,
> > > > wouldn't it be more useful to let the driver provide link mode in
> > > > use instead (and derive number of lanes from it)?
> > >
> > > This is the way it is done with the speed parameter, so I have aligned
> > > it to it. Why the lanes should be done differently comparing to the
> > > speed?
> > 
> > Speed and duplex have worked this way since ages and the interface was probably introduced back in times when combination of
> > speed and duplex was sufficient to identify the link mode. This is no longer the case and even adding number of lanes wouldn't make
> > the combination unique. So if we are going to extend the interface now and update drivers to provide extra information, I believe it
> > would be more useful to provide full information.
> > 
> > Michal
> 
> Hi Michal,
> 
> What do you think of passing the link modes you have suggested as a
> bitmask, similar to "supported", that contains only one positive bit?
> Something like that:
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index afae2beacbc3..dd946c88daa3 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -127,6 +127,7 @@ struct ethtool_link_ksettings {
>                 __ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>                 __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>                 __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
> +               __ETHTOOL_DECLARE_LINK_MODE_MASK(chosen);
>         } link_modes;
>         u32     lanes;
>  };
> 
> Do you have perhaps a better suggestion?

Not sure if it's better but as we know there is only one mode, we could
simply pass the index. We would still need to reserve a special value
for none/unknown but getting an index would make lookup easier.

> And the speed and duplex parameters should be removed from being
> passed like as well, right?

We cannot remove them from struct ethtool_link_settings and the ioctl
and netlink messages as those are part of UAPI and we have to preserve
backward compatibility. But drivers which provide link mode would not
need to fill speed and duplex in their ->get_link_ksettings() as the
common code could do that for them.

Michal