[PATCH net-2.6] ethtool: Remove fallback to old ethtool operations for ETHTOOL_SFEATURES

[PATCH net-2.6] ethtool: Remove fallback to old ethtool operations for ETHTOOL_SFEATURES

[Ben Hutchings]

ethtool_set_feature_compat() squashes the feature mask into a boolean,
which is not correct for ethtool_ops::set_flags.

We could fix this, but the fallback code for ETHTOOL_SFEATURES actually
makes things more complicated for the ethtool utility and any other
application using the ethtool API.  They will still need to fall back to
the old offload control commands in order to support older kernel
versions.  The fallback code in the kernel adds a third possibility for
them to handle.  So make ETHTOOL_SFEATURES fail when the driver
implements the old offload control operations, and let userland do the
fallback.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/ethtool.h |    5 ----
 net/core/ethtool.c      |   55 ++++++-----------------------------------------
 2 files changed, 7 insertions(+), 53 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index dc80d82..5fb916c 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -625,9 +625,6 @@ struct ethtool_sfeatures {
  *      Probably there are other device-specific constraints on some features
  *      in the set. When %ETHTOOL_F_UNSUPPORTED is set, .valid is considered
  *      here as though ignored bits were cleared.
- *   %ETHTOOL_F_COMPAT - some or all changes requested were made by calling
- *      compatibility functions. Requested offload state cannot be properly
- *      managed by kernel.
  *
  * Meaning of bits in the masks are obtained by %ETHTOOL_GSSET_INFO (number of
  * bits in the arrays - always multiple of 32) and %ETHTOOL_GSTRINGS commands
@@ -637,12 +634,10 @@ struct ethtool_sfeatures {
 enum ethtool_sfeatures_retval_bits {
 	ETHTOOL_F_UNSUPPORTED__BIT,
 	ETHTOOL_F_WISH__BIT,
-	ETHTOOL_F_COMPAT__BIT,
 };
 
 #define ETHTOOL_F_UNSUPPORTED   (1 << ETHTOOL_F_UNSUPPORTED__BIT)
 #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
-#define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
 
 #ifdef __KERNEL__
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 74ead9e..5f58f1f 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -207,52 +207,6 @@ static void ethtool_get_features_compat(struct net_device *dev,
 		features[0].available |= flags_dup_features;
 }
 
-static int ethtool_set_feature_compat(struct net_device *dev,
-	int (*legacy_set)(struct net_device *, u32),
-	struct ethtool_set_features_block *features, u32 mask)
-{
-	u32 do_set;
-
-	if (!legacy_set)
-		return 0;
-
-	if (!(features[0].valid & mask))
-		return 0;
-
-	features[0].valid &= ~mask;
-
-	do_set = !!(features[0].requested & mask);
-
-	if (legacy_set(dev, do_set) < 0)
-		netdev_info(dev,
-			"Legacy feature change (%s) failed for 0x%08x\n",
-			do_set ? "set" : "clear", mask);
-
-	return 1;
-}
-
-static int ethtool_set_features_compat(struct net_device *dev,
-	struct ethtool_set_features_block *features)
-{
-	int compat;
-
-	if (!dev->ethtool_ops)
-		return 0;
-
-	compat  = ethtool_set_feature_compat(dev, dev->ethtool_ops->set_sg,
-		features, NETIF_F_SG);
-	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_tx_csum,
-		features, NETIF_F_ALL_CSUM);
-	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_tso,
-		features, NETIF_F_ALL_TSO);
-	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_rx_csum,
-		features, NETIF_F_RXCSUM);
-	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_flags,
-		features, flags_dup_features);
-
-	return compat;
-}
-
 static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_gfeatures cmd = {
@@ -307,8 +261,13 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
 	if (features[0].valid & ~NETIF_F_ETHTOOL_BITS)
 		return -EINVAL;
 
-	if (ethtool_set_features_compat(dev, features))
-		ret |= ETHTOOL_F_COMPAT;
+	/* If the driver implements any of the old operations, don't
+	 * update features directly.
+	 */
+	if (dev->ethtool_ops->set_sg || dev->ethtool_ops->set_tx_csum ||
+	    dev->ethtool_ops->set_tso || dev->ethtool_ops->set_rx_csum ||
+	    dev->ethtool_ops->set_flags)
+		return -EOPNOTSUPP;
 
 	if (features[0].valid & ~dev->hw_features) {
 		features[0].valid &= dev->hw_features;
-- 
1.7.4


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

Re: [PATCH net-2.6] ethtool: Remove fallback to old ethtool operations for ETHTOOL_SFEATURES

[Michał Mirosław]

On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote:
> ethtool_set_feature_compat() squashes the feature mask into a boolean,
> which is not correct for ethtool_ops::set_flags.
> 
> We could fix this, but the fallback code for ETHTOOL_SFEATURES actually
> makes things more complicated for the ethtool utility and any other
> application using the ethtool API.  They will still need to fall back to
> the old offload control commands in order to support older kernel
> versions.  The fallback code in the kernel adds a third possibility for
> them to handle.  So make ETHTOOL_SFEATURES fail when the driver
> implements the old offload control operations, and let userland do the
> fallback.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

This will disable SFEATURES for drivers which implement changing newer
features that have no old ethtool ops (e.g.  NETIF_F_LOOPBACK), but are
not converted, yet. This might matter when bisecting.

It's easy to fix this. The code is going away for 2.6.40, though.
Do you want to get rid of ETHTOOL_F_COMPAT bit before 2.6.39?

BTW, what are the complications for userspace?

This change misses ethtool_get_features_compat() setting available bits.

Best Regards,
Michał Mirosław

Re: [PATCH net-2.6] ethtool: Remove fallback to old ethtool operations for ETHTOOL_SFEATURES

[Ben Hutchings]

[Sending this from my home address as solarflare.com mail is under
maintenance and SPF prevents me using that address entirely.]

On Sat, 2011-05-14 at 11:54 +0200, Michał Mirosław wrote:
> On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote:
> > ethtool_set_feature_compat() squashes the feature mask into a boolean,
> > which is not correct for ethtool_ops::set_flags.
> > 
> > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually
> > makes things more complicated for the ethtool utility and any other
> > application using the ethtool API.  They will still need to fall back to
> > the old offload control commands in order to support older kernel
> > versions.  The fallback code in the kernel adds a third possibility for
> > them to handle.  So make ETHTOOL_SFEATURES fail when the driver
> > implements the old offload control operations, and let userland do the
> > fallback.
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> 
> This will disable SFEATURES for drivers which implement changing newer
> features that have no old ethtool ops (e.g.  NETIF_F_LOOPBACK), but are
> not converted, yet. This might matter when bisecting.
> 
> It's easy to fix this.

Yes, I realise that.

> The code is going away for 2.6.40, though.
> Do you want to get rid of ETHTOOL_F_COMPAT bit before 2.6.39?

Right, I don't want this to ever be in a stable release.

> BTW, what are the complications for userspace?

With 2.6.38 and earlier, ETHTOOL_SFEATURES will fail; ethtool -K will
fall back to the individual operations can can report which of them
failed and why.

With 2.6.39 and a converted driver, ethtool -K can find out exactly
which features are changeable and report whether any of the individual
changes are not supported at all.  If there's something wrong with the
combination of features then ETHTOOL_SFEATURES will return
ETHTOOL_F_WISH and it can report that the combination is not supported.

With 2.6.39 and an unconverted driver, ETHTOOL_SFEATURES can return
ETHTOOL_F_COMPAT which just tells us some change failed, but not why.
We can try to guess what went wrong by reading back the features, but
it's unclear.  This is particularly problematic with set_flags (again)
where the ethtool core can't tell which flags are settable in an
unconverted driver.

I would much prefer not to have this third case existing in a single
stable release.

> This change misses ethtool_get_features_compat() setting available bits.

Yes, but that is less of a problem in practice.

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.

[PATCH] net: fix ETHTOOL_SFEATURES compatibility with old ethtool_ops.set_flags

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
Against net-next, but this should also go to net.

 net/core/ethtool.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b8c2bcf..7bb3276 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -233,6 +233,29 @@ static int ethtool_set_feature_compat(struct net_device *dev,
 	return 1;
 }
 
+static int ethtool_set_flags_compat(struct net_device *dev,
+	int (*legacy_set)(struct net_device *, u32),
+	struct ethtool_set_features_block *features, u32 mask)
+{
+	u32 value;
+
+	if (!legacy_set)
+		return 0;
+
+	if (!(features[0].valid & mask))
+		return 0;
+
+	features[0].valid &= ~mask;
+
+	value = dev->features & ~features[0].valid;
+	value |= features[0].requested;
+
+	if (legacy_set(dev, value & mask) < 0)
+		netdev_info(dev, "Legacy flags change failed\n");
+
+	return 1;
+}
+
 static int ethtool_set_features_compat(struct net_device *dev,
 	struct ethtool_set_features_block *features)
 {
@@ -249,7 +272,7 @@ static int ethtool_set_features_compat(struct net_device *dev,
 		features, NETIF_F_ALL_TSO);
 	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_rx_csum,
 		features, NETIF_F_RXCSUM);
-	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_flags,
+	compat |= ethtool_set_flags_compat(dev, dev->ethtool_ops->set_flags,
 		features, flags_dup_features);
 
 	return compat;
-- 
1.7.2.5

Re: [PATCH net-2.6] ethtool: Remove fallback to old ethtool operations for ETHTOOL_SFEATURES

[Michał Mirosław]

On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote:
> ethtool_set_feature_compat() squashes the feature mask into a boolean,
> which is not correct for ethtool_ops::set_flags.
> 
> We could fix this, but the fallback code for ETHTOOL_SFEATURES actually
> makes things more complicated for the ethtool utility and any other
> application using the ethtool API.  They will still need to fall back to
> the old offload control commands in order to support older kernel
> versions.  The fallback code in the kernel adds a third possibility for
> them to handle.  So make ETHTOOL_SFEATURES fail when the driver
> implements the old offload control operations, and let userland do the
> fallback.

BTW, the idea behind the compat code is that if ETHTOOL_[GS]FEATURES is
available, then there should be no need to fallback to old ops. For
a userspace tool that targets only kernels >= 2.6.39 there's no need
to care about old ops at all.

Best Regards,
Michał Mirosław

Re: [PATCH net-2.6] ethtool: Remove fallback to old ethtool operations for ETHTOOL_SFEATURES

[Ben Hutchings]

On Sat, 2011-05-14 at 12:35 +0200, Michał Mirosław wrote:
> On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote:
> > ethtool_set_feature_compat() squashes the feature mask into a boolean,
> > which is not correct for ethtool_ops::set_flags.
> > 
> > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually
> > makes things more complicated for the ethtool utility and any other
> > application using the ethtool API.  They will still need to fall back to
> > the old offload control commands in order to support older kernel
> > versions.  The fallback code in the kernel adds a third possibility for
> > them to handle.  So make ETHTOOL_SFEATURES fail when the driver
> > implements the old offload control operations, and let userland do the
> > fallback.
> 
> BTW, the idea behind the compat code is that if ETHTOOL_[GS]FEATURES is
> available, then there should be no need to fallback to old ops. For
> a userspace tool that targets only kernels >= 2.6.39 there's no need
> to care about old ops at all.

Well that's not true, because those tools still have to deal with
ETHTOOL_F_COMPAT.  And we're supposed to have all drivers converted for
2.6.40, so the hypothetical new tool only has to wait one more release.

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.

Re: [PATCH net-2.6] ethtool: Remove fallback to old ethtool operations for ETHTOOL_SFEATURES

[Michał Mirosław]

On Mon, May 16, 2011 at 03:45:23AM +0100, Ben Hutchings wrote:
> On Sat, 2011-05-14 at 12:35 +0200, Michał Mirosław wrote:
> > On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote:
> > > ethtool_set_feature_compat() squashes the feature mask into a boolean,
> > > which is not correct for ethtool_ops::set_flags.
> > > 
> > > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually
> > > makes things more complicated for the ethtool utility and any other
> > > application using the ethtool API.  They will still need to fall back to
> > > the old offload control commands in order to support older kernel
> > > versions.  The fallback code in the kernel adds a third possibility for
> > > them to handle.  So make ETHTOOL_SFEATURES fail when the driver
> > > implements the old offload control operations, and let userland do the
> > > fallback.
> > BTW, the idea behind the compat code is that if ETHTOOL_[GS]FEATURES is
> > available, then there should be no need to fallback to old ops. For
> > a userspace tool that targets only kernels >= 2.6.39 there's no need
> > to care about old ops at all.
> Well that's not true, because those tools still have to deal with
> ETHTOOL_F_COMPAT.  And we're supposed to have all drivers converted for
> 2.6.40, so the hypothetical new tool only has to wait one more release.

I think that handling ETHTOOL_F_COMPAT is like ETHTOOL_F_WISH - the tool
needs to reread features and check what was changed. So we can get rid
of the former by replacing it with the latter.

Changing some feature state may change others that were not requested
to be changed (not present in .valid). This probably should be signalled
as well. Extend ETHTOOL_F_WISH meaning (I prefer this one) or introduce
new bit?

Best Regards,
Michał Mirosław

[PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

[Michał Mirosław]

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.

 include/linux/ethtool.h |    5 -----
 net/core/ethtool.c      |    2 +-
 2 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index dc80d82..5fb916c 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -625,9 +625,6 @@ struct ethtool_sfeatures {
  *      Probably there are other device-specific constraints on some features
  *      in the set. When %ETHTOOL_F_UNSUPPORTED is set, .valid is considered
  *      here as though ignored bits were cleared.
- *   %ETHTOOL_F_COMPAT - some or all changes requested were made by calling
- *      compatibility functions. Requested offload state cannot be properly
- *      managed by kernel.
  *
  * Meaning of bits in the masks are obtained by %ETHTOOL_GSSET_INFO (number of
  * bits in the arrays - always multiple of 32) and %ETHTOOL_GSTRINGS commands
@@ -637,12 +634,10 @@ struct ethtool_sfeatures {
 enum ethtool_sfeatures_retval_bits {
 	ETHTOOL_F_UNSUPPORTED__BIT,
 	ETHTOOL_F_WISH__BIT,
-	ETHTOOL_F_COMPAT__BIT,
 };
 
 #define ETHTOOL_F_UNSUPPORTED   (1 << ETHTOOL_F_UNSUPPORTED__BIT)
 #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
-#define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
 
 #ifdef __KERNEL__
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 74ead9e..dc07569 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -308,7 +308,7 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
 		return -EINVAL;
 
 	if (ethtool_set_features_compat(dev, features))
-		ret |= ETHTOOL_F_COMPAT;
+		ret |= ETHTOOL_F_WISH;
 
 	if (features[0].valid & ~dev->hw_features) {
 		features[0].valid &= dev->hw_features;

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

[Ben Hutchings]

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.

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.

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

[Michał Mirosław]

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

Best Regards,
Michał Mirosław

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

[Ben Hutchings]

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.

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.

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

[Michał Mirosław]

W dniu 16 maja 2011 16:53 użytkownik Ben Hutchings
<bhutchings@solarflare.com> napisał:
> 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.

You haven't posted the updates to ethtool to netdev. I just work blind here.

For now I use userspace version I sent along some of the first series
of the conversion patches. It actually makes it easier to use if the
compatibility handling stays in kernel (doesn't care about
ETHTOOL_F_COMPAT, BTW).

Best Regards,
Michał Mirosław

[RFC PATCH ethtool 1/3] ethtool: Regularise offload feature settings

[Ben Hutchings]

This is partly preparation for use of the new net device features API,
but is useful in its own right.

Replace repetitive code for getting/setting offload flags with data-
driven loops.

This changes error messages to use the same long names for offload
flags as in dump_offload(), and changes various exit codes to 1.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 ethtool-util.h |   24 ++++
 ethtool.c      |  321 ++++++++++++++++++-------------------------------------
 2 files changed, 129 insertions(+), 216 deletions(-)

diff --git a/ethtool-util.h b/ethtool-util.h
index 79be7f2..10b8c00 100644
--- a/ethtool-util.h
+++ b/ethtool-util.h
@@ -67,6 +67,30 @@ static inline u64 cpu_to_be64(u64 value)
 
 #define	RX_CLS_LOC_UNSPEC	0xffffffffUL
 
+#ifndef NETIF_F_SG
+#define NETIF_F_SG		(1 << 0)
+#define NETIF_F_IP_CSUM		(1 << 1)
+#define NETIF_F_NO_CSUM		(1 << 2)
+#define NETIF_F_HW_CSUM		(1 << 3)
+#define NETIF_F_IPV6_CSUM	(1 << 4)
+#define NETIF_F_HW_VLAN_TX	(1 << 7)
+#define NETIF_F_HW_VLAN_RX	(1 << 8)
+#define NETIF_F_HW_VLAN_FILTER	(1 << 9)
+#define NETIF_F_GSO		(1 << 11)
+#define NETIF_F_GRO		(1 << 14)
+#define NETIF_F_LRO		(1 << 15)
+#define NETIF_F_TSO		(1 << 16)
+#define NETIF_F_UFO		(1 << 17)
+#define NETIF_F_TSO_ECN		(1 << 19)
+#define NETIF_F_TSO6		(1 << 20)
+#define NETIF_F_NTUPLE		(1 << 27)
+#define NETIF_F_RXHASH		(1 << 28)
+#define NETIF_F_RXCSUM		(1 << 29)
+#define NETIF_F_ALL_CSUM	(NETIF_F_NO_CSUM | NETIF_F_HW_CSUM | \
+				 NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)
+#define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
+#endif
+
 /* National Semiconductor DP83815, DP83816 */
 int natsemi_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
 int natsemi_dump_eeprom(struct ethtool_drvinfo *info,
diff --git a/ethtool.c b/ethtool.c
index 34fe107..419f349 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -297,15 +297,8 @@ static void show_usage(void)
 static char *devname = NULL;
 
 static int goffload_changed = 0;
-static int off_csum_rx_wanted = -1;
-static int off_csum_tx_wanted = -1;
-static int off_sg_wanted = -1;
-static int off_tso_wanted = -1;
-static int off_ufo_wanted = -1;
-static int off_gso_wanted = -1;
-static u32 off_flags_wanted = 0;
-static u32 off_flags_mask = 0;
-static int off_gro_wanted = -1;
+static u32 off_features_wanted = 0;
+static u32 off_features_mask = 0;
 
 static struct ethtool_pauseparam epause;
 static int gpause_changed = 0;
@@ -446,23 +439,30 @@ static struct cmdline_info cmdline_seeprom[] = {
 };
 
 static struct cmdline_info cmdline_offload[] = {
-	{ "rx", CMDL_BOOL, &off_csum_rx_wanted, NULL },
-	{ "tx", CMDL_BOOL, &off_csum_tx_wanted, NULL },
-	{ "sg", CMDL_BOOL, &off_sg_wanted, NULL },
-	{ "tso", CMDL_BOOL, &off_tso_wanted, NULL },
-	{ "ufo", CMDL_BOOL, &off_ufo_wanted, NULL },
-	{ "gso", CMDL_BOOL, &off_gso_wanted, NULL },
-	{ "lro", CMDL_FLAG, &off_flags_wanted, NULL,
-	  ETH_FLAG_LRO, &off_flags_mask },
-	{ "gro", CMDL_BOOL, &off_gro_wanted, NULL },
-	{ "rxvlan", CMDL_FLAG, &off_flags_wanted, NULL,
-	  ETH_FLAG_RXVLAN, &off_flags_mask },
-	{ "txvlan", CMDL_FLAG, &off_flags_wanted, NULL,
-	  ETH_FLAG_TXVLAN, &off_flags_mask },
-	{ "ntuple", CMDL_FLAG, &off_flags_wanted, NULL,
-	  ETH_FLAG_NTUPLE, &off_flags_mask },
-	{ "rxhash", CMDL_FLAG, &off_flags_wanted, NULL,
-	  ETH_FLAG_RXHASH, &off_flags_mask },
+	{ "rx", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_RXCSUM, &off_features_mask },
+	{ "tx", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_ALL_CSUM, &off_features_mask },
+	{ "sg", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_SG, &off_features_mask },
+	{ "tso", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_ALL_TSO, &off_features_mask },
+	{ "ufo", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_UFO, &off_features_mask },
+	{ "gso", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_GSO, &off_features_mask },
+	{ "lro", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_LRO, &off_features_mask },
+	{ "gro", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_GRO, &off_features_mask },
+	{ "rxvlan", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_HW_VLAN_TX, &off_features_mask },
+	{ "txvlan", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_HW_VLAN_TX, &off_features_mask },
+	{ "ntuple", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_NTUPLE, &off_features_mask },
+	{ "rxhash", CMDL_FLAG, &off_features_wanted, NULL,
+	  NETIF_F_RXHASH, &off_features_mask },
 };
 
 static struct cmdline_info cmdline_pause[] = {
@@ -1788,35 +1788,36 @@ static int dump_coalesce(void)
 	return 0;
 }
 
-static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,
-			int gro, int lro, int rxvlan, int txvlan, int ntuple,
-			int rxhash)
+static const struct {
+	const char *long_name;
+	u32 cmd;
+	u32 value;
+} off_feature_def[] = {
+	{ "rx-checksumming",		  ETHTOOL_GRXCSUM, NETIF_F_RXCSUM },
+	{ "tx-checksumming",		  ETHTOOL_GTXCSUM, NETIF_F_ALL_CSUM },
+	{ "scatter-gather",		  ETHTOOL_GSG,	   NETIF_F_SG },
+	{ "tcp-segmentation-offload",	  ETHTOOL_GTSO,	   NETIF_F_ALL_TSO },
+	{ "udp-fragmentation-offload",	  ETHTOOL_GUFO,	   NETIF_F_UFO },
+	{ "generic-segmentation-offload", ETHTOOL_GGSO,	   NETIF_F_GSO },
+	{ "generic-receive-offload",	  ETHTOOL_GGRO,	   NETIF_F_GRO },
+	{ "large-receive-offload",	  0,		   NETIF_F_LRO },
+	{ "rx-vlan-offload",		  0,		   NETIF_F_HW_VLAN_RX },
+	{ "tx-vlan-offload",		  0,		   NETIF_F_HW_VLAN_TX },
+	{ "ntuple-filters",		  0,		   NETIF_F_NTUPLE },
+	{ "receive-hashing",		  0,		   NETIF_F_RXHASH },
+};
+
+static int dump_offload(u32 active)
 {
-	fprintf(stdout,
-		"rx-checksumming: %s\n"
-		"tx-checksumming: %s\n"
-		"scatter-gather: %s\n"
-		"tcp-segmentation-offload: %s\n"
-		"udp-fragmentation-offload: %s\n"
-		"generic-segmentation-offload: %s\n"
-		"generic-receive-offload: %s\n"
-		"large-receive-offload: %s\n"
-		"rx-vlan-offload: %s\n"
-		"tx-vlan-offload: %s\n"
-		"ntuple-filters: %s\n"
-		"receive-hashing: %s\n",
-		rx ? "on" : "off",
-		tx ? "on" : "off",
-		sg ? "on" : "off",
-		tso ? "on" : "off",
-		ufo ? "on" : "off",
-		gso ? "on" : "off",
-		gro ? "on" : "off",
-		lro ? "on" : "off",
-		rxvlan ? "on" : "off",
-		txvlan ? "on" : "off",
-		ntuple ? "on" : "off",
-		rxhash ? "on" : "off");
+	u32 value;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
+		value = off_feature_def[i].value;
+		printf("%s: %s\n",
+		       off_feature_def[i].long_name,
+		       (active & value) ? "on" : "off");
+	}
 
 	return 0;
 }
@@ -2139,73 +2140,38 @@ static int do_scoalesce(int fd, struct ifreq *ifr)
 	return 0;
 }
 
+/* the following list of flags are the same as their associated
+ * NETIF_F_xxx values in include/linux/netdevice.h
+ */
+static const u32 flags_dup_features =
+	(ETH_FLAG_LRO | ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN | ETH_FLAG_NTUPLE |
+	 ETH_FLAG_RXHASH);
+
 static int do_goffload(int fd, struct ifreq *ifr)
 {
 	struct ethtool_value eval;
-	int err, allfail = 1, rx = 0, tx = 0, sg = 0;
-	int tso = 0, ufo = 0, gso = 0, gro = 0, lro = 0, rxvlan = 0, txvlan = 0,
-	    ntuple = 0, rxhash = 0;
+	int err, allfail = 1;
+	u32 features = 0, value;
+	int i;
 
 	fprintf(stdout, "Offload parameters for %s:\n", devname);
 
-	eval.cmd = ETHTOOL_GRXCSUM;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = send_ioctl(fd, ifr);
-	if (err)
-		perror("Cannot get device rx csum settings");
-	else {
-		rx = eval.data;
-		allfail = 0;
-	}
-
-	eval.cmd = ETHTOOL_GTXCSUM;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = send_ioctl(fd, ifr);
-	if (err)
-		perror("Cannot get device tx csum settings");
-	else {
-		tx = eval.data;
-		allfail = 0;
-	}
-
-	eval.cmd = ETHTOOL_GSG;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = send_ioctl(fd, ifr);
-	if (err)
-		perror("Cannot get device scatter-gather settings");
-	else {
-		sg = eval.data;
-		allfail = 0;
-	}
-
-	eval.cmd = ETHTOOL_GTSO;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = send_ioctl(fd, ifr);
-	if (err)
-		perror("Cannot get device tcp segmentation offload settings");
-	else {
-		tso = eval.data;
-		allfail = 0;
-	}
-
-	eval.cmd = ETHTOOL_GUFO;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = ioctl(fd, SIOCETHTOOL, ifr);
-	if (err)
-		perror("Cannot get device udp large send offload settings");
-	else {
-		ufo = eval.data;
-		allfail = 0;
-	}
-
-	eval.cmd = ETHTOOL_GGSO;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = ioctl(fd, SIOCETHTOOL, ifr);
-	if (err)
-		perror("Cannot get device generic segmentation offload settings");
-	else {
-		gso = eval.data;
-		allfail = 0;
+	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
+		value = off_feature_def[i].value;
+		if (!off_feature_def[i].cmd)
+			continue;
+		eval.cmd = off_feature_def[i].cmd;
+		ifr->ifr_data = (caddr_t)&eval;
+		err = send_ioctl(fd, ifr);
+		if (err) {
+			fprintf(stderr,
+				"Cannot get device %s settings: %m\n",
+				off_feature_def[i].long_name);
+		} else {
+			if (eval.data)
+				features |= value;
+			allfail = 0;
+		}
 	}
 
 	eval.cmd = ETHTOOL_GFLAGS;
@@ -2214,21 +2180,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err) {
 		perror("Cannot get device flags");
 	} else {
-		lro = (eval.data & ETH_FLAG_LRO) != 0;
-		rxvlan = (eval.data & ETH_FLAG_RXVLAN) != 0;
-		txvlan = (eval.data & ETH_FLAG_TXVLAN) != 0;
-		ntuple = (eval.data & ETH_FLAG_NTUPLE) != 0;
-		rxhash = (eval.data & ETH_FLAG_RXHASH) != 0;
-		allfail = 0;
-	}
-
-	eval.cmd = ETHTOOL_GGRO;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = ioctl(fd, SIOCETHTOOL, ifr);
-	if (err)
-		perror("Cannot get device GRO settings");
-	else {
-		gro = eval.data;
+		features |= eval.data & flags_dup_features;
 		allfail = 0;
 	}
 
@@ -2237,86 +2189,33 @@ static int do_goffload(int fd, struct ifreq *ifr)
 		return 83;
 	}
 
-	return dump_offload(rx, tx, sg, tso, ufo, gso, gro, lro, rxvlan, txvlan,
-			    ntuple, rxhash);
+	return dump_offload(features);
 }
 
 static int do_soffload(int fd, struct ifreq *ifr)
 {
 	struct ethtool_value eval;
-	int err, changed = 0;
-
-	if (off_csum_rx_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_SRXCSUM;
-		eval.data = (off_csum_rx_wanted == 1);
-		ifr->ifr_data = (caddr_t)&eval;
-		err = send_ioctl(fd, ifr);
-		if (err) {
-			perror("Cannot set device rx csum settings");
-			return 84;
-		}
-	}
-
-	if (off_csum_tx_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_STXCSUM;
-		eval.data = (off_csum_tx_wanted == 1);
-		ifr->ifr_data = (caddr_t)&eval;
-		err = send_ioctl(fd, ifr);
-		if (err) {
-			perror("Cannot set device tx csum settings");
-			return 85;
-		}
-	}
-
-	if (off_sg_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_SSG;
-		eval.data = (off_sg_wanted == 1);
-		ifr->ifr_data = (caddr_t)&eval;
-		err = send_ioctl(fd, ifr);
-		if (err) {
-			perror("Cannot set device scatter-gather settings");
-			return 86;
-		}
-	}
+	int err;
+	int i;
 
-	if (off_tso_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_STSO;
-		eval.data = (off_tso_wanted == 1);
-		ifr->ifr_data = (caddr_t)&eval;
-		err = send_ioctl(fd, ifr);
-		if (err) {
-			perror("Cannot set device tcp segmentation offload settings");
-			return 88;
-		}
-	}
-	if (off_ufo_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_SUFO;
-		eval.data = (off_ufo_wanted == 1);
-		ifr->ifr_data = (caddr_t)&eval;
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set device udp large send offload settings");
-			return 89;
-		}
-	}
-	if (off_gso_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_SGSO;
-		eval.data = (off_gso_wanted == 1);
-		ifr->ifr_data = (caddr_t)&eval;
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set device generic segmentation offload settings");
-			return 90;
+	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
+		if (!off_feature_def[i].cmd)
+			continue;
+		if (off_features_mask & off_feature_def[i].value) {
+			eval.cmd = off_feature_def[i].cmd + 1;
+			eval.data = !!(off_features_wanted &
+				       off_feature_def[i].value);
+			ifr->ifr_data = (caddr_t)&eval;
+			err = send_ioctl(fd, ifr);
+			if (err) {
+				fprintf(stderr,
+					"Cannot set device %s settings: %m\n",
+					off_feature_def[i].long_name);
+				return 1;
+			}
 		}
 	}
-	if (off_flags_mask) {
-		changed = 1;
+	if (off_features_mask & flags_dup_features) {
 		eval.cmd = ETHTOOL_GFLAGS;
 		eval.data = 0;
 		ifr->ifr_data = (caddr_t)&eval;
@@ -2327,8 +2226,9 @@ static int do_soffload(int fd, struct ifreq *ifr)
 		}
 
 		eval.cmd = ETHTOOL_SFLAGS;
-		eval.data = ((eval.data & ~off_flags_mask) |
-			     off_flags_wanted);
+		eval.data &= ~(off_features_mask & flags_dup_features);
+		eval.data |= (off_features_wanted &
+			      flags_dup_features);
 
 		err = ioctl(fd, SIOCETHTOOL, ifr);
 		if (err) {
@@ -2336,19 +2236,8 @@ static int do_soffload(int fd, struct ifreq *ifr)
 			return 92;
 		}
 	}
-	if (off_gro_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_SGRO;
-		eval.data = (off_gro_wanted == 1);
-		ifr->ifr_data = (caddr_t)&eval;
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set device GRO settings");
-			return 93;
-		}
-	}
 
-	if (!changed) {
+	if (off_features_mask == 0) {
 		fprintf(stdout, "no offload settings changed\n");
 	}
 
-- 
1.7.4



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

[RFC PATCH ethtool 2/3] ethtool: Report any consequential offload feature changes

[Ben Hutchings]

When an offload feature is enabled or disabled, this can change the
state of other features that depend on it.  Report any such changes.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 ethtool.c |   47 +++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 419f349..5eeca64 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1807,13 +1807,15 @@ static const struct {
 	{ "receive-hashing",		  0,		   NETIF_F_RXHASH },
 };
 
-static int dump_offload(u32 active)
+static int dump_offload(u32 active, u32 mask)
 {
 	u32 value;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
 		value = off_feature_def[i].value;
+		if (!(mask & value))
+			continue;
 		printf("%s: %s\n",
 		       off_feature_def[i].long_name,
 		       (active & value) ? "on" : "off");
@@ -2147,14 +2149,14 @@ static const u32 flags_dup_features =
 	(ETH_FLAG_LRO | ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN | ETH_FLAG_NTUPLE |
 	 ETH_FLAG_RXHASH);
 
-static int do_goffload(int fd, struct ifreq *ifr)
+static int get_offload(int fd, struct ifreq *ifr, u32 *features)
 {
 	struct ethtool_value eval;
 	int err, allfail = 1;
-	u32 features = 0, value;
+	u32 value;
 	int i;
 
-	fprintf(stdout, "Offload parameters for %s:\n", devname);
+	*features = 0; 
 
 	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
 		value = off_feature_def[i].value;
@@ -2169,7 +2171,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 				off_feature_def[i].long_name);
 		} else {
 			if (eval.data)
-				features |= value;
+				*features |= value;
 			allfail = 0;
 		}
 	}
@@ -2180,24 +2182,39 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err) {
 		perror("Cannot get device flags");
 	} else {
-		features |= eval.data & flags_dup_features;
+		*features |= eval.data & flags_dup_features;
 		allfail = 0;
 	}
 
-	if (allfail) {
+	return allfail;
+}
+
+static int do_goffload(int fd, struct ifreq *ifr)
+{
+	u32 features;
+
+	fprintf(stdout, "Offload parameters for %s:\n", devname);
+
+	if (get_offload(fd, ifr, &features)) {
 		fprintf(stdout, "no offload info available\n");
 		return 83;
 	}
 
-	return dump_offload(features);
+	return dump_offload(features, ~(u32)0);
 }
 
 static int do_soffload(int fd, struct ifreq *ifr)
 {
+	u32 old_features, new_features, diff;
 	struct ethtool_value eval;
 	int err;
 	int i;
 
+	if (get_offload(fd, ifr, &old_features)) {
+		fprintf(stderr, "no offload info available\n");
+		return 1;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
 		if (!off_feature_def[i].cmd)
 			continue;
@@ -2239,6 +2256,20 @@ static int do_soffload(int fd, struct ifreq *ifr)
 
 	if (off_features_mask == 0) {
 		fprintf(stdout, "no offload settings changed\n");
+		return 0;
+	}
+
+	/* Were any additional changes made automatically? */
+	if (get_offload(fd, ifr, &new_features)) {
+		fprintf(stderr, "no offload info available\n");
+		return 1;
+	}
+	diff = ((old_features & ~off_features_mask) |
+		(off_features_wanted & off_features_mask)) ^
+		new_features;
+	if (diff) {
+		printf("Additional changes:\n");
+		dump_offload(new_features, diff);
 	}
 
 	return 0;
-- 
1.7.4



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

[RFC PATCH ethtool 3/3] ethtool: Use ETHTOOL_{G,S}FEATURES where available

[Ben Hutchings]

Also use the new structures in any case.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 ethtool.c |  294 +++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 199 insertions(+), 95 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 5eeca64..db1873c 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -297,8 +297,7 @@ static void show_usage(void)
 static char *devname = NULL;
 
 static int goffload_changed = 0;
-static u32 off_features_wanted = 0;
-static u32 off_features_mask = 0;
+struct ethtool_set_features_block off_features;
 
 static struct ethtool_pauseparam epause;
 static int gpause_changed = 0;
@@ -439,30 +438,30 @@ static struct cmdline_info cmdline_seeprom[] = {
 };
 
 static struct cmdline_info cmdline_offload[] = {
-	{ "rx", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_RXCSUM, &off_features_mask },
-	{ "tx", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_ALL_CSUM, &off_features_mask },
-	{ "sg", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_SG, &off_features_mask },
-	{ "tso", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_ALL_TSO, &off_features_mask },
-	{ "ufo", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_UFO, &off_features_mask },
-	{ "gso", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_GSO, &off_features_mask },
-	{ "lro", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_LRO, &off_features_mask },
-	{ "gro", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_GRO, &off_features_mask },
-	{ "rxvlan", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_HW_VLAN_TX, &off_features_mask },
-	{ "txvlan", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_HW_VLAN_TX, &off_features_mask },
-	{ "ntuple", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_NTUPLE, &off_features_mask },
-	{ "rxhash", CMDL_FLAG, &off_features_wanted, NULL,
-	  NETIF_F_RXHASH, &off_features_mask },
+	{ "rx", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_RXCSUM, &off_features.valid },
+	{ "tx", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_ALL_CSUM, &off_features.valid },
+	{ "sg", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_SG, &off_features.valid },
+	{ "tso", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_ALL_TSO, &off_features.valid },
+	{ "ufo", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_UFO, &off_features.valid },
+	{ "gso", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_GSO, &off_features.valid },
+	{ "lro", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_LRO, &off_features.valid },
+	{ "gro", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_GRO, &off_features.valid },
+	{ "rxvlan", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_HW_VLAN_TX, &off_features.valid },
+	{ "txvlan", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_HW_VLAN_TX, &off_features.valid },
+	{ "ntuple", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_NTUPLE, &off_features.valid },
+	{ "rxhash", CMDL_FLAG, &off_features.requested, NULL,
+	  NETIF_F_RXHASH, &off_features.valid },
 };
 
 static struct cmdline_info cmdline_pause[] = {
@@ -1807,7 +1806,8 @@ static const struct {
 	{ "receive-hashing",		  0,		   NETIF_F_RXHASH },
 };
 
-static int dump_offload(u32 active, u32 mask)
+static int
+dump_offload(const struct ethtool_get_features_block *features, u32 mask)
 {
 	u32 value;
 	int i;
@@ -1816,9 +1816,12 @@ static int dump_offload(u32 active, u32 mask)
 		value = off_feature_def[i].value;
 		if (!(mask & value))
 			continue;
-		printf("%s: %s\n",
+		printf("%s: %s%s%s\n",
 		       off_feature_def[i].long_name,
-		       (active & value) ? "on" : "off");
+		       (features->active & value) ? "on" : "off",
+		       (features->requested & ~features->active & value) ?
+		       " [requested on]" : "",
+		       !(features->available & value) ? " [unchangeable]" : "");
 	}
 
 	return 0;
@@ -2149,41 +2152,65 @@ static const u32 flags_dup_features =
 	(ETH_FLAG_LRO | ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN | ETH_FLAG_NTUPLE |
 	 ETH_FLAG_RXHASH);
 
-static int get_offload(int fd, struct ifreq *ifr, u32 *features)
+static int get_offload(int fd, struct ifreq *ifr,
+		       struct ethtool_get_features_block *pfeatures)
 {
+	struct {
+		struct ethtool_gfeatures cmd;
+		struct ethtool_get_features_block data[1];
+	} features;
 	struct ethtool_value eval;
 	int err, allfail = 1;
 	u32 value;
 	int i;
 
-	*features = 0; 
+	features.cmd.cmd = ETHTOOL_GFEATURES;
+	features.cmd.size = ARRAY_SIZE(features.data);
+	ifr->ifr_data = (caddr_t)&features;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err == 0) {
+		allfail = 0;
+		pfeatures[0] = features.data[0];
+	} else if (errno != EOPNOTSUPP && errno != EPERM) {
+		perror("Cannot get device offload settings");
+	} else {
+		memset(pfeatures, 0, sizeof(*pfeatures));
 
-	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
-		value = off_feature_def[i].value;
-		if (!off_feature_def[i].cmd)
-			continue;
-		eval.cmd = off_feature_def[i].cmd;
+		for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
+			value = off_feature_def[i].value;
+
+			/* Assume that anything we can get is changeable */
+			pfeatures[0].available |= value;
+
+			if (!off_feature_def[i].cmd)
+				continue;
+
+			eval.cmd = off_feature_def[i].cmd;
+			ifr->ifr_data = (caddr_t)&eval;
+			err = send_ioctl(fd, ifr);
+			if (err) {
+				fprintf(stderr,
+					"Cannot get device %s settings: %m\n",
+					off_feature_def[i].long_name);
+			} else {
+				if (eval.data)
+					pfeatures[0].active |= value;
+				allfail = 0;
+			}
+		}
+
+		eval.cmd = ETHTOOL_GFLAGS;
 		ifr->ifr_data = (caddr_t)&eval;
-		err = send_ioctl(fd, ifr);
+		err = ioctl(fd, SIOCETHTOOL, ifr);
 		if (err) {
-			fprintf(stderr,
-				"Cannot get device %s settings: %m\n",
-				off_feature_def[i].long_name);
+			perror("Cannot get device flags");
 		} else {
-			if (eval.data)
-				*features |= value;
+			pfeatures[0].active |=
+				eval.data & flags_dup_features;
 			allfail = 0;
 		}
-	}
 
-	eval.cmd = ETHTOOL_GFLAGS;
-	ifr->ifr_data = (caddr_t)&eval;
-	err = ioctl(fd, SIOCETHTOOL, ifr);
-	if (err) {
-		perror("Cannot get device flags");
-	} else {
-		*features |= eval.data & flags_dup_features;
-		allfail = 0;
+		pfeatures[0].requested = pfeatures[0].active;
 	}
 
 	return allfail;
@@ -2191,7 +2218,7 @@ static int get_offload(int fd, struct ifreq *ifr, u32 *features)
 
 static int do_goffload(int fd, struct ifreq *ifr)
 {
-	u32 features;
+	struct ethtool_get_features_block features;
 
 	fprintf(stdout, "Offload parameters for %s:\n", devname);
 
@@ -2200,79 +2227,156 @@ static int do_goffload(int fd, struct ifreq *ifr)
 		return 83;
 	}
 
-	return dump_offload(features, ~(u32)0);
+	return dump_offload(&features, ~(u32)0);
 }
 
 static int do_soffload(int fd, struct ifreq *ifr)
 {
-	u32 old_features, new_features, diff;
-	struct ethtool_value eval;
+	struct ethtool_get_features_block old_features, new_features;
+	struct {
+		struct ethtool_sfeatures cmd;
+		struct ethtool_set_features_block data[1];
+	} set_features;
+	int failed;
+	u32 diff;
 	int err;
 	int i;
 
+	if (off_features.valid == 0) {
+		fprintf(stdout, "no offload settings changed\n");
+		return 0;
+	}
+
 	if (get_offload(fd, ifr, &old_features)) {
 		fprintf(stderr, "no offload info available\n");
 		return 1;
 	}
 
+	set_features.cmd.cmd = ETHTOOL_SFEATURES;
+	set_features.cmd.size = ARRAY_SIZE(set_features.data);
+	set_features.data[0] = off_features;
+
 	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
-		if (!off_feature_def[i].cmd)
+		u32 value = off_feature_def[i].value;
+
+		if (!(off_features.valid & value))
 			continue;
-		if (off_features_mask & off_feature_def[i].value) {
-			eval.cmd = off_feature_def[i].cmd + 1;
-			eval.data = !!(off_features_wanted &
-				       off_feature_def[i].value);
-			ifr->ifr_data = (caddr_t)&eval;
-			err = send_ioctl(fd, ifr);
-			if (err) {
-				fprintf(stderr,
-					"Cannot set device %s settings: %m\n",
-					off_feature_def[i].long_name);
-				return 1;
-			}
+		if (!(old_features.available & value)) {
+			/* None of these features can be changed */
+			fprintf(stderr,
+				"Cannot set device %s settings: "
+				"Operation not supported\n",
+				off_feature_def[i].long_name);
+			return 1;
 		}
+		/* At least some of these features can be
+		 * enabled; mask out any that cannot
+		 */
+		set_features.data[0].valid &=
+			~(value & ~old_features.available);
 	}
-	if (off_features_mask & flags_dup_features) {
-		eval.cmd = ETHTOOL_GFLAGS;
-		eval.data = 0;
-		ifr->ifr_data = (caddr_t)&eval;
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot get device flag settings");
-			return 91;
+
+	ifr->ifr_data = (caddr_t)&set_features;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err >= 0) {
+		/* We'll break down the warnings/errors below */
+	} else if (errno != EOPNOTSUPP && errno != EPERM) {
+		perror("Cannot set device offload settings");
+		return 1;
+	} else {
+		struct ethtool_value eval;
+
+		for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
+			if (!off_feature_def[i].cmd)
+				continue;
+			if (off_features.valid & off_feature_def[i].value) {
+				eval.cmd = off_feature_def[i].cmd + 1;
+				eval.data = !!(off_features.requested &
+					       off_feature_def[i].value);
+				ifr->ifr_data = (caddr_t)&eval;
+				err = send_ioctl(fd, ifr);
+				if (err) {
+					fprintf(stderr,
+						"Cannot set device %s settings: %m\n",
+						off_feature_def[i].long_name);
+					return 1;
+				}
+			}
 		}
+		if (off_features.valid & flags_dup_features) {
+			eval.cmd = ETHTOOL_GFLAGS;
+			eval.data = 0;
+			ifr->ifr_data = (caddr_t)&eval;
+			err = ioctl(fd, SIOCETHTOOL, ifr);
+			if (err) {
+				perror("Cannot get device flag settings");
+				return 91;
+			}
 
-		eval.cmd = ETHTOOL_SFLAGS;
-		eval.data &= ~(off_features_mask & flags_dup_features);
-		eval.data |= (off_features_wanted &
-			      flags_dup_features);
+			eval.cmd = ETHTOOL_SFLAGS;
+			eval.data &= ~(off_features.valid & flags_dup_features);
+			eval.data |= (off_features.requested &
+				      flags_dup_features);
 
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set device flag settings");
-			return 92;
+			err = ioctl(fd, SIOCETHTOOL, ifr);
+			if (err) {
+				perror("Cannot set device flag settings");
+				return 92;
+			}
 		}
 	}
 
-	if (off_features_mask == 0) {
-		fprintf(stdout, "no offload settings changed\n");
-		return 0;
-	}
-
-	/* Were any additional changes made automatically? */
 	if (get_offload(fd, ifr, &new_features)) {
 		fprintf(stderr, "no offload info available\n");
 		return 1;
 	}
-	diff = ((old_features & ~off_features_mask) |
-		(off_features_wanted & off_features_mask)) ^
-		new_features;
+
+	failed = 0;
+
+	/* Report specific failures */
+	for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
+		if (!(off_feature_def[i].value & off_features.valid))
+			continue;
+
+		/* For offloads where we have one name for multiple
+		 * feature flags, squash them into a boolean.
+		 */
+		if (!(off_features.requested & off_feature_def[i].value) != 
+		    !(new_features.active & off_feature_def[i].value)) {
+			int did1 = 0;
+
+			fprintf(stderr,
+				"Cannot set device %s settings",
+				off_feature_def[i].long_name);
+			if (err & ETHTOOL_F_WISH) {
+				fprintf(stderr,
+					": Feature depends on other settings");
+				did1++;
+			}
+			/* Report any remaining warning flags (including
+			 * ETHTOOL_F_UNSUPPORTED, which should not be set
+			 * since we checked for unsupported flags above).
+			 */
+			if (err & ~ETHTOOL_F_WISH) {
+				fprintf(stderr, "%s Warning flags %#x",
+					did1 ? ";" : ":",
+					err & ~ETHTOOL_F_WISH);
+			}
+			fprintf(stderr, "\n");
+
+			failed = 1;
+		}
+	}
+
+	/* Were any additional changes made automatically? */
+	diff = (new_features.active ^ old_features.active) &
+		    ~off_features.valid;
 	if (diff) {
 		printf("Additional changes:\n");
-		dump_offload(new_features, diff);
+		dump_offload(&new_features, diff);
 	}
 
-	return 0;
+	return failed;
 }
 
 static int do_gset(int fd, struct ifreq *ifr)
-- 
1.7.4


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

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

[Michał Mirosław]

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.

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

Best Regards,
Michał Mirosław

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

[Ben Hutchings]

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.

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

[Michał Mirosław]

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

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

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

[Ben Hutchings]

On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> 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@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.
> 
> So what's the point in reimplementing old options via ETHTOOL_SFEATURES?

Where, in ethtool?  The benefits include:
- Kernel remembers all the features the user wants on, even if the
combination is impossible.  Turning TX checksumming off and on no longer
forces TSO off.
- ethtool can distinguish and report whether a feature is unsupported or
its dependencies are not met.

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

This is just too subtle a distinction.  It will mostly confuse users.

> Also, this way fallbacks in userspace are avoided.

No, ethtool will be supporting kernels <2.6.40 for many years yet.

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.

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

[Michał Mirosław]

On Mon, May 16, 2011 at 11:09:35PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> > 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@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.
> > 
> > So what's the point in reimplementing old options via ETHTOOL_SFEATURES?
> 
> Where, in ethtool?  The benefits include:
> - Kernel remembers all the features the user wants on, even if the
> combination is impossible.  Turning TX checksumming off and on no longer
> forces TSO off.

This is what you get by using old ethtool on new kernel. And only for
converted drivers, whether using SFEATURES or old calls.

> - ethtool can distinguish and report whether a feature is unsupported or
> its dependencies are not met.

In this case, when feature is unsupported at all, you still get -EOPNOTSUPP.
If you get no error from old call but after readback (via GSG, etc.) the
feature is still disabled - it means that there are some unmet dependencies.
This is the same information you get from [GS]FEATURES calls.

> > > > 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).
> This is just too subtle a distinction.  It will mostly confuse users.

We should just document the difference. I expect users who don't care about
new features to not read docs. So 'tx' will still mean 'all TX checksumming'
for them, and they will expect it to turn all TX checksumming
offloads driver supports. If the set changes (like: even in earlier kernels,
some drivers add NETIF_F_SCTP_CSUM to this set) you'll need to update
ethtool userspace. That won't happen if you keep using old calls for
old options as the change will be contained in kernel-side wrapper.

> > Also, this way fallbacks in userspace are avoided.
> No, ethtool will be supporting kernels <2.6.40 for many years yet.

Sure it will. I meant no fallbacks for old options (because they aren't
needed for the tool to work correctly) and no fallback for new options
(as that is not supported in old kernels anyway).

Best Regards,
Michał Mirosław

[RFC PATCH ethtool] ethtool: merge ETHTOOL_[GS]FEATURES support to -k/-K modes

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---

This depends on the previous patch introducing -w/-W for [GS]FEATURES.

BTW, I noticed an old bug in ethtool (present currently and in debian-lenny's
version 6+20080913-1): "ethtool -k" -- i.e. with no other parameters -- runs
and tries to check device named '-k'.

Example:

icybox:~# ./ethtool -k ge0
Offload parameters for ge0:
rx-checksumming: on
tx-checksumming: off
scatter-gather: off
tcp-segmentation-offload: off
udp-fragmentation-offload: off
generic-segmentation-offload: off
generic-receive-offload: on
large-receive-offload: off
rx-vlan-offload: off
tx-vlan-offload: off
ntuple-filters: off
receive-hashing: off

Full offload state:  (feature-name: active,wanted,changable)
     tx-scatter-gather                 no,yes,yes
     tx-checksum-ipv4                  no, no,yes
     tx-checksum-unneeded              no,---, no
     tx-checksum-ip-generic            no,---, no
     tx_checksum-ipv6                  no, no,yes
     highdma                           no,---, no
     tx-scatter-gather-fraglist        no,---, no
     tx-vlan-hw-insert                 no,---, no
     rx-vlan-hw-parse                  no,---, no
     rx-vlan-filter                    no,---, no
     vlan-challenged                   no,---,---
     tx-generic-segmentation           no,yes,yes
     tx-lockless                       no,---,---
     netns-local                       no,---,---
     rx-gro                           yes,yes,yes
     rx-lro                            no,---, no
     tx-tcp-segmentation               no, no,yes
     tx-udp-fragmentation              no,---, no
     tx-gso-robust                     no,---, no
     tx-tcp-ecn-segmentation           no, no,yes
     tx-tcp6-segmentation              no, no,yes
     tx-fcoe-segmentation              no,---, no
     tx-checksum-fcoe-crc              no,---, no
     tx-checksum-sctp                  no,---, no
     fcoe-mtu                          no,---, no
     rx-ntuple-filter                  no,---, no
     rx-hashing                        no,---, no
     rx-checksum                      yes,yes,yes
     tx-nocache-copy                   no, no,yes
     loopback                          no,---, no

icybox:~# ./ethtool -K ge0 tx_checksum-ipv6 on
feature tx-scatter-gather is enabled (expected: disabled, saved: enabled)
feature tx-generic-segmentation is enabled (expected: disabled, saved: enabled)

icybox:~# ./ethtool -K ge0 sg off
[turns off SG and GSO; like before]

icybox:~# ./ethtool -K ge0 tx_checksum-ipv6 off
[no other feature changed state]

icybox:~# ./ethtool -K ge0 sg on
[SG was remembered this time, but is inactive (no checksum offloads)]

icybox:~# ./ethtool -K ge0 tx_checksum-ipv6 on
feature tx-scatter-gather is enabled (expected: disabled, saved: enabled)
feature tx-generic-segmentation is enabled (expected: disabled, saved: enabled)

icybox:~# ./ethtool -K ge0 tx_checksum-ipv6 off
feature tx-scatter-gather is disabled (expected: enabled, saved: enabled)
feature tx-generic-segmentation is disabled (expected: enabled, saved: enabled)

---
 ethtool.c |  155 ++++++++++++++++++++++++++++--------------------------------
 1 files changed, 72 insertions(+), 83 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 86a5a8b..a541007 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -123,8 +123,6 @@ static enum {
 	MODE_SRING,
 	MODE_GOFFLOAD,
 	MODE_SOFFLOAD,
-	MODE_GFEATURES,
-	MODE_SFEATURES,
 	MODE_GSTATS,
 	MODE_GNFC,
 	MODE_SNFC,
@@ -202,9 +200,6 @@ static struct option {
 		"		[ txvlan on|off ]\n"
 		"		[ ntuple on|off ]\n"
 		"		[ rxhash on|off ]\n"
-    },
-    { "-w", "--show-features", MODE_GFEATURES, "Get offload status" },
-    { "-W", "--request-features", MODE_SFEATURES, "Set requested offload",
 		"		[ feature-name on|off [...] ]\n"
 		"		see --show-features output for feature-name strings\n" },
     { "-i", "--driver", MODE_GDRV, "Show driver information" },
@@ -306,7 +301,6 @@ static void show_usage(void)
 
 static char *devname = NULL;
 
-static int goffload_changed = 0;
 static int off_csum_rx_wanted = -1;
 static int off_csum_tx_wanted = -1;
 static int off_sg_wanted = -1;
@@ -316,6 +310,7 @@ static int off_gso_wanted = -1;
 static u32 off_flags_wanted = 0;
 static u32 off_flags_mask = 0;
 static int off_gro_wanted = -1;
+static struct ethtool_sfeatures *features_req;
 
 static struct ethtool_pauseparam epause;
 static int gpause_changed = 0;
@@ -778,8 +773,6 @@ static void parse_cmdline(int argc, char **argp)
 			    (mode == MODE_SRING) ||
 			    (mode == MODE_GOFFLOAD) ||
 			    (mode == MODE_SOFFLOAD) ||
-			    (mode == MODE_GFEATURES) ||
-			    (mode == MODE_SFEATURES) ||
 			    (mode == MODE_GSTATS) ||
 			    (mode == MODE_GNFC) ||
 			    (mode == MODE_SNFC) ||
@@ -863,14 +856,6 @@ static void parse_cmdline(int argc, char **argp)
 				break;
 			}
 			if (mode == MODE_SOFFLOAD) {
-				parse_generic_cmdline(argc, argp, i,
-					&goffload_changed,
-			      		cmdline_offload,
-			      		ARRAY_SIZE(cmdline_offload));
-				i = argc;
-				break;
-			}
-			if (mode == MODE_SFEATURES) {
 				parse_sfeatures_args(argc, argp, i);
 				i = argc;
 				break;
@@ -1944,10 +1929,6 @@ static int doit(void)
 		return do_goffload(fd, &ifr);
 	} else if (mode == MODE_SOFFLOAD) {
 		return do_soffload(fd, &ifr);
-	} else if (mode == MODE_GFEATURES) {
-		return do_gfeatures(fd, &ifr);
-	} else if (mode == MODE_SFEATURES) {
-		return do_sfeatures(fd, &ifr);
 	} else if (mode == MODE_GSTATS) {
 		return do_gstats(fd, &ifr);
 	} else if (mode == MODE_GNFC) {
@@ -2262,13 +2243,20 @@ static int do_goffload(int fd, struct ifreq *ifr)
 		allfail = 0;
 	}
 
+	if (!allfail)
+		dump_offload(rx, tx, sg, tso, ufo, gso, gro, lro, rxvlan, txvlan,
+			    ntuple, rxhash);
+
+	err = do_gfeatures(fd, ifr);
+	if (!err)
+		allfail = 0;
+
 	if (allfail) {
 		fprintf(stdout, "no offload info available\n");
 		return 83;
 	}
 
-	return dump_offload(rx, tx, sg, tso, ufo, gso, gro, lro, rxvlan, txvlan,
-			    ntuple, rxhash);
+	return 0;
 }
 
 static int do_soffload(int fd, struct ifreq *ifr)
@@ -2277,116 +2265,114 @@ static int do_soffload(int fd, struct ifreq *ifr)
 	int err, changed = 0;
 
 	if (off_csum_rx_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SRXCSUM;
 		eval.data = (off_csum_rx_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device rx csum settings");
-			return 84;
-		}
+		else
+			changed = 1;
 	}
 
 	if (off_csum_tx_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_STXCSUM;
 		eval.data = (off_csum_tx_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device tx csum settings");
-			return 85;
-		}
+		else
+			changed = 1;
 	}
 
 	if (off_sg_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SSG;
 		eval.data = (off_sg_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device scatter-gather settings");
-			return 86;
-		}
+		else
+			changed = 1;
 	}
 
 	if (off_tso_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_STSO;
 		eval.data = (off_tso_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device tcp segmentation offload settings");
-			return 88;
-		}
+		else
+			changed = 1;
 	}
 	if (off_ufo_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SUFO;
 		eval.data = (off_ufo_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device udp large send offload settings");
-			return 89;
-		}
+		else
+			changed = 1;
 	}
 	if (off_gso_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SGSO;
 		eval.data = (off_gso_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device generic segmentation offload settings");
-			return 90;
-		}
+		else
+			changed = 1;
 	}
 	if (off_flags_mask) {
-		changed = 1;
 		eval.cmd = ETHTOOL_GFLAGS;
 		eval.data = 0;
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
 		if (err) {
 			perror("Cannot get device flag settings");
-			return 91;
-		}
+		} else {
+			eval.cmd = ETHTOOL_SFLAGS;
+			eval.data = ((eval.data & ~off_flags_mask) |
+				     off_flags_wanted);
 
-		eval.cmd = ETHTOOL_SFLAGS;
-		eval.data = ((eval.data & ~off_flags_mask) |
-			     off_flags_wanted);
-
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set device flag settings");
-			return 92;
+			err = ioctl(fd, SIOCETHTOOL, ifr);
+			if (err)
+				perror("Cannot set device flag settings");
+			else
+				changed = 1;
 		}
 	}
 	if (off_gro_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SGRO;
 		eval.data = (off_gro_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device GRO settings");
-			return 93;
-		}
+		else
+			changed = 1;
+	}
+
+	if (features_req) {
+		err = do_sfeatures(fd, ifr);
+		if (!err)
+			changed = 1;
 	}
 
 	if (!changed) {
 		fprintf(stdout, "no offload settings changed\n");
+		return err;
 	}
 
 	return 0;
 }
 
 static int get_feature_strings(int fd, struct ifreq *ifr,
-	struct ethtool_gstrings **strs)
+	struct ethtool_gstrings **strs, int quiet_nx)
 {
 	struct ethtool_sset_info *sset_info;
 	struct ethtool_gstrings *strings;
@@ -2398,16 +2384,18 @@ static int get_feature_strings(int fd, struct ifreq *ifr,
 	ifr->ifr_data = (caddr_t)sset_info;
 	err = send_ioctl(fd, ifr);
 
-	if ((err < 0) ||
-	    (!(sset_info->sset_mask & (1ULL << ETH_SS_FEATURES)))) {
-		perror("Cannot get driver strings info");
-		return -100;
-	}
-
 	n_strings = sset_info->data[0];
 	free(sset_info);
+
+	if ((err < 0) ||
+	    (!(sset_info->sset_mask & (1ULL << ETH_SS_FEATURES))) ||
+	    (n_strings == 0)) {
+		if (!quiet_nx)
+			perror("Cannot get driver strings info");
+		return -100;
+	}
+
 	sz_str = n_strings * ETH_GSTRING_LEN;
-
 	strings = calloc(1, sz_str + sizeof(struct ethtool_gstrings));
 	if (!strings) {
 		fprintf(stderr, "no memory available\n");
@@ -2429,8 +2417,6 @@ static int get_feature_strings(int fd, struct ifreq *ifr,
 	return n_strings;
 }
 
-struct ethtool_sfeatures *features_req;
-
 static void parse_sfeatures_args(int argc, char **argp, int argi)
 {
 	struct cmdline_info *cmdline_desc, *cp;
@@ -2443,13 +2429,21 @@ static void parse_sfeatures_args(int argc, char **argp, int argi)
 	if (fd < 0)
 		exit(100);
 
-	n_strings = get_feature_strings(fd, &ifr, &strings);
-	if (n_strings < 0)
-		exit(-n_strings);
+	n_strings = get_feature_strings(fd, &ifr, &strings, 1);
+	if (n_strings < 0) {
+		/* ETHTOOL_GFEATURES unavailable */
+		parse_generic_cmdline(argc, argp, argi, &changed,
+			cmdline_offload, ARRAY_SIZE(cmdline_offload));
+		return;
+	}
 
 	sz_features = sizeof(*features_req->features) * ((n_strings + 31) / 32);
 
-	cp = cmdline_desc = calloc(n_strings, sizeof(*cmdline_desc));
+	cp = cmdline_desc = calloc(n_strings + ARRAY_SIZE(cmdline_offload),
+		sizeof(*cmdline_desc));
+	memcpy(cp, cmdline_offload, sizeof(cmdline_offload));
+	cp += ARRAY_SIZE(cmdline_offload);
+
 	features_req = calloc(1, sizeof(*features_req) + sz_features);
 	if (!cmdline_desc || !features_req) {
 		fprintf(stderr, "no memory available\n");
@@ -2518,7 +2512,7 @@ static int do_gfeatures(int fd, struct ifreq *ifr)
 	struct ethtool_gfeatures *features;
 	int n_strings, err, i;
 
-	n_strings = get_feature_strings(fd, ifr, &strings);
+	n_strings = get_feature_strings(fd, ifr, &strings, 1);
 	if (n_strings < 0)
 		return -n_strings;
 
@@ -2528,7 +2522,7 @@ static int do_gfeatures(int fd, struct ifreq *ifr)
 		return err;
 	}
 
-	fprintf(stdout, "Offload state:  (name: enabled,wanted,changable)\n");
+	fprintf(stdout, "\nFull offload state:  (feature-name: active,wanted,changable)\n");
 	for (i = 0; i < n_strings; i++) {
 		if (!strings->data[i * ETH_GSTRING_LEN])
 			continue;	/* empty */
@@ -2585,12 +2579,7 @@ static int do_sfeatures(int fd, struct ifreq *ifr)
 	struct ethtool_gfeatures *features0, *features1;
 	int n_strings, err, i;
 
-	if (!features_req) {
-		fprintf(stderr, "no features changed\n");
-		return 97;
-	}
-
-	n_strings = get_feature_strings(fd, ifr, &strings);
+	n_strings = get_feature_strings(fd, ifr, &strings, 0);
 	if (n_strings < 0) {
 		free(features_req);
 		return -n_strings;
-- 
1.7.2.5

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

[Ben Hutchings]

On Mon, 2011-05-16 at 23:09 +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
[...]
> > > 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).
> 
> This is just too subtle a distinction.  It will mostly confuse users.
[...]

Sorry, I think I misunderstood you here.  I agree that new feature names
that do not correspond exactly to existing keywords should be supported
as keywords after the -K option.  I think those that do (e.g.
"tx-udp-fragmentation" vs "ufo") should not be, as adding a
kernel-version-dependent *alias* would be confusing.

I also want users to benefit from your improvements (as I explained
above) even when they use the old names, if they are using a new kernel
version.  That is why I want ethtool to try using ETHTOOL_SFEATURES
first, and why the fallback in the kernel is problematic.

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.

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

[Michał Mirosław]

On Wed, May 18, 2011 at 08:02:59PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 23:09 +0100, Ben Hutchings wrote:
> > On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> > > On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
> > > > 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).
> > This is just too subtle a distinction.  It will mostly confuse users.
> Sorry, I think I misunderstood you here.  I agree that new feature names
> that do not correspond exactly to existing keywords should be supported
> as keywords after the -K option.  I think those that do (e.g.
> "tx-udp-fragmentation" vs "ufo") should not be, as adding a
> kernel-version-dependent *alias* would be confusing.

The alias can be marked as such in the documentation. Shouldn't it be
that hard for a user to read the manpage to know what the new options
are for when he sees them. I don't like the idea of translating strings,
either, because if e.g. ufo becomes split in the feature to ufo4+ufo6
or new checksum offloads are implemented, it will break.

> I also want users to benefit from your improvements (as I explained
> above) even when they use the old names, if they are using a new kernel
> version.  That is why I want ethtool to try using ETHTOOL_SFEATURES
> first, and why the fallback in the kernel is problematic.

Which benefits do you want to have? If checking what other features
changed with selected one, it's easily done by rereading the state -
possibly with GFEATURES.

I'll cook another PoC patch over those I sent to show the idea.

Best Regards,
Michał Mirosław

[RFC PATCH v3 ethtool] ethtool: implement [GS]FEATURES calls

[Michał Mirosław]

This is all-in-one PoC patch for [GS]FEATURES support and checking of
all feature changes when altering some.

Example result:

icybox:~# ./ethtool -K ge0 tx_checksum-ipv6 on
feature group tx is enabled (expected: disabled)
feature group sg is enabled (expected: disabled)
feature group gso is enabled (expected: disabled)
feature tx-scatter-gather is enabled (expected: disabled, saved: enabled)
feature tx-generic-segmentation is enabled (expected: disabled, saved: enabled)

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 ethtool.c |  583 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 494 insertions(+), 89 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 34fe107..40456bb 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -33,6 +33,7 @@
 #include <limits.h>
 #include <ctype.h>
 
+#include <unistd.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
@@ -83,6 +84,8 @@ static int do_gcoalesce(int fd, struct ifreq *ifr);
 static int do_scoalesce(int fd, struct ifreq *ifr);
 static int do_goffload(int fd, struct ifreq *ifr);
 static int do_soffload(int fd, struct ifreq *ifr);
+static void parse_sfeatures_args(int argc, char **argp, int argi);
+static int do_gfeatures(int fd, struct ifreq *ifr);
 static int do_gstats(int fd, struct ifreq *ifr);
 static int rxflow_str_to_type(const char *str);
 static int parse_rxfhashopts(char *optstr, u32 *data);
@@ -196,7 +199,8 @@ static struct option {
 		"		[ txvlan on|off ]\n"
 		"		[ ntuple on|off ]\n"
 		"		[ rxhash on|off ]\n"
-    },
+		"		[ feature-name on|off [...] ]\n"
+		"		see --show-offload output for feature-name strings\n" },
     { "-i", "--driver", MODE_GDRV, "Show driver information" },
     { "-d", "--register-dump", MODE_GREGS, "Do a register dump",
 		"		[ raw on|off ]\n"
@@ -296,7 +300,6 @@ static void show_usage(void)
 
 static char *devname = NULL;
 
-static int goffload_changed = 0;
 static int off_csum_rx_wanted = -1;
 static int off_csum_tx_wanted = -1;
 static int off_sg_wanted = -1;
@@ -306,6 +309,9 @@ static int off_gso_wanted = -1;
 static u32 off_flags_wanted = 0;
 static u32 off_flags_mask = 0;
 static int off_gro_wanted = -1;
+static int n_feature_strings;
+static const char **feature_strings;
+static struct ethtool_sfeatures *features_req;
 
 static struct ethtool_pauseparam epause;
 static int gpause_changed = 0;
@@ -851,10 +857,7 @@ static void parse_cmdline(int argc, char **argp)
 				break;
 			}
 			if (mode == MODE_SOFFLOAD) {
-				parse_generic_cmdline(argc, argp, i,
-					&goffload_changed,
-			      		cmdline_offload,
-			      		ARRAY_SIZE(cmdline_offload));
+				parse_sfeatures_args(argc, argp, i);
 				i = argc;
 				break;
 			}
@@ -1788,9 +1791,15 @@ static int dump_coalesce(void)
 	return 0;
 }
 
-static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,
-			int gro, int lro, int rxvlan, int txvlan, int ntuple,
-			int rxhash)
+struct offload_state {
+	int rx, tx, sg, tso, ufo, gso, gro, lro, rxvlan, txvlan, ntuple, rxhash;
+};
+
+const char *const old_feature_names[] = {
+	"rx", "tx", "sg", "tso", "ufo", "gso", "gro", "lro", "rxvlan", "txvlan", "ntuple", "rxhash"
+};
+
+static int dump_offload(const struct offload_state *offload)
 {
 	fprintf(stdout,
 		"rx-checksumming: %s\n"
@@ -1805,18 +1814,18 @@ static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,
 		"tx-vlan-offload: %s\n"
 		"ntuple-filters: %s\n"
 		"receive-hashing: %s\n",
-		rx ? "on" : "off",
-		tx ? "on" : "off",
-		sg ? "on" : "off",
-		tso ? "on" : "off",
-		ufo ? "on" : "off",
-		gso ? "on" : "off",
-		gro ? "on" : "off",
-		lro ? "on" : "off",
-		rxvlan ? "on" : "off",
-		txvlan ? "on" : "off",
-		ntuple ? "on" : "off",
-		rxhash ? "on" : "off");
+		offload->rx ? "on" : "off",
+		offload->tx ? "on" : "off",
+		offload->sg ? "on" : "off",
+		offload->tso ? "on" : "off",
+		offload->ufo ? "on" : "off",
+		offload->gso ? "on" : "off",
+		offload->gro ? "on" : "off",
+		offload->lro ? "on" : "off",
+		offload->rxvlan ? "on" : "off",
+		offload->txvlan ? "on" : "off",
+		offload->ntuple ? "on" : "off",
+		offload->rxhash ? "on" : "off");
 
 	return 0;
 }
@@ -1867,21 +1876,33 @@ static int dump_rxfhash(int fhash, u64 val)
 	return 0;
 }
 
-static int doit(void)
-{
-	struct ifreq ifr;
-	int fd;
+static int control_fd = -1;
 
+static int get_control_socket(struct ifreq *ifr)
+{
 	/* Setup our control structures. */
-	memset(&ifr, 0, sizeof(ifr));
-	strcpy(ifr.ifr_name, devname);
+	memset(ifr, 0, sizeof(*ifr));
+	strcpy(ifr->ifr_name, devname);
+
+	if (control_fd >= 0)
+		return control_fd;
 
 	/* Open control socket. */
-	fd = socket(AF_INET, SOCK_DGRAM, 0);
-	if (fd < 0) {
+	control_fd = socket(AF_INET, SOCK_DGRAM, 0);
+	if (control_fd < 0)
 		perror("Cannot get control socket");
+
+	return control_fd;
+}
+
+static int doit(void)
+{
+	struct ifreq ifr;
+	int fd;
+
+	fd = get_control_socket(&ifr);
+	if (fd < 0)
 		return 70;
-	}
 
 	/* all of these are expected to populate ifr->ifr_data as needed */
 	if (mode == MODE_GDRV) {
@@ -2139,14 +2160,13 @@ static int do_scoalesce(int fd, struct ifreq *ifr)
 	return 0;
 }
 
-static int do_goffload(int fd, struct ifreq *ifr)
+static int send_goffloads(int fd, struct ifreq *ifr,
+	struct offload_state *offload)
 {
 	struct ethtool_value eval;
-	int err, allfail = 1, rx = 0, tx = 0, sg = 0;
-	int tso = 0, ufo = 0, gso = 0, gro = 0, lro = 0, rxvlan = 0, txvlan = 0,
-	    ntuple = 0, rxhash = 0;
+	int err, allfail = 1;
 
-	fprintf(stdout, "Offload parameters for %s:\n", devname);
+	memset(offload, 0, sizeof(*offload));
 
 	eval.cmd = ETHTOOL_GRXCSUM;
 	ifr->ifr_data = (caddr_t)&eval;
@@ -2154,7 +2174,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err)
 		perror("Cannot get device rx csum settings");
 	else {
-		rx = eval.data;
+		offload->rx = eval.data;
 		allfail = 0;
 	}
 
@@ -2164,7 +2184,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err)
 		perror("Cannot get device tx csum settings");
 	else {
-		tx = eval.data;
+		offload->tx = eval.data;
 		allfail = 0;
 	}
 
@@ -2174,7 +2194,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err)
 		perror("Cannot get device scatter-gather settings");
 	else {
-		sg = eval.data;
+		offload->sg = eval.data;
 		allfail = 0;
 	}
 
@@ -2184,7 +2204,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err)
 		perror("Cannot get device tcp segmentation offload settings");
 	else {
-		tso = eval.data;
+		offload->tso = eval.data;
 		allfail = 0;
 	}
 
@@ -2194,7 +2214,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err)
 		perror("Cannot get device udp large send offload settings");
 	else {
-		ufo = eval.data;
+		offload->ufo = eval.data;
 		allfail = 0;
 	}
 
@@ -2204,7 +2224,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err)
 		perror("Cannot get device generic segmentation offload settings");
 	else {
-		gso = eval.data;
+		offload->gso = eval.data;
 		allfail = 0;
 	}
 
@@ -2214,11 +2234,11 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err) {
 		perror("Cannot get device flags");
 	} else {
-		lro = (eval.data & ETH_FLAG_LRO) != 0;
-		rxvlan = (eval.data & ETH_FLAG_RXVLAN) != 0;
-		txvlan = (eval.data & ETH_FLAG_TXVLAN) != 0;
-		ntuple = (eval.data & ETH_FLAG_NTUPLE) != 0;
-		rxhash = (eval.data & ETH_FLAG_RXHASH) != 0;
+		offload->lro = (eval.data & ETH_FLAG_LRO) != 0;
+		offload->rxvlan = (eval.data & ETH_FLAG_RXVLAN) != 0;
+		offload->txvlan = (eval.data & ETH_FLAG_TXVLAN) != 0;
+		offload->ntuple = (eval.data & ETH_FLAG_NTUPLE) != 0;
+		offload->rxhash = (eval.data & ETH_FLAG_RXHASH) != 0;
 		allfail = 0;
 	}
 
@@ -2228,130 +2248,515 @@ static int do_goffload(int fd, struct ifreq *ifr)
 	if (err)
 		perror("Cannot get device GRO settings");
 	else {
-		gro = eval.data;
+		offload->gro = eval.data;
 		allfail = 0;
 	}
 
+	return -allfail;
+}
+
+static int do_goffload(int fd, struct ifreq *ifr)
+{
+	struct offload_state offload;
+	int err, allfail;
+
+	allfail = send_goffloads(fd, ifr, &offload);
+
+	if (!allfail) {
+		fprintf(stdout, "Offload parameters for %s:\n", devname);
+
+		dump_offload(&offload);
+	}
+
+	err = do_gfeatures(fd, ifr);
+	if (!err)
+		allfail = 0;
+
 	if (allfail) {
 		fprintf(stdout, "no offload info available\n");
 		return 83;
 	}
 
-	return dump_offload(rx, tx, sg, tso, ufo, gso, gro, lro, rxvlan, txvlan,
-			    ntuple, rxhash);
+	return 0;
 }
 
-static int do_soffload(int fd, struct ifreq *ifr)
+static int send_soffloads(int fd, struct ifreq *ifr)
 {
 	struct ethtool_value eval;
 	int err, changed = 0;
 
 	if (off_csum_rx_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SRXCSUM;
 		eval.data = (off_csum_rx_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device rx csum settings");
-			return 84;
-		}
+		else
+			changed = 1;
 	}
 
 	if (off_csum_tx_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_STXCSUM;
 		eval.data = (off_csum_tx_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device tx csum settings");
-			return 85;
-		}
+		else
+			changed = 1;
 	}
 
 	if (off_sg_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SSG;
 		eval.data = (off_sg_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device scatter-gather settings");
-			return 86;
-		}
+		else
+			changed = 1;
 	}
 
 	if (off_tso_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_STSO;
 		eval.data = (off_tso_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = send_ioctl(fd, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device tcp segmentation offload settings");
-			return 88;
-		}
+		else
+			changed = 1;
 	}
 	if (off_ufo_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SUFO;
 		eval.data = (off_ufo_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device udp large send offload settings");
-			return 89;
-		}
+		else
+			changed = 1;
 	}
 	if (off_gso_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SGSO;
 		eval.data = (off_gso_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device generic segmentation offload settings");
-			return 90;
-		}
+		else
+			changed = 1;
 	}
 	if (off_flags_mask) {
-		changed = 1;
 		eval.cmd = ETHTOOL_GFLAGS;
 		eval.data = 0;
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
 		if (err) {
 			perror("Cannot get device flag settings");
-			return 91;
-		}
-
-		eval.cmd = ETHTOOL_SFLAGS;
-		eval.data = ((eval.data & ~off_flags_mask) |
-			     off_flags_wanted);
-
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set device flag settings");
-			return 92;
+		} else {
+			eval.cmd = ETHTOOL_SFLAGS;
+			eval.data = ((eval.data & ~off_flags_mask) |
+				     off_flags_wanted);
+
+			err = ioctl(fd, SIOCETHTOOL, ifr);
+			if (err)
+				perror("Cannot set device flag settings");
+			else
+				changed = 1;
 		}
 	}
 	if (off_gro_wanted >= 0) {
-		changed = 1;
 		eval.cmd = ETHTOOL_SGRO;
 		eval.data = (off_gro_wanted == 1);
 		ifr->ifr_data = (caddr_t)&eval;
 		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
+		if (err)
 			perror("Cannot set device GRO settings");
-			return 93;
+		else
+			changed = 1;
+	}
+
+	return changed;
+}
+
+static int get_feature_strings(int fd, struct ifreq *ifr,
+	struct ethtool_gstrings **strs)
+{
+	struct ethtool_sset_info *sset_info;
+	struct ethtool_gstrings *strings;
+	int sz_str, n_strings, err;
+
+	sset_info = malloc(sizeof(struct ethtool_sset_info) + sizeof(u32));
+	sset_info->cmd = ETHTOOL_GSSET_INFO;
+	sset_info->sset_mask = (1ULL << ETH_SS_FEATURES);
+	ifr->ifr_data = (caddr_t)sset_info;
+	err = send_ioctl(fd, ifr);
+
+	n_strings = sset_info->data[0];
+	free(sset_info);
+
+	if ((err < 0) ||
+	    (!(sset_info->sset_mask & (1ULL << ETH_SS_FEATURES))) ||
+	    (n_strings == 0)) {
+		return -100;
+	}
+
+	sz_str = n_strings * ETH_GSTRING_LEN;
+	strings = calloc(1, sz_str + sizeof(struct ethtool_gstrings));
+	if (!strings) {
+		fprintf(stderr, "no memory available\n");
+		exit(95);
+	}
+
+	strings->cmd = ETHTOOL_GSTRINGS;
+	strings->string_set = ETH_SS_FEATURES;
+	strings->len = n_strings;
+	ifr->ifr_data = (caddr_t) strings;
+	err = send_ioctl(fd, ifr);
+	if (err < 0) {
+		perror("Cannot get feature strings information");
+		free(strings);
+		exit(96);
+	}
+
+	*strs = strings;
+	return n_strings;
+}
+
+static int init_feature_strings(void)
+{
+	struct ethtool_gstrings *strings;
+	struct ifreq ifr;
+	int fd, i, n;
+
+	if (feature_strings)
+		return n_feature_strings;
+
+	fd = get_control_socket(&ifr);
+	if (fd < 0)
+		exit(100);
+
+	n = get_feature_strings(fd, &ifr, &strings);
+
+	if (n < 0)
+		return n;
+
+	n_feature_strings = n;
+	feature_strings = calloc(n, sizeof(*feature_strings));
+	if (!feature_strings) {
+		fprintf(stderr, "no memory available for string table [size=%d]\n", n);
+		exit(95);
+	}
+
+	for (i = 0; i < n; ++i) {
+		if (!strings->data[i*ETH_GSTRING_LEN])
+			continue;
+
+		feature_strings[i] = strndup(
+			(const char *)&strings->data[i * ETH_GSTRING_LEN],
+			ETH_GSTRING_LEN);
+
+		if (!feature_strings[i]) {
+			fprintf(stderr, "no memory available for a string\n");
+			exit(95);
 		}
 	}
 
+	free(strings);
+	return n;
+}
+
+static void parse_sfeatures_args(int argc, char **argp, int argi)
+{
+	struct cmdline_info *cmdline_desc, *cp;
+	int sz_features, i;
+	int changed = 0;
+
+	if (init_feature_strings() < 0) {
+		/* ETHTOOL_GFEATURES unavailable */
+		parse_generic_cmdline(argc, argp, argi, &changed,
+			cmdline_offload, ARRAY_SIZE(cmdline_offload));
+		return;
+	}
+
+	sz_features = sizeof(*features_req->features) * ((n_feature_strings + 31) / 32);
+
+	cp = cmdline_desc = calloc(n_feature_strings + ARRAY_SIZE(cmdline_offload),
+		sizeof(*cmdline_desc));
+	memcpy(cp, cmdline_offload, sizeof(cmdline_offload));
+	cp += ARRAY_SIZE(cmdline_offload);
+
+	features_req = calloc(1, sizeof(*features_req) + sz_features);
+	if (!cmdline_desc || !features_req) {
+		fprintf(stderr, "no memory available\n");
+		exit(95);
+	}
+
+	features_req->size = (n_feature_strings + 31) / 32;
+
+	for (i = 0; i < n_feature_strings; ++i) {
+		if (!feature_strings[i])
+			continue;
+
+		cp->name = feature_strings[i];
+		cp->type = CMDL_FLAG;
+		cp->flag_val = 1 << (i % 32);
+		cp->wanted_val = &features_req->features[i / 32].requested;
+		cp->seen_val = &features_req->features[i / 32].valid;
+		++cp;
+	}
+
+	parse_generic_cmdline(argc, argp, argi, &changed,
+		cmdline_desc, cp - cmdline_desc);
+
+	free(cmdline_desc);
+
+	if (!changed) {
+		free(features_req);
+		features_req = NULL;
+	}
+}
+
+static int send_gfeatures(int fd, struct ifreq *ifr,
+	struct ethtool_gfeatures **features_p)
+{
+	struct ethtool_gfeatures *features;
+	int err, sz_features;
+
+	sz_features = sizeof(*features->features) * ((n_feature_strings + 31) / 32);
+	features = calloc(1, sz_features + sizeof(*features));
+	if (!features) {
+		fprintf(stderr, "no memory available\n");
+		return 95;
+	}
+
+	features->cmd = ETHTOOL_GFEATURES;
+	features->size = (n_feature_strings + 31) / 32;
+	ifr->ifr_data = (caddr_t) features;
+	err = send_ioctl(fd, ifr);
+
+	if (err < 0) {
+		perror("Cannot get feature status");
+		free(features);
+		return 97;
+	}
+
+	*features_p = features;
+	return 0;
+}
+
+static int do_gfeatures(int fd, struct ifreq *ifr)
+{
+	struct ethtool_gfeatures *features;
+	int err, i;
+
+	err = init_feature_strings();
+	if (err < 0)
+		return -err;
+
+	err = send_gfeatures(fd, ifr, &features);
+	if (err)
+		return err;
+
+	fprintf(stdout, "\nFull offload state:  (feature-name: active,wanted,changable)\n");
+	for (i = 0; i < n_feature_strings; i++) {
+		if (!feature_strings[i])
+			continue;	/* empty */
+#define P_FLAG(f) \
+	(features->features[i / 32].f & (1 << (i % 32))) ? "yes" : " no"
+#define PA_FLAG(f) \
+	(features->features[i / 32].available & (1 << (i % 32))) ? P_FLAG(f) : "---"
+#define PN_FLAG(f) \
+	(features->features[i / 32].never_changed & (1 << (i % 32))) ? "---" : P_FLAG(f)
+		fprintf(stdout, "     %-*.*s %s,%s,%s\n",
+			ETH_GSTRING_LEN, ETH_GSTRING_LEN, feature_strings[i],
+			P_FLAG(active), PA_FLAG(requested), PN_FLAG(available));
+#undef P_FLAG
+#undef PA_FLAG
+#undef PN_FLAG
+	}
+	free(features);
+
+	return 0;
+}
+
+static void print_gfeatures_diff(
+	const struct ethtool_get_features_block *expected,
+	const struct ethtool_get_features_block *set,
+	const char **strings, int n_strings)
+{
+	int i;
+
+	if (n_strings > 32)
+		n_strings = 32;
+
+	for (i = 0; i < n_strings; ++i) {
+		u32 mask = 1 << i;
+
+		if (!strings[i])
+			continue;
+
+		if (!((expected->active ^ set->active) & mask))
+			continue;
+
+		fprintf(stderr, "feature %.*s is %s (expected: %s, saved: %s)\n",
+			ETH_GSTRING_LEN, strings[i],
+			set->active & mask ? "enabled" : "disabled",
+			expected->active & mask ? "enabled" : "disabled",
+			!(set->available & mask) ? "not user-changeable" :
+				set->requested & mask ? "enabled" : "disabled"
+		);
+	}
+}
+
+static int get_offload_state(int fd, struct ifreq *ifr,
+	struct ethtool_gfeatures **features,
+	struct offload_state *offload)
+{
+	int err, allfail;
+
+	allfail = send_goffloads(fd, ifr, offload);
+
+	err = init_feature_strings();
+	if (err < 0)
+		return allfail ? err : 0;
+
+	err = send_gfeatures(fd, ifr, features);
+	if (err)
+		perror("Cannot read features");
+
+	return allfail ? -err : 0;
+}
+
+static int send_sfeatures(int fd, struct ifreq *ifr)
+{
+	int err;
+
+	features_req->cmd = ETHTOOL_SFEATURES;
+	ifr->ifr_data = (caddr_t) features_req;
+	err = send_ioctl(fd, ifr);
+	if (err < 0) {
+		perror("Cannot change features");
+		return 97;
+	}
+
+	return 0;
+}
+
+static void compare_offload_state(struct offload_state *offload0,
+	struct offload_state *offload1)
+{
+	int *expected = (int *)offload0, *new = (int *)offload1;
+	int i;
+
+	if (off_csum_rx_wanted >= 0)
+		offload0->rx = off_csum_rx_wanted;
+
+	if (off_csum_tx_wanted >= 0)
+		offload0->tx = off_csum_tx_wanted;
+
+	if (off_sg_wanted >= 0)
+		offload0->sg = off_sg_wanted;
+
+	if (off_tso_wanted >= 0)
+		offload0->tso = off_tso_wanted;
+
+	if (off_ufo_wanted >= 0)
+		offload0->ufo = off_ufo_wanted;
+
+	if (off_gso_wanted >= 0)
+		offload0->gso = off_gso_wanted;
+
+	if (off_gro_wanted >= 0)
+		offload0->gro = off_gro_wanted;
+
+	if (off_flags_mask & ETH_FLAG_LRO)
+		offload0->lro = !!(off_flags_wanted & ETH_FLAG_LRO);
+
+	if (off_flags_mask & ETH_FLAG_RXVLAN)
+		offload0->rxvlan = !!(off_flags_wanted & ETH_FLAG_RXVLAN);
+
+	if (off_flags_mask & ETH_FLAG_TXVLAN)
+		offload0->txvlan = !!(off_flags_wanted & ETH_FLAG_TXVLAN);
+
+	if (off_flags_mask & ETH_FLAG_NTUPLE)
+		offload0->ntuple = !!(off_flags_wanted & ETH_FLAG_NTUPLE);
+
+	if (off_flags_mask & ETH_FLAG_RXHASH)
+		offload0->rxhash = !!(off_flags_wanted & ETH_FLAG_RXHASH);
+
+	for (i = 0; i < ARRAY_SIZE(old_feature_names); i++) {
+		if (expected[i] == new[i])
+			continue;
+
+		fprintf(stderr, "feature group %s is %s (expected: %s)\n",
+			old_feature_names[i],
+			new[i] ? "enabled" : "disabled",
+			expected[i] ? "enabled" : "disabled"
+		);
+	}
+}
+
+static void compare_features(struct ethtool_gfeatures *features0,
+	struct ethtool_gfeatures *features1)
+{
+	int i;
+
+	/* make features0 .active what we expect to be set */
+	i = (n_feature_strings + 31) / 32;
+	while (i--) {
+		features0->features[i].active &= ~features_req->features[i].valid;
+		features0->features[i].active |=
+			features_req->features[i].requested &
+			features_req->features[i].valid;
+	}
+
+	for (i = 0; i < n_feature_strings; i += 32)
+		print_gfeatures_diff(&features0->features[i / 32],
+			&features1->features[i / 32],
+			feature_strings + i,
+			n_feature_strings - i);
+}
+
+static int do_soffload(int fd, struct ifreq *ifr)
+{
+	struct ethtool_gfeatures *features_old, *features_new;
+	struct offload_state offload_old, offload_new;
+	int err, changed;
+
+	err = get_offload_state(fd, ifr, &features_old, &offload_old);
+	if (err)
+		return -err;
+
+	changed = send_soffloads(fd, ifr);
+
+	if (features_req) {
+		err = send_sfeatures(fd, ifr);
+		if (!err)
+			changed = 1;
+	}
+
 	if (!changed) {
 		fprintf(stdout, "no offload settings changed\n");
+		return err;
+	}
+
+	err = get_offload_state(fd, ifr, &features_new, &offload_new);
+	if (err) {
+		perror("can't verify offload setting");
+		return 101;
+	}
+	if ((!features_old) ^ (!features_new)) {
+		fprintf(stderr, "can't compare features (one GFEATURES failed)\n");
+		features_old = NULL;
 	}
 
+	compare_offload_state(&offload_old, &offload_new);
+	if (features_old)
+		compare_features(features_old, features_new);
+
 	return 0;
 }
 

[RFC PATCH ethtool] ethtool: implement G/SFEATURES calls

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 ethtool.c |  299 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 293 insertions(+), 6 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 34fe107..86a5a8b 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -33,6 +33,7 @@
 #include <limits.h>
 #include <ctype.h>
 
+#include <unistd.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
@@ -83,6 +84,9 @@ static int do_gcoalesce(int fd, struct ifreq *ifr);
 static int do_scoalesce(int fd, struct ifreq *ifr);
 static int do_goffload(int fd, struct ifreq *ifr);
 static int do_soffload(int fd, struct ifreq *ifr);
+static void parse_sfeatures_args(int argc, char **argp, int argi);
+static int do_gfeatures(int fd, struct ifreq *ifr);
+static int do_sfeatures(int fd, struct ifreq *ifr);
 static int do_gstats(int fd, struct ifreq *ifr);
 static int rxflow_str_to_type(const char *str);
 static int parse_rxfhashopts(char *optstr, u32 *data);
@@ -119,6 +123,8 @@ static enum {
 	MODE_SRING,
 	MODE_GOFFLOAD,
 	MODE_SOFFLOAD,
+	MODE_GFEATURES,
+	MODE_SFEATURES,
 	MODE_GSTATS,
 	MODE_GNFC,
 	MODE_SNFC,
@@ -197,6 +203,10 @@ static struct option {
 		"		[ ntuple on|off ]\n"
 		"		[ rxhash on|off ]\n"
     },
+    { "-w", "--show-features", MODE_GFEATURES, "Get offload status" },
+    { "-W", "--request-features", MODE_SFEATURES, "Set requested offload",
+		"		[ feature-name on|off [...] ]\n"
+		"		see --show-features output for feature-name strings\n" },
     { "-i", "--driver", MODE_GDRV, "Show driver information" },
     { "-d", "--register-dump", MODE_GREGS, "Do a register dump",
 		"		[ raw on|off ]\n"
@@ -768,6 +778,8 @@ static void parse_cmdline(int argc, char **argp)
 			    (mode == MODE_SRING) ||
 			    (mode == MODE_GOFFLOAD) ||
 			    (mode == MODE_SOFFLOAD) ||
+			    (mode == MODE_GFEATURES) ||
+			    (mode == MODE_SFEATURES) ||
 			    (mode == MODE_GSTATS) ||
 			    (mode == MODE_GNFC) ||
 			    (mode == MODE_SNFC) ||
@@ -858,6 +870,11 @@ static void parse_cmdline(int argc, char **argp)
 				i = argc;
 				break;
 			}
+			if (mode == MODE_SFEATURES) {
+				parse_sfeatures_args(argc, argp, i);
+				i = argc;
+				break;
+			}
 			if (mode == MODE_SCLSRULE) {
 				if (!strcmp(argp[i], "flow-type")) {
 					i += 1;
@@ -1867,21 +1884,30 @@ static int dump_rxfhash(int fhash, u64 val)
 	return 0;
 }
 
-static int doit(void)
+static int get_control_socket(struct ifreq *ifr)
 {
-	struct ifreq ifr;
 	int fd;
 
 	/* Setup our control structures. */
-	memset(&ifr, 0, sizeof(ifr));
-	strcpy(ifr.ifr_name, devname);
+	memset(ifr, 0, sizeof(*ifr));
+	strcpy(ifr->ifr_name, devname);
 
 	/* Open control socket. */
 	fd = socket(AF_INET, SOCK_DGRAM, 0);
-	if (fd < 0) {
+	if (fd < 0)
 		perror("Cannot get control socket");
+
+	return fd;
+}
+
+static int doit(void)
+{
+	struct ifreq ifr;
+	int fd;
+
+	fd = get_control_socket(&ifr);
+	if (fd < 0)
 		return 70;
-	}
 
 	/* all of these are expected to populate ifr->ifr_data as needed */
 	if (mode == MODE_GDRV) {
@@ -1918,6 +1944,10 @@ static int doit(void)
 		return do_goffload(fd, &ifr);
 	} else if (mode == MODE_SOFFLOAD) {
 		return do_soffload(fd, &ifr);
+	} else if (mode == MODE_GFEATURES) {
+		return do_gfeatures(fd, &ifr);
+	} else if (mode == MODE_SFEATURES) {
+		return do_sfeatures(fd, &ifr);
 	} else if (mode == MODE_GSTATS) {
 		return do_gstats(fd, &ifr);
 	} else if (mode == MODE_GNFC) {
@@ -2355,6 +2385,263 @@ static int do_soffload(int fd, struct ifreq *ifr)
 	return 0;
 }
 
+static int get_feature_strings(int fd, struct ifreq *ifr,
+	struct ethtool_gstrings **strs)
+{
+	struct ethtool_sset_info *sset_info;
+	struct ethtool_gstrings *strings;
+	int sz_str, n_strings, err;
+
+	sset_info = malloc(sizeof(struct ethtool_sset_info) + sizeof(u32));
+	sset_info->cmd = ETHTOOL_GSSET_INFO;
+	sset_info->sset_mask = (1ULL << ETH_SS_FEATURES);
+	ifr->ifr_data = (caddr_t)sset_info;
+	err = send_ioctl(fd, ifr);
+
+	if ((err < 0) ||
+	    (!(sset_info->sset_mask & (1ULL << ETH_SS_FEATURES)))) {
+		perror("Cannot get driver strings info");
+		return -100;
+	}
+
+	n_strings = sset_info->data[0];
+	free(sset_info);
+	sz_str = n_strings * ETH_GSTRING_LEN;
+
+	strings = calloc(1, sz_str + sizeof(struct ethtool_gstrings));
+	if (!strings) {
+		fprintf(stderr, "no memory available\n");
+		return -95;
+	}
+
+	strings->cmd = ETHTOOL_GSTRINGS;
+	strings->string_set = ETH_SS_FEATURES;
+	strings->len = n_strings;
+	ifr->ifr_data = (caddr_t) strings;
+	err = send_ioctl(fd, ifr);
+	if (err < 0) {
+		perror("Cannot get feature strings information");
+		free(strings);
+		return -96;
+	}
+
+	*strs = strings;
+	return n_strings;
+}
+
+struct ethtool_sfeatures *features_req;
+
+static void parse_sfeatures_args(int argc, char **argp, int argi)
+{
+	struct cmdline_info *cmdline_desc, *cp;
+	struct ethtool_gstrings *strings;
+	struct ifreq ifr;
+	int n_strings, sz_features, i;
+	int fd, changed = 0;
+
+	fd = get_control_socket(&ifr);
+	if (fd < 0)
+		exit(100);
+
+	n_strings = get_feature_strings(fd, &ifr, &strings);
+	if (n_strings < 0)
+		exit(-n_strings);
+
+	sz_features = sizeof(*features_req->features) * ((n_strings + 31) / 32);
+
+	cp = cmdline_desc = calloc(n_strings, sizeof(*cmdline_desc));
+	features_req = calloc(1, sizeof(*features_req) + sz_features);
+	if (!cmdline_desc || !features_req) {
+		fprintf(stderr, "no memory available\n");
+		exit(95);
+	}
+
+	features_req->size = (n_strings + 31) / 32;
+
+	for (i = 0; i < n_strings; ++i) {
+		if (!strings->data[i*ETH_GSTRING_LEN])
+			continue;
+
+		strings->data[i*ETH_GSTRING_LEN + ETH_GSTRING_LEN-1] = 0;
+		cp->name = (const char *)strings->data + i * ETH_GSTRING_LEN;
+		cp->type = CMDL_FLAG;
+		cp->flag_val = 1 << (i % 32);
+		cp->wanted_val = &features_req->features[i / 32].requested;
+		cp->seen_val = &features_req->features[i / 32].valid;
+		++cp;
+	}
+
+	parse_generic_cmdline(argc, argp, argi, &changed,
+		cmdline_desc, cp - cmdline_desc);
+
+	free(cmdline_desc);
+	free(strings);
+	close(fd);
+
+	if (!changed) {
+		free(features_req);
+		features_req = NULL;
+	}
+}
+
+static int send_gfeatures(int fd, struct ifreq *ifr, int n_strings,
+	struct ethtool_gfeatures **features_p)
+{
+	struct ethtool_gfeatures *features;
+	int err, sz_features;
+
+	sz_features = sizeof(*features->features) * ((n_strings + 31) / 32);
+	features = calloc(1, sz_features + sizeof(*features));
+	if (!features) {
+		fprintf(stderr, "no memory available\n");
+		return 95;
+	}
+
+	features->cmd = ETHTOOL_GFEATURES;
+	features->size = (n_strings + 31) / 32;
+	ifr->ifr_data = (caddr_t) features;
+	err = send_ioctl(fd, ifr);
+
+	if (err < 0) {
+		perror("Cannot get feature status");
+		free(features);
+		return 97;
+	}
+
+	*features_p = features;
+	return 0;
+}
+
+static int do_gfeatures(int fd, struct ifreq *ifr)
+{
+	struct ethtool_gstrings *strings;
+	struct ethtool_gfeatures *features;
+	int n_strings, err, i;
+
+	n_strings = get_feature_strings(fd, ifr, &strings);
+	if (n_strings < 0)
+		return -n_strings;
+
+	err = send_gfeatures(fd, ifr, n_strings, &features);
+	if (err) {
+		free(strings);
+		return err;
+	}
+
+	fprintf(stdout, "Offload state:  (name: enabled,wanted,changable)\n");
+	for (i = 0; i < n_strings; i++) {
+		if (!strings->data[i * ETH_GSTRING_LEN])
+			continue;	/* empty */
+#define P_FLAG(f) \
+	(features->features[i / 32].f & (1 << (i % 32))) ? "yes" : " no"
+#define PA_FLAG(f) \
+	(features->features[i / 32].available & (1 << (i % 32))) ? P_FLAG(f) : "---"
+#define PN_FLAG(f) \
+	(features->features[i / 32].never_changed & (1 << (i % 32))) ? "---" : P_FLAG(f)
+		fprintf(stdout, "     %-*.*s %s,%s,%s\n",
+			ETH_GSTRING_LEN, ETH_GSTRING_LEN,
+			&strings->data[i * ETH_GSTRING_LEN],
+			P_FLAG(active), PA_FLAG(requested), PN_FLAG(available));
+#undef P_FLAG
+#undef PA_FLAG
+#undef PN_FLAG
+	}
+	free(strings);
+	free(features);
+
+	return 0;
+}
+
+static void print_gfeatures_diff(
+	const struct ethtool_get_features_block *expected,
+	const struct ethtool_get_features_block *set,
+	const char *strings, int n_strings)
+{
+	int i;
+
+	if (n_strings > 32)
+		n_strings = 32;
+
+	for (i = 0; i < n_strings; ++i) {
+		const char *name = &strings[i * ETH_GSTRING_LEN];
+		u32 mask = 1 << i;
+
+		if (!((expected->active ^ set->active) & mask))
+			continue;
+
+		fprintf(stderr, "feature %.*s is %s (expected: %s, saved: %s)\n",
+			ETH_GSTRING_LEN, name,
+			set->active & mask ? "enabled" : "disabled",
+			expected->active & mask ? "enabled" : "disabled",
+			!(set->available & mask) ? "not user-changeable" :
+				set->requested & mask ? "enabled" : "disabled"
+		);
+	}
+}
+
+static int do_sfeatures(int fd, struct ifreq *ifr)
+{
+	struct ethtool_gstrings *strings;
+	struct ethtool_gfeatures *features0, *features1;
+	int n_strings, err, i;
+
+	if (!features_req) {
+		fprintf(stderr, "no features changed\n");
+		return 97;
+	}
+
+	n_strings = get_feature_strings(fd, ifr, &strings);
+	if (n_strings < 0) {
+		free(features_req);
+		return -n_strings;
+	}
+
+	err = send_gfeatures(fd, ifr, n_strings, &features0);
+	if (err) {
+		perror("Cannot read features");
+		goto free_strings;
+	}
+
+	features_req->cmd = ETHTOOL_SFEATURES;
+	ifr->ifr_data = (caddr_t) features_req;
+	err = send_ioctl(fd, ifr);
+	if (err < 0) {
+		perror("Cannot change features");
+		err = 97;
+		goto free_features;
+	}
+
+	err = send_gfeatures(fd, ifr, n_strings, &features1);
+	if (err) {
+		perror("Cannot verify features");
+		goto free_features;
+	}
+
+	/* make features0 .active what we expect to be set */
+	i = (n_strings + 31) / 32;
+	while (i--) {
+		features0->features[i].active &= ~features_req->features[i].valid;
+		features0->features[i].active |=
+			features_req->features[i].requested &
+			features_req->features[i].valid;
+	}
+
+	for (i = 0; i < n_strings; i += 32)
+		print_gfeatures_diff(&features0->features[i / 32],
+			&features1->features[i / 32],
+			(char *)&strings->data[i * ETH_GSTRING_LEN],
+			n_strings - i);
+
+	free(features1);
+free_features:
+	free(features0);
+free_strings:
+	free(strings);
+	free(features_req);
+	return err;
+}
+
+
 static int do_gset(int fd, struct ifreq *ifr)
 {
 	int err;
-- 
1.7.2.5

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

[David Miller]

You guys really need to sort this out properly.

Please resubmit whatever final solution is agreed upon.

Thanks.

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

[Michał Mirosław]

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.

Ben, do you agree?

Best Regards,
Michał Mirosław

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

[Michał Mirosław]

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.
> 
> Ben, do you agree?

Ping?

http://patchwork.ozlabs.org/patch/95552/
(this is a bugfix, so should go to stable)

http://patchwork.ozlabs.org/patch/95753/
(removes ETHTOOL_F_COMPAT; this we need to decide on)

Best Regards,
Michał Mirosław

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

[David Miller]

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.
>> 
>> Ben, do you agree?
> 
> Ping?
> 
> http://patchwork.ozlabs.org/patch/95552/
> (this is a bugfix, so should go to stable)
> 
> http://patchwork.ozlabs.org/patch/95753/
> (removes ETHTOOL_F_COMPAT; this we need to decide on)

You and Ben are still arguing over details.

I want fresh versions of these patches (yes, both of them) once
all of the issues are resolved.

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

[Michał Mirosław]

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.
> >> 
> >> Ben, do you agree?
> > Ping?
> > http://patchwork.ozlabs.org/patch/95552/
> > (this is a bugfix, so should go to stable)
> > 
> > http://patchwork.ozlabs.org/patch/95753/
> > (removes ETHTOOL_F_COMPAT; this we need to decide on)
> You and Ben are still arguing over details.
> 
> I want fresh versions of these patches (yes, both of them) once
> all of the issues are resolved.

We could just wait for 2.6.40 and pretend this code part never existed. ;-)

I'll rebase the first patch tomorrow. Without it the compatibility in
ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.

Best Regards,
Michał Mirosław

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

[Ben Hutchings]

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.
> > >> 
> > >> Ben, do you agree?
> > > Ping?
> > > http://patchwork.ozlabs.org/patch/95552/
> > > (this is a bugfix, so should go to stable)
> > > 
> > > http://patchwork.ozlabs.org/patch/95753/
> > > (removes ETHTOOL_F_COMPAT; this we need to decide on)
> > You and Ben are still arguing over details.
> > 
> > I want fresh versions of these patches (yes, both of them) once
> > all of the issues are resolved.
> 
> 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.

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.

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

[Michał Mirosław]

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 (note: ETHTOOL_S{SG,...} are not ever going away)
   - causes NETIF_F_* to be an ABI
   - does not support new features

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

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

Best Regards,
Michał Mirosław

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

[Ben Hutchings]

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.

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

[Michał Mirosław]

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

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.

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

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

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

[Ben Hutchings]

On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> 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:
[...]
> > >      (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?

xen-api <https://github.com/xen-org/xen-api> uses it in
scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
used for a particularly good reason...

> [...]
> > > 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?

So it does; for some reason I didn't realise that.  Then I suppose there
isn't a benefit from using ETHTOOL_SFEATURES for existing feature flags.

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

That was already true.

> This means that
> userspace needs to reread features after any SFEATURES call, not just
> those returning non-zero.

Not necessarily, though of course it is helpful to report consequential
feature changes.

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.

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

[Michał Mirosław]

On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
> On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> > 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:
> [...]
> > > >      (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?
> xen-api <https://github.com/xen-org/xen-api> uses it in
> scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
> used for a particularly good reason...

Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().

Best Regards,
Michał Mirosław

[added Cc: xen-devel]

Re: [Xen-devel] Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

[Ian Campbell]

On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> > > 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:
> > [...]
> > > > >      (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?
> > xen-api <https://github.com/xen-org/xen-api> uses it in
> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
> > used for a particularly good reason...
> 
> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().
> 
> Best Regards,
> Michał Mirosław
> 
> [added Cc: xen-devel]

added Cc: xen-api list and dev@openvswitch as well.

Complete thread is at
http://thread.gmane.org/gmane.linux.network/195552/focus=197019

Ian.


_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [Xen-devel] Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

[Jesse Gross]

2011/5/28 Ian Campbell <Ian.Campbell@citrix.com>:
> On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
>> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
>> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
>> > > 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:
>> > [...]
>> > > > >      (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?
>> > xen-api <https://github.com/xen-org/xen-api> uses it in
>> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
>> > used for a particularly good reason...
>>
>> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().

ETHTOOL_GFLAGS didn't expose the vlan acceleration flags until 2.6.37,
which is why /sys/class/net was used instead.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [Xen-devel] Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

[Michał Mirosław]

On Sat, May 28, 2011 at 10:31:03AM -0700, Jesse Gross wrote:
> 2011/5/28 Ian Campbell <Ian.Campbell-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>:
> > On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
> >> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
> >> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> >> > > 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:
> >> > [...]
> >> > > > >      (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?
> >> > xen-api <https://github.com/xen-org/xen-api> uses it in
> >> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
> >> > used for a particularly good reason...
> >> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().
> ETHTOOL_GFLAGS didn't expose the vlan acceleration flags until 2.6.37,
> which is why /sys/class/net was used instead.

https://github.com/xen-org/xen-api/commit/78b8078e6ae3cf48179859bed6350bb326987546

The commit using it was introduced after 2.6.37 kernel was released and uses
undocumented acccess path to the bits in question. What is the kernel patch
this commit is referring to?

Best Regards,
Michał Mirosław

Re: [Xen-devel] Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

[Jesse Gross]

2011/5/29 Michał Mirosław <mirq-linux@rere.qmqm.pl>:
> On Sat, May 28, 2011 at 10:31:03AM -0700, Jesse Gross wrote:
>> 2011/5/28 Ian Campbell <Ian.Campbell@citrix.com>:
>> > On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
>> >> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
>> >> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
>> >> > > 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:
>> >> > [...]
>> >> > > > >      (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?
>> >> > xen-api <https://github.com/xen-org/xen-api> uses it in
>> >> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
>> >> > used for a particularly good reason...
>> >> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().
>> ETHTOOL_GFLAGS didn't expose the vlan acceleration flags until 2.6.37,
>> which is why /sys/class/net was used instead.
>
> https://github.com/xen-org/xen-api/commit/78b8078e6ae3cf48179859bed6350bb326987546
>
> The commit using it was introduced after 2.6.37 kernel was released

Well people do use kernels other than the most recently released one...

> and uses
> undocumented acccess path to the bits in question. What is the kernel patch
> this commit is referring to?

It's a temporary workaround to deal with the fact that many drivers
that support vlan acceleration do not properly handle vlan packets if
the corresponding group isn't configured on them.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[RESEND PATCH net] net: fix ETHTOOL_SFEATURES compatibility with old ethtool_ops.set_flags

[Michał Mirosław]

Current code squashes flags to bool - this makes set_flags fail whenever
some ETH_FLAG_* equivalent features are set. Fix this.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/ethtool.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 84e7304..fd14116 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -233,6 +233,29 @@ static int ethtool_set_feature_compat(struct net_device *dev,
 	return 1;
 }
 
+static int ethtool_set_flags_compat(struct net_device *dev,
+	int (*legacy_set)(struct net_device *, u32),
+	struct ethtool_set_features_block *features, u32 mask)
+{
+	u32 value;
+
+	if (!legacy_set)
+		return 0;
+
+	if (!(features[0].valid & mask))
+		return 0;
+
+	value = dev->features & ~features[0].valid;
+	value |= features[0].requested;
+
+	features[0].valid &= ~mask;
+
+	if (legacy_set(dev, value & mask) < 0)
+		netdev_info(dev, "Legacy flags change failed\n");
+
+	return 1;
+}
+
 static int ethtool_set_features_compat(struct net_device *dev,
 	struct ethtool_set_features_block *features)
 {
@@ -249,7 +272,7 @@ static int ethtool_set_features_compat(struct net_device *dev,
 		features, NETIF_F_ALL_TSO);
 	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_rx_csum,
 		features, NETIF_F_RXCSUM);
-	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_flags,
+	compat |= ethtool_set_flags_compat(dev, dev->ethtool_ops->set_flags,
 		features, flags_dup_features);
 
 	return compat;
-- 
1.7.2.5

Re: [RESEND PATCH net] net: fix ETHTOOL_SFEATURES compatibility with old ethtool_ops.set_flags

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 26 May 2011 12:42:57 +0200 (CEST)

> Current code squashes flags to bool - this makes set_flags fail whenever
> some ETH_FLAG_* equivalent features are set. Fix this.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.

[PATCH v2] net: fix ETHTOOL_SFEATURES compatibility with old ethtool_ops.set_flags

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: fix 'valid' bits clearing before use

 net/core/ethtool.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b8c2bcf..d1c8b64 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -233,6 +233,29 @@ static int ethtool_set_feature_compat(struct net_device *dev,
 	return 1;
 }
 
+static int ethtool_set_flags_compat(struct net_device *dev,
+	int (*legacy_set)(struct net_device *, u32),
+	struct ethtool_set_features_block *features, u32 mask)
+{
+	u32 value;
+
+	if (!legacy_set)
+		return 0;
+
+	if (!(features[0].valid & mask))
+		return 0;
+
+	value = dev->features & ~features[0].valid;
+	value |= features[0].requested;
+
+	features[0].valid &= ~mask;
+
+	if (legacy_set(dev, value & mask) < 0)
+		netdev_info(dev, "Legacy flags change failed\n");
+
+	return 1;
+}
+
 static int ethtool_set_features_compat(struct net_device *dev,
 	struct ethtool_set_features_block *features)
 {
@@ -249,7 +272,7 @@ static int ethtool_set_features_compat(struct net_device *dev,
 		features, NETIF_F_ALL_TSO);
 	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_rx_csum,
 		features, NETIF_F_RXCSUM);
-	compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_flags,
+	compat |= ethtool_set_flags_compat(dev, dev->ethtool_ops->set_flags,
 		features, flags_dup_features);
 
 	return compat;
-- 
1.7.2.5