-----Original Message-----
From: Michal Kubecek <redacted>
Sent: Thursday, January 28, 2021 10:27 PM
To: Danielle Ratson <redacted>
Cc: Edwin Peer <redacted>; netdev <redacted>; David S . Miller <davem@davemloft.net>; Jakub
Kicinski [off-list ref]; Jiri Pirko [off-list ref]; Andrew Lunn [off-list ref]; f.fainelli@gmail.com; mlxsw
[off-list ref]; Ido Schimmel [off-list ref]
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:

quoted

quoted

-----Original Message-----
From: Edwin Peer <redacted>
Sent: Tuesday, January 26, 2021 7:14 PM
To: Danielle Ratson <redacted>
Cc: netdev <redacted>; David S . Miller
[off-list ref]; Jakub Kicinski [off-list ref]; Jiri Pirko
[off-list ref]; Andrew Lunn [off-list ref];
f.fainelli@gmail.com; Michal Kubecek [off-list ref]; mlxsw
[off-list ref]; Ido Schimmel [off-list ref]
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.

quoted

           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