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

From: Michal Kubecek <mkubecek@suse.cz>
To: Jiri Pirko <jiri@resnulli.us>
Cc: Danielle Ratson <danieller@nvidia.com>,
	Andrew Lunn <andrew@lunn.ch>, Jakub Kicinski <kuba@kernel.org>,
	Ido Schimmel <idosch@idosch.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	Jiri Pirko <jiri@nvidia.com>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	mlxsw <mlxsw@nvidia.com>, Ido Schimmel <idosch@nvidia.com>,
	"johannes@sipsolutions.net" <johannes@sipsolutions.net>
Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI with lanes
Date: Mon, 19 Oct 2020 15:24:46 +0200	[thread overview]
Message-ID: <20201019132446.tgtelkzmfjdonhfx@lion.mk-sys.cz> (raw)
In-Reply-To: <20201019122643.GC11282@nanopsycho.orion>

On Mon, Oct 19, 2020 at 02:26:43PM +0200, Jiri Pirko wrote:
> Mon, Oct 19, 2020 at 01:04:22PM CEST, mkubecek@suse.cz wrote:
> >
> >It would be quite easy to extend the ethtool command line parser to
> >allow also
> >
> >  ethtool -s <dev> advertise <mode> ...
> >
> >in addition to already supported
> >
> >  ethtool -s <dev> advertise <mask>
> >  ethtool -s <dev> advertise <mask>/<mask>
> >  ethtool -s { <mode> { on | off } } ...

This should have been

  ethtool -s <dev> advertise { <mode> { on | off } } ...

> >Parser converting simple list of values into a maskless bitset is
> >already there so it would be only matter of checking if there are at
> >least two arguments and second is "on" or "off" and using corresponding
> >parser. I think it would be useful independently of this series.
> 
> Understood. So basically you will pass some selectors on cmdline and the
> uapi would stay intact.
> How do you imagine the specific lane number selection should look like
> on the cmdline?

As I said, I meant the extension suggested in my mail as independent of
what this series is about. For lanes count selector, I find proposed

  ethtool -s <dev> ... lanes <lanes_num> ...

the most natural.

From purely syntactic/semantic point of view, there are three types of
requests:

  (1) enable specific set of modes, disable the rest
  (2) enable/disable specific modes, leave the rest as they are
  (3) enable modes matching a condition (and disable the rest)

What I proposed was to allow the use symbolic names instead of masks
(which are getting more and more awful with each new mode) also for (1),
like they can already be used for (2).

The lanes selector is an extension of (3) which I would prefer not to
mix with (1) or (2) within one command line, i.e. either "advertise" or
"speed / duplex / lanes".

IIUC Jakub's and Andrew's comments were not so much about the syntax and
semantic (which is quite clear) but rather about the question if the
requests like "advertise exactly the modes with (100Gb/s speed and) two
lanes" would really address a real life need and wouldn't be more often
used as shortcuts for "advertise 100000baseKR2/Full". (On the other
hand, I suspect existing speed and duplex selectors are often used the
same way.)

Michal