Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

[Ben Hutchings <bhutchings@solarflare.com>]

On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
> On Fri, May 27, 2011 at 03:13:46PM +0100, Ben Hutchings wrote:
> > On Tue, 2011-05-24 at 23:59 +0200, Michał Mirosław wrote:
> > > On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:
> > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > Date: Tue, 24 May 2011 11:14:37 +0200
> > > > > On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> > > > >> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> > > > >> > You guys really need to sort this out properly.
> > > > >> > Please resubmit whatever final solution is agreed upon.
> > > > >> I noticed that v2.6.39 was tagged today. We should definitely remove
> > > > >> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> > > > >> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> > > > >> in - if we decide that the SFEATURES compatibility should be removed
> > > > >> it won't matter.
> [...]
> > > We could just wait for 2.6.40 and pretend this code part never existed. ;-)
> > I think I will make ethtool check for a minimum kernel version of 2.6.40
> > before using ETHTOOL_{G,S}FEATURES.
> 
> > > I'll rebase the first patch tomorrow. Without it the compatibility in
> > > ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.
> > This is an improvement, but I still think the fallback is fundamentally
> > broken - there's no good way for userland to tell what (if anything)
> > went wrong when the return value has ETHTOOL_F_COMPAT set.
> 
> The same situation happens with ETHTOOL_F_WISH (userspace needs to reread
> the features to find out what happened) and with old ETHTOOL_S{TSO,SG,...}
> (those return success if any of the features in the group changes and also
> posibly disable other features when one is disabled). This wasn't really
> checked before.
> 
> Ben, I think I commented on your proposal of the userspace part, but I might
> have missed some of your arguments about mine. Let's sum those up:
> 
> Your version:
>    - reimplements ETHTOOL_Sxx via ETHTOOL_SFEATURES in userspace for kernels
>      supporting the latter

No, it implements 'ethtool -K' using ETHTOOL_SFEATURES.  Maybe you
consider the ethtool utility to be a thin wrapper over the ethtool API,
but that is not my intent.

>      (note: ETHTOOL_S{SG,...} are not ever going away)
>    - causes NETIF_F_* to be an ABI

If feature flag numbers are not stable then what is the point of
/sys/class/net/<name>/features?  Also, I'm not aware that features have
ever been renumbered in the past.

I think ethtool should maintain a feature bitmask rather than the
separate flags it currently does, and I previously attempted this using
a private set of flags.  Shortly afterward that, you proposed to
introduce the new features interfaces, and it seemed to me to make sense
to use the net device feature flags in ethtool.

David, do you think feature flag numbers should be considered a
userspace (i.e. stable) ABI or not?

>    - does not support new features

Not immediately.  I intend to do that afterward.

> My version:
>    - implements only new features via ETHTOOL_SFEATURES (old calls are still used)
>    - makes feature names an ABI (for scripts actually, not the tool)
>    - supports any new features kernel reports without code changes

Right.  I definitely should incorporate your code for looking up
features by string.

> Both versions are rough at the edges, but working. Both assume that no
> behaviour changes are to be made for old '-K' options.

No, my changes are intended to enhance the old options.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.