Re: [RFC PATCH net-next v3] ethtool: Improve compatibility between netlink and ioctl interfaces

From: Ido Schimmel <idosch@idosch.org>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, davem@davemloft.net,
	f.fainelli@gmail.com, andrew@lunn.ch, David.Laight@aculab.com,
	mlxsw@nvidia.com, Ido Schimmel <idosch@nvidia.com>
Subject: Re: [RFC PATCH net-next v3] ethtool: Improve compatibility between netlink and ioctl interfaces
Date: Wed, 28 Oct 2020 19:34:36 +0200	[thread overview]
Message-ID: <20201028173436.GA504959@shredder> (raw)
In-Reply-To: <20201028005339.45daonidsidbzawn@lion.mk-sys.cz>

On Wed, Oct 28, 2020 at 01:53:39AM +0100, Michal Kubecek wrote:
> On Tue, Oct 27, 2020 at 02:53:05PM -0700, Jakub Kicinski wrote:
> > On Tue, 27 Oct 2020 16:51:14 +0200 Ido Schimmel wrote:
> > > From: Ido Schimmel <idosch@nvidia.com>
> > > 
> > > With the ioctl interface, when autoneg is enabled, but without
> > > specifying speed, duplex or link modes, the advertised link modes are
> > > set to the supported link modes by the ethtool user space utility.
> > 
> > > With the netlink interface, the same thing is done by the kernel, but
> > > only if speed or duplex are specified. In which case, the advertised
> > > link modes are set by traversing the supported link modes and picking
> > > the ones matching the specified speed or duplex.
> > 
> > > Fix this incompatibility problem by introducing a new flag in the
> > > ethtool netlink request header: 'ETHTOOL_FLAG_LEGACY'. The purpose of
> > > the flag is to indicate to the kernel that it needs to be compatible
> > > with the legacy ioctl interface. A patch to the ethtool user space
> > > utility will make sure the flag is set, when supported by the kernel.
> > 
> > I did not look at the legacy code but I'm confused by what you wrote.
> > 
> > IIUC for ioctl it's the user space that sets the advertised.
> > For netlink it's the kernel.
> > So how does the legacy flag make the kernel behave like it used to?
> 
> The idea why I suggested "legacy" as the name was that it allowed
> ethtool to preserve the old behaviour (without having to query for
> supported modes first). But from this point of view it's indeed a bit
> confusing.

I think it would be best to solve this by having user space query the
kernel for supported link modes if autoneg is being enabled without
additional parameters. Then user space will issue a set request with
ETHTOOL_A_LINKMODES_OURS being set to all supported link modes.

It does not require kernel changes and would be easier on users that
currently need to resort to old ethtool despite having a kernel that
supports netlink-based ethtool.