[PATCH] net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

[PATCH] net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

[Roger Luethi]

register_netdevice() removes NETIF_F_GSO from dev->features and
dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from
netdev_fix_features()".

However, as the flag remains set in dev->hw_features, users trying to
enable GSO via ethtool will still end up with NETIF_F_GSO stuck in
wanted_features, resulting in a warning every time they try to change any
feature (until they use ethtool to "disable" GSO):

# ethtool -K eth2 gso on
via-rhine 0000:03:00.0: eth2: Dropping NETIF_F_GSO since no SG feature.
# ethtool -K eth2 rx on
via-rhine 0000:03:00.0: eth2: Dropping NETIF_F_GSO since no SG feature.
via-rhine 0000:03:00.0: eth2: Features changed: 0x00004380 -> 0x20004380
# ethtool -K eth2 rx off
via-rhine 0000:03:00.0: eth2: Dropping NETIF_F_GSO since no SG feature.
via-rhine 0000:03:00.0: eth2: Features changed: 0x20004380 -> 0x00004380
# ethtool -K eth2 gso off
#

With NETIF_F_GSO depending on NETIF_F_SG, it seems we should not
advertise the former as a changeable option if the latter is missing.
---
 net/core/dev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 0d39032..122d4ca 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5433,6 +5433,7 @@ int register_netdevice(struct net_device *dev)
 
 	/* Avoid warning from netdev_fix_features() for GSO without SG */
 	if (!(dev->wanted_features & NETIF_F_SG)) {
+		dev->hw_features &= ~NETIF_F_GSO;
 		dev->wanted_features &= ~NETIF_F_GSO;
 		dev->features &= ~NETIF_F_GSO;
 	}
-- 
1.7.3.4

Re: [PATCH] net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

[Michał Mirosław]

2011/3/18 Roger Luethi <rl@hellgate.ch>:
> register_netdevice() removes NETIF_F_GSO from dev->features and
> dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from
> netdev_fix_features()".
>
> However, as the flag remains set in dev->hw_features, users trying to
> enable GSO via ethtool will still end up with NETIF_F_GSO stuck in
> wanted_features, resulting in a warning every time they try to change any
> feature (until they use ethtool to "disable" GSO):

That's expected, as it allows the user to debug why his request didn't
work as expected. I think that those messages should get demoted to
DEBUG if that's too noisy. The network core imposed conditions could
be explained in Documentation/ or ethtool manpage instead.

Best Regards,
Michał Mirosław

Re: [PATCH] net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

[Roger Luethi]

On Fri, 18 Mar 2011 21:43:23 +0100, Micha? Miros?aw wrote:
> 2011/3/18 Roger Luethi <rl@hellgate.ch>:
> > register_netdevice() removes NETIF_F_GSO from dev->features and
> > dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from
> > netdev_fix_features()".
> >
> > However, as the flag remains set in dev->hw_features, users trying to
> > enable GSO via ethtool will still end up with NETIF_F_GSO stuck in
> > wanted_features, resulting in a warning every time they try to change any
> > feature (until they use ethtool to "disable" GSO):
> 
> That's expected, as it allows the user to debug why his request didn't
> work as expected. I think that those messages should get demoted to
> DEBUG if that's too noisy. The network core imposed conditions could
> be explained in Documentation/ or ethtool manpage instead.

I agree that it is good to have a note explaining what happened in the
kernel log, and the patch I posted does not do that; that is easy to fix.

However, I still think the current, unpatched behavior is confusing users.
ethtool_get_features() will happily tell the user that GSO is available for
a NIC even if it is not (because the NIC does not support SG). Then, if the
user tries to enable GSO, the operation fails silently: ethtool won't get
an error code, so it looks as if the operation succeeded. The user needs to
look into the kernel log or actually verify the offload settings to realize
that something went wrong.

Roger

Re: [PATCH] net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

[Michał Mirosław]

2011/3/19 Roger Luethi <rl@hellgate.ch>:
> On Fri, 18 Mar 2011 21:43:23 +0100, Micha? Miros?aw wrote:
>> 2011/3/18 Roger Luethi <rl@hellgate.ch>:
>> > register_netdevice() removes NETIF_F_GSO from dev->features and
>> > dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from
>> > netdev_fix_features()".
>> >
>> > However, as the flag remains set in dev->hw_features, users trying to
>> > enable GSO via ethtool will still end up with NETIF_F_GSO stuck in
>> > wanted_features, resulting in a warning every time they try to change any
>> > feature (until they use ethtool to "disable" GSO):
>>
>> That's expected, as it allows the user to debug why his request didn't
>> work as expected. I think that those messages should get demoted to
>> DEBUG if that's too noisy. The network core imposed conditions could
>> be explained in Documentation/ or ethtool manpage instead.
>
> I agree that it is good to have a note explaining what happened in the
> kernel log, and the patch I posted does not do that; that is easy to fix.
>
> However, I still think the current, unpatched behavior is confusing users.
> ethtool_get_features() will happily tell the user that GSO is available for
> a NIC even if it is not (because the NIC does not support SG). Then, if the
> user tries to enable GSO, the operation fails silently: ethtool won't get
> an error code, so it looks as if the operation succeeded. The user needs to
> look into the kernel log or actually verify the offload settings to realize
> that something went wrong.

Actually, the user will get the information that GSO wasn't enabled,
though not while using ETHTOOL_SGSO.

OTOH, I think I can see what's your point - I got confused by your
proposed patch. The correct way is to remove NETIF_F_GSO from
hw_features when NETIF_F_SG is not present in hw_features (and not
wanted_features as you're patch is doing). The idea here is that a
driver might advertise SG in changeable features (that's hw_features)
but not enable it by default for whatever reason.

Do you want to prepare a patch for this? Please remember about
compatibility with per-offload ethtool ops if you do - this needs to
be there until all drivers are converted to hw_features.

Best Regards,
Michał Mirosław

Re: [PATCH] net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

[Roger Luethi]

On Sat, 19 Mar 2011 17:18:17 +0100, Micha? Miros?aw wrote:
> > However, I still think the current, unpatched behavior is confusing users.
> > ethtool_get_features() will happily tell the user that GSO is available for
> > a NIC even if it is not (because the NIC does not support SG). Then, if the
> > user tries to enable GSO, the operation fails silently: ethtool won't get
> > an error code, so it looks as if the operation succeeded. The user needs to
> > look into the kernel log or actually verify the offload settings to realize
> > that something went wrong.
> 
> Do you want to prepare a patch for this? Please remember about
> compatibility with per-offload ethtool ops if you do - this needs to
> be there until all drivers are converted to hw_features.

Something like this?

diff --git a/net/core/dev.c b/net/core/dev.c
index 0b88eba..5861a59 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5442,6 +5442,9 @@ int register_netdevice(struct net_device *dev)
 	if (!(dev->wanted_features & NETIF_F_SG)) {
 		dev->wanted_features &= ~NETIF_F_GSO;
 		dev->features &= ~NETIF_F_GSO;
+		if (!(dev->hw_features & NETIF_F_SG) &&
+		    (!dev->ethtool_ops || !dev->ethtool_ops->set_sg))
+			dev->hw_features &= ~NETIF_F_GSO;
 	}
 
 	/* Enable GRO and NETIF_F_HIGHDMA for vlans by default,