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:

quoted

On Tue, 2011-05-24 at 23:59 +0200, Michał Mirosław wrote:

quoted

On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:

quoted

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Tue, 24 May 2011 11:14:37 +0200

quoted

On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:

quoted

On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:

quoted

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.

[...]

quoted

quoted

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.

quoted

quoted

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.