On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:

On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:

quoted

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.

You're right. I assumed just that. -K and other modes of operation look
like they are adapting the ETHTOOL_* calls for human consumption.
There's some additional code for analysing register and other dumps,
but I see it as just merging two tools for convenience.

quoted

     (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.

Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
is a debugging aid. What is it used for besides that?

[...]

quoted

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.

Kernel side already enhances them to not forget other features
'wanted' state. What other enhancements are on your mind?

BTW, I just recalled that ETHTOOL_F_WISH is set only as an information
about bits in features[].valid masks. So zero return does not mean, that
other features (not in .valid masks) haven't changed. This means that
userspace needs to reread features after any SFEATURES call, not just
those returning non-zero.

Best Regards,
Michał Mirosław