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

[Ben Hutchings <bhutchings@solarflare.com>]

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@rere.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.

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

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.