RE: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters

[Danielle Ratson <danieller@nvidia.com>]

> -----Original Message-----
> From: Michal Kubecek <mkubecek@suse.cz>
> Sent: Thursday, January 28, 2021 10:27 PM
> To: Danielle Ratson <danieller@nvidia.com>
> Cc: Edwin Peer <edwin.peer@broadcom.com>; netdev <netdev@vger.kernel.org>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; Jiri Pirko <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; f.fainelli@gmail.com; mlxsw
> <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>
> Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters
> 
> On Wed, Jan 27, 2021 at 01:22:02PM +0000, Danielle Ratson wrote:
> > > -----Original Message-----
> > > From: Edwin Peer <edwin.peer@broadcom.com>
> > > Sent: Tuesday, January 26, 2021 7:14 PM
> > > To: Danielle Ratson <danieller@nvidia.com>
> > > Cc: netdev <netdev@vger.kernel.org>; David S . Miller
> > > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Jiri Pirko
> > > <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>;
> > > f.fainelli@gmail.com; Michal Kubecek <mkubecek@suse.cz>; mlxsw
> > > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com>
> > > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use
> > > instead of speed and duplex parameters
> > >
> > > For one thing, it's cleaner if the driver API is symmetric. The
> > > proposed solution sets attributes in terms of speeds and lanes,
> > > etc., but it gets them in terms of a compound link_info. But, this
> > > asymmetry aside, if link_mode may eventually become R/W at the
> > > driver API, as you suggest, then it is more appropriate to guard it
> > > with a capability bit, as has been done for lanes, rather than use
> > > the -1 special value to indicate that the driver did not set it.
> > >
> > > Regards,
> > > Edwin Peer
> >
> > This patchset adds lanes parameter, not link_mode. The link_mode
> > addition was added as a read-only parameter for the reasons we
> > mentioned, and I am not sure that implementing the symmetric side is
> > relevant for this patchset.
> >
> > Michal, do you think we will use the Write side of the link_mode
> > parameter?
> 
> It makes sense, IMHO. Unless we also add "media" (or whatever name would be most appropriate) parameter, we cannot in general
> fully determine the link mode by speed, duplex and lanes. And using "advertise" to select a specific mode with disabled
> autonegotiation would be rather confusing, I believe. (At the moment, ethtool does not even support syntax like "advertise <mode>"
> but it will be easy to support "advertise <mode>... [--]" and I think we should extend the syntax to support it, regardless of what we
> choose.) So if we want to allow user to pick a specific link node by name or bit mask (or rather index?), I would prefer using a separate
> parameter.
> 
> >            And if so, do you think it is relevant for this specific
> > patchset?
> 
> I don't see an obvious problem with leaving that part for later so I would say it's up to you.
> 
> Michal

Thanks Michal.

Edwin, adding the a new parameter requires a new patchset in my opinion. Implementing the symmetrical side of the link_mode get,
however can be a part of this set. But, the problem with that would be that, as Michal said, speed lanes and duplex can't provide us a single
link_mode because of the media. And since link_mode is one bit parameter, passing it to the driver while randomly choosing the media, may
cause either information loss, or even fail to sync on a link mode if the chosen media is specifically not supported in the lower levels.
So, in my opinion it is related to adding the new parameter we discussed, and should be done in a separate set.

Thanks,
Danielle