LRO disable warnings on kernel 2.6.38

LRO disable warnings on kernel 2.6.38

[Jesper Dangaard Brouer]

Hi

I'm seeing the LRO disable warnings using kernel 2.6.38:

[    8.664759] NET: Registered protocol family 10
[    8.838148] ADDRCONF(NETDEV_UP): eth71: link is not ready
[    8.872639] ------------[ cut here ]------------
[    8.872645] WARNING: at net/core/dev.c:1363 dev_disable_lro+0x7b/0x80()
[    8.872647] Hardware name: ProLiant DL370 G6
[    8.872648] Modules linked in: ipv6 nf_conntrack ip_tables loop i7core_edac edac_core ipmi_si ipmi_msghandler joydev hpilo pcspkr sg hpsa igb ata_piix netxen_nic dca [last unloaded: scsi_wait_scan]
[    8.872660] Pid: 2221, comm: sysctl Not tainted 2.6.38-comx04 #2
[    8.872662] Call Trace:
[    8.872671]  [<ffffffff81056e1f>] ? warn_slowpath_common+0x7f/0xc0
[    8.872675]  [<ffffffff81056e7a>] ? warn_slowpath_null+0x1a/0x20
[    8.872680]  [<ffffffff8140c0ab>] ? dev_disable_lro+0x7b/0x80
[    8.872686]  [<ffffffff81474f27>] ? devinet_sysctl_forward+0x147/0x180
[    8.872691]  [<ffffffff811872f7>] ? proc_sys_call_handler+0x97/0xd0
[    8.872700]  [<ffffffff81187344>] ? proc_sys_write+0x14/0x20
[    8.872704]  [<ffffffff81124148>] ? vfs_write+0xc8/0x180
[    8.872707]  [<ffffffff81124301>] ? sys_write+0x51/0x90
[    8.872712]  [<ffffffff8100b8c2>] ? system_call_fastpath+0x16/0x1b
[    8.872714] ---[ end trace 6245283cb8d484cc ]---

The strange part is that I didn't see this warning on my testlab and
pre-prod servers.  The warning is from the first production server,
which got kernel 2.6.38 deployed this morning.

The NIC driver is igb.

The only difference in hardware between the production and
pre-production server (which didn't show the warning), is the
prod-server have an extra dual-port original Intel NIC, dev-named
"eth71".  And its just after the init of eth71, the warning occurs.

We usually use a 6 port NIC from Hotlava, which is based on the same
chip 82576 and also uses the same igb driver.

Intel orig NIC eth71
 albpd4:~# ethtool -i eth71
 driver: igb
 version: 2.1.0-k2
 firmware-version: 1.2-1
 bus-info: 0000:21:00.0

Hotlava Intel chip based NIC eth51:
 albpd4:~# ethtool -i eth51
 driver: igb
 version: 2.1.0-k2
 firmware-version: 1.2-1
 bus-info: 0000:1d:00.1

I don't understand why I don't see the warning on my pre-prod server,
which only have the Hotlava NIC?!?

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network Kernel Developer
  Cand. Scient Datalog / MSc.CS
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: LRO disable warnings on kernel 2.6.38

[Eric Dumazet]

Le vendredi 18 mars 2011 à 12:12 +0100, Jesper Dangaard Brouer a écrit :
> Hi
> 
> I'm seeing the LRO disable warnings using kernel 2.6.38:
> 
> [    8.664759] NET: Registered protocol family 10
> [    8.838148] ADDRCONF(NETDEV_UP): eth71: link is not ready
> [    8.872639] ------------[ cut here ]------------
> [    8.872645] WARNING: at net/core/dev.c:1363 dev_disable_lro+0x7b/0x80()
> [    8.872647] Hardware name: ProLiant DL370 G6
> [    8.872648] Modules linked in: ipv6 nf_conntrack ip_tables loop i7core_edac edac_core ipmi_si ipmi_msghandler joydev hpilo pcspkr sg hpsa igb ata_piix netxen_nic dca [last unloaded: scsi_wait_scan]
> [    8.872660] Pid: 2221, comm: sysctl Not tainted 2.6.38-comx04 #2
> [    8.872662] Call Trace:
> [    8.872671]  [<ffffffff81056e1f>] ? warn_slowpath_common+0x7f/0xc0
> [    8.872675]  [<ffffffff81056e7a>] ? warn_slowpath_null+0x1a/0x20
> [    8.872680]  [<ffffffff8140c0ab>] ? dev_disable_lro+0x7b/0x80
> [    8.872686]  [<ffffffff81474f27>] ? devinet_sysctl_forward+0x147/0x180
> [    8.872691]  [<ffffffff811872f7>] ? proc_sys_call_handler+0x97/0xd0
> [    8.872700]  [<ffffffff81187344>] ? proc_sys_write+0x14/0x20
> [    8.872704]  [<ffffffff81124148>] ? vfs_write+0xc8/0x180
> [    8.872707]  [<ffffffff81124301>] ? sys_write+0x51/0x90
> [    8.872712]  [<ffffffff8100b8c2>] ? system_call_fastpath+0x16/0x1b
> [    8.872714] ---[ end trace 6245283cb8d484cc ]---
> 
> The strange part is that I didn't see this warning on my testlab and
> pre-prod servers.  The warning is from the first production server,
> which got kernel 2.6.38 deployed this morning.
> 
> The NIC driver is igb.
> 
> The only difference in hardware between the production and
> pre-production server (which didn't show the warning), is the
> prod-server have an extra dual-port original Intel NIC, dev-named
> "eth71".  And its just after the init of eth71, the warning occurs.
> 
> We usually use a 6 port NIC from Hotlava, which is based on the same
> chip 82576 and also uses the same igb driver.
> 
> Intel orig NIC eth71
>  albpd4:~# ethtool -i eth71
>  driver: igb
>  version: 2.1.0-k2
>  firmware-version: 1.2-1
>  bus-info: 0000:21:00.0
> 
> Hotlava Intel chip based NIC eth51:
>  albpd4:~# ethtool -i eth51
>  driver: igb
>  version: 2.1.0-k2
>  firmware-version: 1.2-1
>  bus-info: 0000:1d:00.1
> 
> I don't understand why I don't see the warning on my pre-prod server,
> which only have the Hotlava NIC?!?
> 

Hmm, WARN_ON() message is not very nice in this case I'm afraid, we dont
even know offender

diff --git a/net/core/dev.c b/net/core/dev.c
index 0b88eba..571ab70 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1361,7 +1361,8 @@ void dev_disable_lro(struct net_device *dev)
 			dev->ethtool_ops->set_flags(dev, flags);
 		}
 	}
-	WARN_ON(dev->features & NETIF_F_LRO);
+	if (dev->features & NETIF_F_LRO)
+		netdev_err(dev, "Could not disable LRO\n");
 }
 EXPORT_SYMBOL(dev_disable_lro);
 

Re: LRO disable warnings on kernel 2.6.38

[Ben Hutchings]

On Fri, 2011-03-18 at 14:05 +0100, Eric Dumazet wrote:
> Le vendredi 18 mars 2011 à 12:12 +0100, Jesper Dangaard Brouer a écrit :
> > Hi
> > 
> > I'm seeing the LRO disable warnings using kernel 2.6.38:
> > 
> > [    8.664759] NET: Registered protocol family 10
> > [    8.838148] ADDRCONF(NETDEV_UP): eth71: link is not ready
> > [    8.872639] ------------[ cut here ]------------
> > [    8.872645] WARNING: at net/core/dev.c:1363 dev_disable_lro+0x7b/0x80()
> > [    8.872647] Hardware name: ProLiant DL370 G6
> > [    8.872648] Modules linked in: ipv6 nf_conntrack ip_tables loop i7core_edac edac_core ipmi_si ipmi_msghandler joydev hpilo pcspkr sg hpsa igb ata_piix netxen_nic dca [last unloaded: scsi_wait_scan]
> > [    8.872660] Pid: 2221, comm: sysctl Not tainted 2.6.38-comx04 #2
> > [    8.872662] Call Trace:
> > [    8.872671]  [<ffffffff81056e1f>] ? warn_slowpath_common+0x7f/0xc0
> > [    8.872675]  [<ffffffff81056e7a>] ? warn_slowpath_null+0x1a/0x20
> > [    8.872680]  [<ffffffff8140c0ab>] ? dev_disable_lro+0x7b/0x80
> > [    8.872686]  [<ffffffff81474f27>] ? devinet_sysctl_forward+0x147/0x180
> > [    8.872691]  [<ffffffff811872f7>] ? proc_sys_call_handler+0x97/0xd0
> > [    8.872700]  [<ffffffff81187344>] ? proc_sys_write+0x14/0x20
> > [    8.872704]  [<ffffffff81124148>] ? vfs_write+0xc8/0x180
> > [    8.872707]  [<ffffffff81124301>] ? sys_write+0x51/0x90
> > [    8.872712]  [<ffffffff8100b8c2>] ? system_call_fastpath+0x16/0x1b
> > [    8.872714] ---[ end trace 6245283cb8d484cc ]---
> > 
> > The strange part is that I didn't see this warning on my testlab and
> > pre-prod servers.  The warning is from the first production server,
> > which got kernel 2.6.38 deployed this morning.
> > 
> > The NIC driver is igb.
> > 
> > The only difference in hardware between the production and
> > pre-production server (which didn't show the warning), is the
> > prod-server have an extra dual-port original Intel NIC, dev-named
> > "eth71".  And its just after the init of eth71, the warning occurs.
> > 
> > We usually use a 6 port NIC from Hotlava, which is based on the same
> > chip 82576 and also uses the same igb driver.
> > 
> > Intel orig NIC eth71
> >  albpd4:~# ethtool -i eth71
> >  driver: igb
> >  version: 2.1.0-k2
> >  firmware-version: 1.2-1
> >  bus-info: 0000:21:00.0
> > 
> > Hotlava Intel chip based NIC eth51:
> >  albpd4:~# ethtool -i eth51
> >  driver: igb
> >  version: 2.1.0-k2
> >  firmware-version: 1.2-1
> >  bus-info: 0000:1d:00.1
> > 
> > I don't understand why I don't see the warning on my pre-prod server,
> > which only have the Hotlava NIC?!?
> > 
> 
> Hmm, WARN_ON() message is not very nice in this case I'm afraid, we dont
> even know offender

WARN is correct as this is a driver bug.  But I agree that the
device/driver ID should be included.

Ben.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0b88eba..571ab70 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1361,7 +1361,8 @@ void dev_disable_lro(struct net_device *dev)
>  			dev->ethtool_ops->set_flags(dev, flags);
>  		}
>  	}
> -	WARN_ON(dev->features & NETIF_F_LRO);
> +	if (dev->features & NETIF_F_LRO)
> +		netdev_err(dev, "Could not disable LRO\n");
>  }
>  EXPORT_SYMBOL(dev_disable_lro);
>  
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: LRO disable warnings on kernel 2.6.38

[Eric Dumazet]

Le vendredi 18 mars 2011 à 14:17 +0000, Ben Hutchings a écrit :

> WARN is correct as this is a driver bug.  But I agree that the
> device/driver ID should be included.

stack trace gives absolutely no useful indication here.

Bug is in driver, yet we dump information on core network stack ?

pr_err() is an error indication, not a warning by the way ;)

Re: LRO disable warnings on kernel 2.6.38

[Stephen Hemminger]

On Fri, 18 Mar 2011 15:33:04 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le vendredi 18 mars 2011 à 14:17 +0000, Ben Hutchings a écrit :
> 
> > WARN is correct as this is a driver bug.  But I agree that the
> > device/driver ID should be included.
> 
> stack trace gives absolutely no useful indication here.
> 
> Bug is in driver, yet we dump information on core network stack ?
> 
> pr_err() is an error indication, not a warning by the way ;)

The advantage of WARN is that it doesn't get ignored and shows
up in kernel oops. But agreed it should print out as much device
info as possible to finger the broken device driver.

Re: LRO disable warnings on kernel 2.6.38

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Fri, 18 Mar 2011 08:15:24 -0700

> On Fri, 18 Mar 2011 15:33:04 +0100
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> Le vendredi 18 mars 2011 à 14:17 +0000, Ben Hutchings a écrit :
>> 
>> > WARN is correct as this is a driver bug.  But I agree that the
>> > device/driver ID should be included.
>> 
>> stack trace gives absolutely no useful indication here.
>> 
>> Bug is in driver, yet we dump information on core network stack ?
>> 
>> pr_err() is an error indication, not a warning by the way ;)
> 
> The advantage of WARN is that it doesn't get ignored and shows
> up in kernel oops. But agreed it should print out as much device
> info as possible to finger the broken device driver.

Infrastructure is not static, therefore we could add a WARN_ON_NETDEV()
or similar.  An in fact such things would probably be very useful.

Re: LRO disable warnings on kernel 2.6.38

[Ben Hutchings]

On Fri, 2011-03-18 at 12:52 -0700, David Miller wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Fri, 18 Mar 2011 08:15:24 -0700
> 
> > On Fri, 18 Mar 2011 15:33:04 +0100
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > 
> >> Le vendredi 18 mars 2011 à 14:17 +0000, Ben Hutchings a écrit :
> >> 
> >> > WARN is correct as this is a driver bug.  But I agree that the
> >> > device/driver ID should be included.
> >> 
> >> stack trace gives absolutely no useful indication here.
> >> 
> >> Bug is in driver, yet we dump information on core network stack ?
> >> 
> >> pr_err() is an error indication, not a warning by the way ;)
> > 
> > The advantage of WARN is that it doesn't get ignored and shows
> > up in kernel oops. But agreed it should print out as much device
> > info as possible to finger the broken device driver.
> 
> Infrastructure is not static, therefore we could add a WARN_ON_NETDEV()
> or similar.  An in fact such things would probably be very useful.

You mean like netdev_WARN()? :-)

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: LRO disable warnings on kernel 2.6.38

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 18 Mar 2011 19:58:06 +0000

> You mean like netdev_WARN()? :-)

Perfect, but it's be nice if we had a variant where the condition were
embedded into the macro as well.

Re: LRO disable warnings on kernel 2.6.38

[Ben Hutchings]

On Fri, 2011-03-18 at 12:12 +0100, Jesper Dangaard Brouer wrote:
> Hi
> 
> I'm seeing the LRO disable warnings using kernel 2.6.38:
> 
> [    8.664759] NET: Registered protocol family 10
> [    8.838148] ADDRCONF(NETDEV_UP): eth71: link is not ready
> [    8.872639] ------------[ cut here ]------------
> [    8.872645] WARNING: at net/core/dev.c:1363 dev_disable_lro+0x7b/0x80()
> [    8.872647] Hardware name: ProLiant DL370 G6
> [    8.872648] Modules linked in: ipv6 nf_conntrack ip_tables loop i7core_edac edac_core ipmi_si ipmi_msghandler joydev hpilo pcspkr sg hpsa igb ata_piix netxen_nic dca [last unloaded: scsi_wait_scan]
> [    8.872660] Pid: 2221, comm: sysctl Not tainted 2.6.38-comx04 #2
> [    8.872662] Call Trace:
> [    8.872671]  [<ffffffff81056e1f>] ? warn_slowpath_common+0x7f/0xc0
> [    8.872675]  [<ffffffff81056e7a>] ? warn_slowpath_null+0x1a/0x20
> [    8.872680]  [<ffffffff8140c0ab>] ? dev_disable_lro+0x7b/0x80
> [    8.872686]  [<ffffffff81474f27>] ? devinet_sysctl_forward+0x147/0x180
> [    8.872691]  [<ffffffff811872f7>] ? proc_sys_call_handler+0x97/0xd0
> [    8.872700]  [<ffffffff81187344>] ? proc_sys_write+0x14/0x20
> [    8.872704]  [<ffffffff81124148>] ? vfs_write+0xc8/0x180
> [    8.872707]  [<ffffffff81124301>] ? sys_write+0x51/0x90
> [    8.872712]  [<ffffffff8100b8c2>] ? system_call_fastpath+0x16/0x1b
> [    8.872714] ---[ end trace 6245283cb8d484cc ]---
> 
> The strange part is that I didn't see this warning on my testlab and
> pre-prod servers.  The warning is from the first production server,
> which got kernel 2.6.38 deployed this morning.
> 
> The NIC driver is igb.
>
> The only difference in hardware between the production and
> pre-production server (which didn't show the warning), is the
> prod-server have an extra dual-port original Intel NIC, dev-named
> "eth71".  And its just after the init of eth71, the warning occurs.

The warning is triggered if you enable forwarding or bridging on a
device without LRO, and the driver fails to disable LRO when requested.
You can see from the call chain that this is being triggered by a sysctl
write, not during registration.

> We usually use a 6 port NIC from Hotlava, which is based on the same
> chip 82576 and also uses the same igb driver.
> 
> Intel orig NIC eth71
>  albpd4:~# ethtool -i eth71
>  driver: igb
>  version: 2.1.0-k2
[...]

This appears to be the in-tree version of igb, which never enables LRO.
So I can't see how it could trigger this warning.

Are any other drivers being used for other interfaces?

And I'm sorry I forgot to include device & driver in this warning!

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: LRO disable warnings on kernel 2.6.38

[Alexander Duyck]

On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> Hi
>
> I'm seeing the LRO disable warnings using kernel 2.6.38:
>
> [    8.664759] NET: Registered protocol family 10
> [    8.838148] ADDRCONF(NETDEV_UP): eth71: link is not ready
> [    8.872639] ------------[ cut here ]------------
> [    8.872645] WARNING: at net/core/dev.c:1363 dev_disable_lro+0x7b/0x80()
> [    8.872647] Hardware name: ProLiant DL370 G6
> [    8.872648] Modules linked in: ipv6 nf_conntrack ip_tables loop i7core_edac edac_core ipmi_si ipmi_msghandler joydev hpilo pcspkr sg hpsa igb ata_piix netxen_nic dca [last unloaded: scsi_wait_scan]
> [    8.872660] Pid: 2221, comm: sysctl Not tainted 2.6.38-comx04 #2
> [    8.872662] Call Trace:
> [    8.872671]  [<ffffffff81056e1f>] ? warn_slowpath_common+0x7f/0xc0
> [    8.872675]  [<ffffffff81056e7a>] ? warn_slowpath_null+0x1a/0x20
> [    8.872680]  [<ffffffff8140c0ab>] ? dev_disable_lro+0x7b/0x80
> [    8.872686]  [<ffffffff81474f27>] ? devinet_sysctl_forward+0x147/0x180
> [    8.872691]  [<ffffffff811872f7>] ? proc_sys_call_handler+0x97/0xd0
> [    8.872700]  [<ffffffff81187344>] ? proc_sys_write+0x14/0x20
> [    8.872704]  [<ffffffff81124148>] ? vfs_write+0xc8/0x180
> [    8.872707]  [<ffffffff81124301>] ? sys_write+0x51/0x90
> [    8.872712]  [<ffffffff8100b8c2>] ? system_call_fastpath+0x16/0x1b
> [    8.872714] ---[ end trace 6245283cb8d484cc ]---
>
> The strange part is that I didn't see this warning on my testlab and
> pre-prod servers.  The warning is from the first production server,
> which got kernel 2.6.38 deployed this morning.
>
> The NIC driver is igb.
>
> The only difference in hardware between the production and
> pre-production server (which didn't show the warning), is the
> prod-server have an extra dual-port original Intel NIC, dev-named
> "eth71".  And its just after the init of eth71, the warning occurs.
>
> We usually use a 6 port NIC from Hotlava, which is based on the same
> chip 82576 and also uses the same igb driver.
>
> Intel orig NIC eth71
>   albpd4:~# ethtool -i eth71
>   driver: igb
>   version: 2.1.0-k2
>   firmware-version: 1.2-1
>   bus-info: 0000:21:00.0
>
> Hotlava Intel chip based NIC eth51:
>   albpd4:~# ethtool -i eth51
>   driver: igb
>   version: 2.1.0-k2
>   firmware-version: 1.2-1
>   bus-info: 0000:1d:00.1
>
> I don't understand why I don't see the warning on my pre-prod server,
> which only have the Hotlava NIC?!?
>

The error doesn't make any sense for igb to be triggering since it 
doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.

By any chance are there any ixgbe or other interfaces in the system?  I 
would suspect the error to come from a driver that at least contains the 
NETIF_F_LRO flag.

Thanks,

Alex

Re: LRO disable warnings on kernel 2.6.38

[Jesper Dangaard Brouer]

On Fri, 2011-03-18 at 09:18 -0700, Alexander Duyck wrote:
> On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> > Hi
> >
> > I'm seeing the LRO disable warnings using kernel 2.6.38:
[...]
> >
> The error doesn't make any sense for igb to be triggering since it 
> doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.
> 
> By any chance are there any ixgbe or other interfaces in the system?  I 
> would suspect the error to come from a driver that at least contains the 
> NETIF_F_LRO flag.

The servers actually also have a four port semi-build in NIC (its a PCIe
slot with an extra connector, most likely for controlling the LEDs).  I
don't use that NIC as it (1) caused kernel panics on 2.6.35, (2) needs a
firmware blob, (3) reading through the driver code (at some point in
time) they didn't implement multiqueue support correctly.

So lets blame that driver ;-)

lspci-info:
 Ethernet controller: NetXen Incorporated NX3031 Multifunction 1/10-Gigabit Server Adapter (rev 42)

driver info via ethtool:
# ethtool -i eth01
 driver: netxen_nic
 version: 4.0.75
 firmware-version: 4.0.530
 bus-info: 0000:06:00.3

The strange part is that my pre-prod server also have a netxen_nic, but
it does not result in a WARN being tricked... 

The pre-prod server do have a different firmware version.
# ethtool -i eth01
 driver: netxen_nic
 version: 4.0.75
 firmware-version: 4.0.406
 bus-info: 0000:06:00.0

(I have pulled the NIC out of some of the prod server, due to the risk
of a kernel panic on the old kernel.  As operations have taken over the
deployment process, these NICs are still in.  Guess I'll ask them to
blacklist the driver, so they don't see the warn stacktrace, they get so
nervous when they see stuff like that ;-))

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network Kernel Developer
  Cand. Scient Datalog / MSc.CS
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: LRO disable warnings on kernel 2.6.38

[Eric Dumazet]

Le lundi 21 mars 2011 à 10:21 +0100, Jesper Dangaard Brouer a écrit :
> On Fri, 2011-03-18 at 09:18 -0700, Alexander Duyck wrote:
> > On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> > > Hi
> > >
> > > I'm seeing the LRO disable warnings using kernel 2.6.38:
> [...]
> > >
> > The error doesn't make any sense for igb to be triggering since it 
> > doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.
> > 
> > By any chance are there any ixgbe or other interfaces in the system?  I 
> > would suspect the error to come from a driver that at least contains the 
> > NETIF_F_LRO flag.
> 
> The servers actually also have a four port semi-build in NIC (its a PCIe
> slot with an extra connector, most likely for controlling the LEDs).  I
> don't use that NIC as it (1) caused kernel panics on 2.6.35, (2) needs a
> firmware blob, (3) reading through the driver code (at some point in
> time) they didn't implement multiqueue support correctly.
> 
> So lets blame that driver ;-)
> 
> lspci-info:
>  Ethernet controller: NetXen Incorporated NX3031 Multifunction 1/10-Gigabit Server Adapter (rev 42)
> 
> driver info via ethtool:
> # ethtool -i eth01
>  driver: netxen_nic
>  version: 4.0.75
>  firmware-version: 4.0.530
>  bus-info: 0000:06:00.3
> 
> The strange part is that my pre-prod server also have a netxen_nic, but
> it does not result in a WARN being tricked... 
> 
> The pre-prod server do have a different firmware version.
> # ethtool -i eth01
>  driver: netxen_nic
>  version: 4.0.75
>  firmware-version: 4.0.406
>  bus-info: 0000:06:00.0
> 
> (I have pulled the NIC out of some of the prod server, due to the risk
> of a kernel panic on the old kernel.  As operations have taken over the
> deployment process, these NICs are still in.  Guess I'll ask them to
> blacklist the driver, so they don't see the warn stacktrace, they get so
> nervous when they see stuff like that ;-))
> 

Well, this warning can be ignored since these NIC dont receive packets
to be forwarded in your setup.

I would say netxen_nic_set_flags() is buggy : It rejects data if other
flag than ETH_FLAG_LRO is set.

if (data & ~ETH_FLAG_LRO)
	return -EINVAL;

Brought by commit ef2519b1dd39940 (netxen: fail when try to setup
unsupported features) later corrected by commit 97d1935a61b7fe7a65f98f
(Make ethtool_ops::set_flags() return -EINVAL for unsupported flags)

Since this drivers asserts NETIF_F_HW_VLAN_TX (mirroring
ETH_FLAG_TXVLAN), ethtool_op_get_flags() can return more than
ETH_FLAG_LRO.

dev_disable_lro() then calls netxen_nic_set_flags() with ETH_FLAG_TXVLAN
-> -EINVAL, and ETH_FLAG_LRO stay ORed in dev->features -> WARNING

Re: LRO disable warnings on kernel 2.6.38

[Eric Dumazet]

Le lundi 21 mars 2011 à 10:45 +0100, Eric Dumazet a écrit :
> Le lundi 21 mars 2011 à 10:21 +0100, Jesper Dangaard Brouer a écrit :
> > On Fri, 2011-03-18 at 09:18 -0700, Alexander Duyck wrote:
> > > On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> > > > Hi
> > > >
> > > > I'm seeing the LRO disable warnings using kernel 2.6.38:
> > [...]
> > > >
> > > The error doesn't make any sense for igb to be triggering since it 
> > > doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.
> > > 
> > > By any chance are there any ixgbe or other interfaces in the system?  I 
> > > would suspect the error to come from a driver that at least contains the 
> > > NETIF_F_LRO flag.
> > 
> > The servers actually also have a four port semi-build in NIC (its a PCIe
> > slot with an extra connector, most likely for controlling the LEDs).  I
> > don't use that NIC as it (1) caused kernel panics on 2.6.35, (2) needs a
> > firmware blob, (3) reading through the driver code (at some point in
> > time) they didn't implement multiqueue support correctly.
> > 
> > So lets blame that driver ;-)
> > 
> > lspci-info:
> >  Ethernet controller: NetXen Incorporated NX3031 Multifunction 1/10-Gigabit Server Adapter (rev 42)
> > 
> > driver info via ethtool:
> > # ethtool -i eth01
> >  driver: netxen_nic
> >  version: 4.0.75
> >  firmware-version: 4.0.530
> >  bus-info: 0000:06:00.3
> > 
> > The strange part is that my pre-prod server also have a netxen_nic, but
> > it does not result in a WARN being tricked... 
> > 
> > The pre-prod server do have a different firmware version.
> > # ethtool -i eth01
> >  driver: netxen_nic
> >  version: 4.0.75
> >  firmware-version: 4.0.406
> >  bus-info: 0000:06:00.0
> > 
> > (I have pulled the NIC out of some of the prod server, due to the risk
> > of a kernel panic on the old kernel.  As operations have taken over the
> > deployment process, these NICs are still in.  Guess I'll ask them to
> > blacklist the driver, so they don't see the warn stacktrace, they get so
> > nervous when they see stuff like that ;-))
> > 
> 
> Well, this warning can be ignored since these NIC dont receive packets
> to be forwarded in your setup.
> 
> I would say netxen_nic_set_flags() is buggy : It rejects data if other
> flag than ETH_FLAG_LRO is set.
> 
> if (data & ~ETH_FLAG_LRO)
> 	return -EINVAL;
> 
> Brought by commit ef2519b1dd39940 (netxen: fail when try to setup
> unsupported features) later corrected by commit 97d1935a61b7fe7a65f98f
> (Make ethtool_ops::set_flags() return -EINVAL for unsupported flags)
> 
> Since this drivers asserts NETIF_F_HW_VLAN_TX (mirroring
> ETH_FLAG_TXVLAN), ethtool_op_get_flags() can return more than
> ETH_FLAG_LRO.
> 
> dev_disable_lro() then calls netxen_nic_set_flags() with ETH_FLAG_TXVLAN
> -> -EINVAL, and ETH_FLAG_LRO stay ORed in dev->features -> WARNING
> 
> 
> 
> 

Couly you please send us the result of "ethtool -k eth01" to make sure
its the problem ?

Make sure your ethtool version includes these bits :
# ethtool -k eth0
Offload parameters for eth0:
...
large-receive-offload: off
rx-vlan-offload: on
tx-vlan-offload: on
ntuple-filters: off
receive-hashing: off


Thanks

Re: LRO disable warnings on kernel 2.6.38

[Jesper Dangaard Brouer]

On Mon, 2011-03-21 at 10:53 +0100, Eric Dumazet wrote:
> Couly you please send us the result of "ethtool -k eth01" to make sure
> its the problem ?

prod server:

albpd4:~# ethtool -k eth01
Offload parameters for eth01:
rx-checksumming: on
tx-checksumming: on
scatter-gather: on
tcp-segmentation-offload: on
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: on
ntuple-filters: off
receive-hashing: off

pre-prod server:

albpd1:~# ethtool -k eth01
Offload parameters for eth01:
rx-checksumming: on
tx-checksumming: on
scatter-gather: on
tcp-segmentation-offload: on
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: off
ntuple-filters: off
receive-hashing: off

Hmm... notice how the large-receive-offload (LRO) is different on the
two machines.

I cannot disable LRO with userspace ethtool:

albpd4:~# ethtool -K eth01 lro off
Cannot set large receive offload settings: Invalid argument

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network Kernel Developer
  Cand. Scient Datalog / MSc.CS
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

Re: LRO disable warnings on kernel 2.6.38

[Eric Dumazet]

Le lundi 21 mars 2011 à 11:04 +0100, Jesper Dangaard Brouer a écrit :
> On Mon, 2011-03-21 at 10:53 +0100, Eric Dumazet wrote:
> > Couly you please send us the result of "ethtool -k eth01" to make sure
> > its the problem ?
> 
> prod server:
> 
> albpd4:~# ethtool -k eth01
> Offload parameters for eth01:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: on
> udp-fragmentation-offload: off
> generic-segmentation-offload: on
> generic-receive-offload: on
> large-receive-offload: on
> ntuple-filters: off
> receive-hashing: off
> 
> pre-prod server:
> 
> albpd1:~# ethtool -k eth01
> Offload parameters for eth01:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: on
> udp-fragmentation-offload: off
> generic-segmentation-offload: on
> generic-receive-offload: on
> large-receive-offload: off
> ntuple-filters: off
> receive-hashing: off
> 
> Hmm... notice how the large-receive-offload (LRO) is different on the
> two machines.
> 
> I cannot disable LRO with userspace ethtool:
> 
> albpd4:~# ethtool -K eth01 lro off
> Cannot set large receive offload settings: Invalid argument
> 

Sure, this is the problem. Still your ethtool dont give us the needed
info ;)

I suspect it is :

rx-vlan-offload: off
tx-vlan-offload: on

But would like a confirmation of this ;)

# ethtool -V | head -n 1
ethtool version 2.6.37

# ethtool --version
ethtool version 2.6.37

Re: LRO disable warnings on kernel 2.6.38

[Jesper Dangaard Brouer]

On Mon, 2011-03-21 at 11:11 +0100, Eric Dumazet wrote:
> Sure, this is the problem. Still your ethtool dont give us the needed
> info ;)
> 
> I suspect it is :
> 
> rx-vlan-offload: off
> tx-vlan-offload: on

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

> But would like a confirmation of this ;)

Bonus! - with a newer version of ethtool ;-0

> # ethtool -V | head -n 1
> ethtool version 2.6.37

albpd4:~# ethtool -V | head -1
ethtool version 2.6.34

> # ethtool --version
> ethtool version 2.6.37

albpd4:~# ~/ethtool-2.6.38 --version
ethtool version 2.6.38
(actually latest git tree, 423c656b22ec which is a release by Ben)


-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network Kernel Developer
  Cand. Scient Datalog / MSc.CS
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

RE: LRO disable warnings on kernel 2.6.38

[Amit Salecha]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Eric Dumazet
> Sent: Monday, March 21, 2011 3:15 PM
> To: Jesper Dangaard Brouer
> Cc: Alexander Duyck; netdev; Neil Horman; Stanislaw Gruszka
> Subject: Re: LRO disable warnings on kernel 2.6.38
>
> Le lundi 21 mars 2011 à 10:21 +0100, Jesper Dangaard Brouer a écrit :
> > On Fri, 2011-03-18 at 09:18 -0700, Alexander Duyck wrote:
> > > On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> > > > Hi
> > > >
> > > > I'm seeing the LRO disable warnings using kernel 2.6.38:
> > [...]
> > > >
> > > The error doesn't make any sense for igb to be triggering since it
> > > doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.
> > >
> > > By any chance are there any ixgbe or other interfaces in the
> system?  I
> > > would suspect the error to come from a driver that at least
> contains the
> > > NETIF_F_LRO flag.
> >
> > The servers actually also have a four port semi-build in NIC (its a
> PCIe
> > slot with an extra connector, most likely for controlling the LEDs).
> I
> > don't use that NIC as it (1) caused kernel panics on 2.6.35, (2)
> needs a
> > firmware blob, (3) reading through the driver code (at some point in
> > time) they didn't implement multiqueue support correctly.
> >
> > So lets blame that driver ;-)
> >
> > lspci-info:
> >  Ethernet controller: NetXen Incorporated NX3031 Multifunction 1/10-
> Gigabit Server Adapter (rev 42)
> >
> > driver info via ethtool:
> > # ethtool -i eth01
> >  driver: netxen_nic
> >  version: 4.0.75
> >  firmware-version: 4.0.530
> >  bus-info: 0000:06:00.3
> >
> > The strange part is that my pre-prod server also have a netxen_nic,
> but
> > it does not result in a WARN being tricked...
> >
> > The pre-prod server do have a different firmware version.
> > # ethtool -i eth01
> >  driver: netxen_nic
> >  version: 4.0.75
> >  firmware-version: 4.0.406
> >  bus-info: 0000:06:00.0
> >
> > (I have pulled the NIC out of some of the prod server, due to the
> risk
> > of a kernel panic on the old kernel.  As operations have taken over
> the
> > deployment process, these NICs are still in.  Guess I'll ask them to
> > blacklist the driver, so they don't see the warn stacktrace, they get
> so
> > nervous when they see stuff like that ;-))
> >
>
> Well, this warning can be ignored since these NIC dont receive packets
> to be forwarded in your setup.
>
> I would say netxen_nic_set_flags() is buggy : It rejects data if other
> flag than ETH_FLAG_LRO is set.
>
> if (data & ~ETH_FLAG_LRO)
>       return -EINVAL;
>
> Brought by commit ef2519b1dd39940 (netxen: fail when try to setup
> unsupported features) later corrected by commit 97d1935a61b7fe7a65f98f
> (Make ethtool_ops::set_flags() return -EINVAL for unsupported flags)
>
> Since this drivers asserts NETIF_F_HW_VLAN_TX (mirroring
> ETH_FLAG_TXVLAN), ethtool_op_get_flags() can return more than
> ETH_FLAG_LRO.
>
> dev_disable_lro() then calls netxen_nic_set_flags() with
> ETH_FLAG_TXVLAN
> -> -EINVAL, and ETH_FLAG_LRO stay ORed in dev->features -> WARNING
>
>
Thanks Eric.

Instead of
        if (data & ~ETH_FLAG_LRO)
                return -EINVAL;

check should be like this:

+    if ((ethtool_op_get_flags(netdev) & ~ETH_FLAG_LRO) !=
+                                       (data & ~ETH_FLAG_LRO))
                return -EINVAL;

Will provide patch soon.

-Amit
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

Re: LRO disable warnings on kernel 2.6.38

[Stanislaw Gruszka]

On Mon, Mar 21, 2011 at 05:00:31AM -0500, Amit Salecha wrote:
> > Well, this warning can be ignored since these NIC dont receive packets
> > to be forwarded in your setup.
> >
> > I would say netxen_nic_set_flags() is buggy : It rejects data if other
> > flag than ETH_FLAG_LRO is set.
> >
> > if (data & ~ETH_FLAG_LRO)
> >       return -EINVAL;
> >
> > Brought by commit ef2519b1dd39940 (netxen: fail when try to setup
> > unsupported features) later corrected by commit 97d1935a61b7fe7a65f98f
> > (Make ethtool_ops::set_flags() return -EINVAL for unsupported flags)
> >
> > Since this drivers asserts NETIF_F_HW_VLAN_TX (mirroring
> > ETH_FLAG_TXVLAN), ethtool_op_get_flags() can return more than
> > ETH_FLAG_LRO.
> >
> > dev_disable_lro() then calls netxen_nic_set_flags() with
> > ETH_FLAG_TXVLAN
> > -> -EINVAL, and ETH_FLAG_LRO stay ORed in dev->features -> WARNING
> >
> >
> Thanks Eric.
> 
> Instead of
>         if (data & ~ETH_FLAG_LRO)
>                 return -EINVAL;
> 
> check should be like this:
> 
> +    if ((ethtool_op_get_flags(netdev) & ~ETH_FLAG_LRO) !=
> +                                       (data & ~ETH_FLAG_LRO))
>                 return -EINVAL;
> 
> Will provide patch soon.

Yep, that should fix issue in netxen.

I'm afraid some other drivers can have similar problem after
adding ETH_FLAG_{TX,RX}VLAN. I think we need to distinguish features
that are configurable at runtime from features that are hardcoded.
I'm going to look at that.

Thanks
Stanislaw

Re: LRO disable warnings on kernel 2.6.38

[Stanislaw Gruszka]

On Mon, Mar 21, 2011 at 01:34:06PM +0100, Stanislaw Gruszka wrote:
> On Mon, Mar 21, 2011 at 05:00:31AM -0500, Amit Salecha wrote:
> > > Well, this warning can be ignored since these NIC dont receive packets
> > > to be forwarded in your setup.
> > >
> > > I would say netxen_nic_set_flags() is buggy : It rejects data if other
> > > flag than ETH_FLAG_LRO is set.
> > >
> > > if (data & ~ETH_FLAG_LRO)
> > >       return -EINVAL;
> > >
> > > Brought by commit ef2519b1dd39940 (netxen: fail when try to setup
> > > unsupported features) later corrected by commit 97d1935a61b7fe7a65f98f
> > > (Make ethtool_ops::set_flags() return -EINVAL for unsupported flags)
> > >
> > > Since this drivers asserts NETIF_F_HW_VLAN_TX (mirroring
> > > ETH_FLAG_TXVLAN), ethtool_op_get_flags() can return more than
> > > ETH_FLAG_LRO.
> > >
> > > dev_disable_lro() then calls netxen_nic_set_flags() with
> > > ETH_FLAG_TXVLAN
> > > -> -EINVAL, and ETH_FLAG_LRO stay ORed in dev->features -> WARNING
> > >
> > >
> > Thanks Eric.
> > 
> > Instead of
> >         if (data & ~ETH_FLAG_LRO)
> >                 return -EINVAL;
> > 
> > check should be like this:
> > 
> > +    if ((ethtool_op_get_flags(netdev) & ~ETH_FLAG_LRO) !=
> > +                                       (data & ~ETH_FLAG_LRO))
> >                 return -EINVAL;
> > 
> > Will provide patch soon.
> 
> Yep, that should fix issue in netxen.
> 
> I'm afraid some other drivers can have similar problem after
> adding ETH_FLAG_{TX,RX}VLAN. I think we need to distinguish features
> that are configurable at runtime from features that are hardcoded.
> I'm going to look at that.

Other drivers have this bug too. I'm going to prepare patch with similar fix
like Amit proposed, but also for other drivers. Something like below:

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c1a71bb..38fd0cb 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -141,9 +141,17 @@ u32 ethtool_op_get_flags(struct net_device *dev)
 }
 EXPORT_SYMBOL(ethtool_op_get_flags);
 
+bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported)
+{
+	if ((dev->features & ~supported) != (data & ~supported))
+		return true;
+	else
+		return false;
+}
+
 int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
 {
-	if (data & ~supported)
+	if (ethtool_invalid_flags(dev, data, supported);
 		return -EINVAL;
 
 	dev->features = ((dev->features & ~flags_dup_features) |
diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
index 81254be..7a662f1 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
@@ -304,8 +304,8 @@ vmxnet3_set_flags(struct net_device *netdev, u32 data)
 	u8 lro_present = (netdev->features & NETIF_F_LRO) == 0 ? 0 : 1;
 	unsigned long flags;
 
-	if (data & ~ETH_FLAG_LRO)
-		return -EOPNOTSUPP;
+	if (ethtool_invalid_flags(netdev, data, ETH_FLAG_LRO))
+		return -EINVAL;
 
 	if (lro_requested ^ lro_present) {
 		/* toggle the LRO feature*/

Re: LRO disable warnings on kernel 2.6.38

[Ben Hutchings]

On Mon, 2011-03-21 at 14:46 +0100, Stanislaw Gruszka wrote:
> On Mon, Mar 21, 2011 at 01:34:06PM +0100, Stanislaw Gruszka wrote:
[...]
> > I'm afraid some other drivers can have similar problem after
> > adding ETH_FLAG_{TX,RX}VLAN. I think we need to distinguish features
> > that are configurable at runtime from features that are hardcoded.
> > I'm going to look at that.
> 
> Other drivers have this bug too. I'm going to prepare patch with similar fix
> like Amit proposed, but also for other drivers. Something like below:

This may be useful temporarily, but the new netdev-features API seems to
make this easier to get right.  We should be moving drivers over to that
instead.

Ben.

> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c1a71bb..38fd0cb 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -141,9 +141,17 @@ u32 ethtool_op_get_flags(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(ethtool_op_get_flags);
>  
> +bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported)
> +{
> +	if ((dev->features & ~supported) != (data & ~supported))
> +		return true;
> +	else
> +		return false;
> +}
> +
>  int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
>  {
> -	if (data & ~supported)
> +	if (ethtool_invalid_flags(dev, data, supported);
>  		return -EINVAL;
>  
>  	dev->features = ((dev->features & ~flags_dup_features) |
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
> index 81254be..7a662f1 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
> @@ -304,8 +304,8 @@ vmxnet3_set_flags(struct net_device *netdev, u32 data)
>  	u8 lro_present = (netdev->features & NETIF_F_LRO) == 0 ? 0 : 1;
>  	unsigned long flags;
>  
> -	if (data & ~ETH_FLAG_LRO)
> -		return -EOPNOTSUPP;
> +	if (ethtool_invalid_flags(netdev, data, ETH_FLAG_LRO))
> +		return -EINVAL;
>  
>  	if (lro_requested ^ lro_present) {
>  		/* toggle the LRO feature*/

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[RFC] net: fix ethtool->set_flags not intended -EINVAL return value

[Stanislaw Gruszka]

After commit d5dbda23804156ae6f35025ade5307a49d1db6d7 "ethtool: Add
support for vlan accleration.", drivers that have NETIF_F_HW_VLAN_TX,
and/or NETIF_F_HW_VLAN_RX feature, but do not allow enable/disable vlan
acceleration via ethtool set_flags, always return -EINVAL from that
function. Fix by returning -EINVAL only if requested features do
not match current settings and can not be changed by driver.

RFC for now, compile tested only.

diff --git a/drivers/net/netxen/netxen_nic_ethtool.c b/drivers/net/netxen/netxen_nic_ethtool.c
index 653d308..3bdcc80 100644
--- a/drivers/net/netxen/netxen_nic_ethtool.c
+++ b/drivers/net/netxen/netxen_nic_ethtool.c
@@ -871,7 +871,7 @@ static int netxen_nic_set_flags(struct net_device *netdev, u32 data)
 	struct netxen_adapter *adapter = netdev_priv(netdev);
 	int hw_lro;
 
-	if (data & ~ETH_FLAG_LRO)
+	if (ethtool_invalid_flags(netdev, data, ETH_FLAG_LRO))
 		return -EINVAL;
 
 	if (!(adapter->capabilities & NX_FW_CAPABILITY_HW_LRO))
diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
index 4c14510..45b2755 100644
--- a/drivers/net/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c
@@ -1003,7 +1003,7 @@ static int qlcnic_set_flags(struct net_device *netdev, u32 data)
 	struct qlcnic_adapter *adapter = netdev_priv(netdev);
 	int hw_lro;
 
-	if (data & ~ETH_FLAG_LRO)
+	if (ethtool_invalid_flags(netdev, data, ETH_FLAG_LRO))
 		return -EINVAL;
 
 	if (!(adapter->capabilities & QLCNIC_FW_CAPABILITY_HW_LRO))
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 2ad6364..356e74d 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -6726,7 +6726,7 @@ static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
 	int rc = 0;
 	int changed = 0;
 
-	if (data & ~ETH_FLAG_LRO)
+	if (ethtool_invalid_flags(dev, data, ETH_FLAG_LRO))
 		return -EINVAL;
 
 	if (data & ETH_FLAG_LRO) {
diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
index 81254be..51f2ef1 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
@@ -304,8 +304,8 @@ vmxnet3_set_flags(struct net_device *netdev, u32 data)
 	u8 lro_present = (netdev->features & NETIF_F_LRO) == 0 ? 0 : 1;
 	unsigned long flags;
 
-	if (data & ~ETH_FLAG_LRO)
-		return -EOPNOTSUPP;
+	if (ethtool_invalid_flags(netdev, data, ETH_FLAG_LRO))
+		return -EINVAL;
 
 	if (lro_requested ^ lro_present) {
 		/* toggle the LRO feature*/
diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
index 1dd3a21..c5eb034 100644
--- a/drivers/net/vxge/vxge-ethtool.c
+++ b/drivers/net/vxge/vxge-ethtool.c
@@ -1117,8 +1117,8 @@ static int vxge_set_flags(struct net_device *dev, u32 data)
 	struct vxgedev *vdev = netdev_priv(dev);
 	enum vxge_hw_status status;
 
-	if (data & ~ETH_FLAG_RXHASH)
-		return -EOPNOTSUPP;
+	if (ethtool_invalid_flags(dev, data, ETH_FLAG_RXHASH))
+		return -EINVAL;
 
 	if (!!(data & ETH_FLAG_RXHASH) == vdev->devh->config.rth_en)
 		return 0;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index aac3e2e..e22b046 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -643,6 +643,7 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data);
 u32 ethtool_op_get_flags(struct net_device *dev);
 int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported);
 void ethtool_ntuple_flush(struct net_device *dev);
+bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported);
 
 /**
  * &ethtool_ops - Alter and report network device settings
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c1a71bb..514127d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -141,9 +141,17 @@ u32 ethtool_op_get_flags(struct net_device *dev)
 }
 EXPORT_SYMBOL(ethtool_op_get_flags);
 
+bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported)
+{
+	u32 features = dev->features & flags_dup_features;
+
+	return ((features & ~supported) != (data & ~supported));
+}
+EXPORT_SYMBOL(ethtool_invalid_flags);
+
 int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
 {
-	if (data & ~supported)
+	if (ethtool_invalid_flags(dev, data, supported))
 		return -EINVAL;
 
 	dev->features = ((dev->features & ~flags_dup_features) |

Re: [RFC] net: fix ethtool->set_flags not intended -EINVAL return value

[Ben Hutchings]

On Mon, 2011-03-21 at 16:10 +0100, Stanislaw Gruszka wrote:
> After commit d5dbda23804156ae6f35025ade5307a49d1db6d7 "ethtool: Add
> support for vlan accleration.", drivers that have NETIF_F_HW_VLAN_TX,
> and/or NETIF_F_HW_VLAN_RX feature, but do not allow enable/disable vlan
> acceleration via ethtool set_flags, always return -EINVAL from that
> function. Fix by returning -EINVAL only if requested features do
> not match current settings and can not be changed by driver.
> 
> RFC for now, compile tested only.

This looks good, but:

[...]
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c1a71bb..514127d 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -141,9 +141,17 @@ u32 ethtool_op_get_flags(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(ethtool_op_get_flags);
>  
> +bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported)
> +{
> +	u32 features = dev->features & flags_dup_features;
> +
> +	return ((features & ~supported) != (data & ~supported));
> +}
> +EXPORT_SYMBOL(ethtool_invalid_flags);
[...]

This needs a comment.

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: [RFC] net: fix ethtool->set_flags not intended -EINVAL return value

[Jesse Gross]

On Mon, Mar 21, 2011 at 8:10 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> After commit d5dbda23804156ae6f35025ade5307a49d1db6d7 "ethtool: Add
> support for vlan accleration.", drivers that have NETIF_F_HW_VLAN_TX,
> and/or NETIF_F_HW_VLAN_RX feature, but do not allow enable/disable vlan
> acceleration via ethtool set_flags, always return -EINVAL from that
> function. Fix by returning -EINVAL only if requested features do
> not match current settings and can not be changed by driver.

Looks good to me, thanks for fixing this.