Message-ID: <20110516215034.GA8463@rere.qmqm.pl>
Date:	Mon, 16 May 2011 23:50:34 +0200
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT
 return

On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 22:51 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> > > On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > > > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > > > all drivers are converted to fix/set_features.
> > > > > > 
> > > > > > Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> > > > > > ---
> > > > > > 
> > > > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > > > [...]
> > > > > ETHTOOL_F_WISH means that the requested features could not all be
> > > > > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > > > > remembered.
> > > > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > > > (net: Implement SFEATURES compatibility for not updated drivers).
> > > That's also problematic because it means we can't make any use of the
> > > 'available' masks from ETHTOOL_GFEATURES.
> > > 
> > > The patch I sent is actually tested with a modified ethtool.  The
> > > fallback works.  I don't think you've tested whether any of your
> > > proposals can actually practically be used by ethtool.
> > 
> > While reading your patches I noted some differences in the way we see
> > the new [GS]FEATURES ops.
> > 
> > First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
> > feature names become an ABI instead. That's what ETH_SS_FEATURES string
> > set is for, and that's what comments in kernel's <linux/ethtool.h>
> > include say.
> 
> We've been through this before.  I can't use those names in ethtool
> because they aren't the same as ethtool used previously.  I could make
> it map strings to strings, but I don't see the point.
> 
> > dev->features are exposed directly by kernel only in two ways:
> >  1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
> >     in headers for userspace, this should be treated like a debugging
> >     facility and not an ABI
> >  2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
> >     and RX hashing) that are renamed to ETH_FLAG_* - only those constants
> >     are in the ABI and only in relation with ETHTOOL_[GS]FLAGS
> > 
> > Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
> > that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel?
> We must not.

So what's the point in reimplementing old options via ETHTOOL_SFEATURES?

> > The
> > assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
> > but there is an conversion layer in kernel that allows old binaries to
> > work correctly in the common case. (-EOPNOTSUPP is still returned for
> > drivers which can't change particular feature. The difference is seen
> > only in that disabling and enabling e.g. checksumming won't disable other
> > dependent features in the result.)
> > 
> > Right now we already agree that NETIF_F_COMPAT should go.
> > 
> > I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
> > keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
> > This might be made even more useful by adding simple wildcard matching.
> I've explained before that I do not want to add new options to do
> (mostly) the same thing.  Users should have not have to use a different
> command depending on the kernel version.

We can avoid new option by checking feature-strings for unrecognised
arguments to -K. This way, we will have the old options which work
regardless of kernel version ('tx', 'rx', 'sg', etc.) and new options
which need recent kernel anyway (separated 'tx-checksum-*', 'loopback',
others coming in for 2.6.40). Also, this way fallbacks in userspace
are avoided.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists