diff mbox

netxen: fix LRO disable warning

Message ID 1300703828-6291-1-git-send-email-amit.salecha@qlogic.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

amit salecha March 21, 2011, 10:37 a.m. UTC
netxen_nic_set_flags() rejects data if other flag than ETH_FLAG_LRO is set.
Driver also supports NETIF_F_HW_VLAN_TX.
Now compare data with ethtool_op_get_flags(), to get all supported features.

Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
Signed-off-by: Rajesh Borundia <rajesh.borundia@qlogic.com>
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/netxen/netxen_nic_ethtool.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Comments

Eric Dumazet March 21, 2011, 10:47 a.m. UTC | #1
Le lundi 21 mars 2011 à 03:37 -0700, Amit Kumar Salecha a écrit :
> netxen_nic_set_flags() rejects data if other flag than ETH_FLAG_LRO is set.
> Driver also supports NETIF_F_HW_VLAN_TX.
> Now compare data with ethtool_op_get_flags(), to get all supported features.
> 
> Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> Signed-off-by: Rajesh Borundia <rajesh.borundia@qlogic.com>
> Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
> ---

Hmm, it would be really nice you provide more information for stable
teams.

If I am not mistaken, bug was introduced in 2.6.36 ?



--
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
amit salecha March 21, 2011, 11:13 a.m. UTC | #2
> Le lundi 21 mars 2011 à 03:37 -0700, Amit Kumar Salecha a écrit :

> > netxen_nic_set_flags() rejects data if other flag than ETH_FLAG_LRO

> is set.

> > Driver also supports NETIF_F_HW_VLAN_TX.

> > Now compare data with ethtool_op_get_flags(), to get all supported

> features.

> >

> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>

> > Signed-off-by: Rajesh Borundia <rajesh.borundia@qlogic.com>

> > Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>

> > ---

>

> Hmm, it would be really nice you provide more information for stable

> teams.

>

> If I am not mistaken, bug was introduced in 2.6.36 ?

>

No, It was introduced in 2.6.37, when ETH_FLAG_TXVLAN and ETH_FLAG_RXVLAN introduced.
Yes it should also make into 2.6.37.4 stable kernel.


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.
David Miller March 28, 2011, 1:08 a.m. UTC | #3
From: Amit Salecha <amit.salecha@qlogic.com>
Date: Mon, 21 Mar 2011 06:13:02 -0500

>> Le lundi 21 mars 2011 à 03:37 -0700, Amit Kumar Salecha a écrit :
>> > netxen_nic_set_flags() rejects data if other flag than ETH_FLAG_LRO
>> is set.
>> > Driver also supports NETIF_F_HW_VLAN_TX.
>> > Now compare data with ethtool_op_get_flags(), to get all supported
>> features.
>> >
>> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
>> > Signed-off-by: Rajesh Borundia <rajesh.borundia@qlogic.com>
>> > Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
>> > ---
>>
>> Hmm, it would be really nice you provide more information for stable
>> teams.
>>
>> If I am not mistaken, bug was introduced in 2.6.36 ?
>>
> No, It was introduced in 2.6.37, when ETH_FLAG_TXVLAN and ETH_FLAG_RXVLAN introduced.
> Yes it should also make into 2.6.37.4 stable kernel.

Applied, but 2.6.37.x maintainence has ceased so there is no point
worrying about this patch for that stable branch any longer.
--
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
Stanislaw Gruszka March 28, 2011, 5:17 a.m. UTC | #4
On Mon, Mar 28, 2011 at 12:33:43AM -0500, Amit Salecha wrote:
> > >> > netxen_nic_set_flags() rejects data if other flag than
> > ETH_FLAG_LRO
> > >> is set.
> > >> > Driver also supports NETIF_F_HW_VLAN_TX.
> > >> > Now compare data with ethtool_op_get_flags(), to get all supported
> > >> features.
> > >> >
> > >> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> > >> > Signed-off-by: Rajesh Borundia <rajesh.borundia@qlogic.com>
> > >> > Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
> > >> > ---
> > >>
> > >> Hmm, it would be really nice you provide more information for stable
> > >> teams.
> > >>
> > >> If I am not mistaken, bug was introduced in 2.6.36 ?
> > >>
> > > No, It was introduced in 2.6.37, when ETH_FLAG_TXVLAN and
> > ETH_FLAG_RXVLAN introduced.
> > > Yes it should also make into 2.6.37.4 stable kernel.
> >
> > Applied, but 2.6.37.x maintainence has ceased so there is no point
> > worrying about this patch for that stable branch any longer.

2.6.38 needs that fix as well ...

> I thought you will apply patch from Stanislaw Gruszka (net: fix ethtool->set_flags not intended -EINVAL return value), which is more generic and takes care of all drivers.
> 
> David, you are not cc in that patch. I will forward you, in case you miss it.

Patch is here:
http://patchwork.ozlabs.org/patch/88060/

I could also repost rebased patch if needed.

Stanislaw
--
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
amit salecha March 28, 2011, 5:33 a.m. UTC | #5
> >> > netxen_nic_set_flags() rejects data if other flag than
> ETH_FLAG_LRO
> >> is set.
> >> > Driver also supports NETIF_F_HW_VLAN_TX.
> >> > Now compare data with ethtool_op_get_flags(), to get all supported
> >> features.
> >> >
> >> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> >> > Signed-off-by: Rajesh Borundia <rajesh.borundia@qlogic.com>
> >> > Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
> >> > ---
> >>
> >> Hmm, it would be really nice you provide more information for stable
> >> teams.
> >>
> >> If I am not mistaken, bug was introduced in 2.6.36 ?
> >>
> > No, It was introduced in 2.6.37, when ETH_FLAG_TXVLAN and
> ETH_FLAG_RXVLAN introduced.
> > Yes it should also make into 2.6.37.4 stable kernel.
>
> Applied, but 2.6.37.x maintainence has ceased so there is no point
> worrying about this patch for that stable branch any longer.

I thought you will apply patch from Stanislaw Gruszka (net: fix ethtool->set_flags not intended -EINVAL return value), which is more generic and takes care of all drivers.

David, you are not cc in that patch. I will forward you, in case you miss it.

-Amit

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.

--
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
David Miller March 28, 2011, 6:29 a.m. UTC | #6
From: Amit Salecha <amit.salecha@qlogic.com>
Date: Mon, 28 Mar 2011 00:33:43 -0500

> I thought you will apply patch from Stanislaw Gruszka (net: fix
> ethtool->set_flags not intended -EINVAL return value), which is more
> generic and takes care of all drivers.
> 
> David, you are not cc in that patch. I will forward you, in case you miss it.

Ok, I was wondering about that, thanks for explaining I'll fix this up.
--
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
David Miller March 28, 2011, 6:31 a.m. UTC | #7
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Mon, 28 Mar 2011 07:17:46 +0200

> Patch is here:
> http://patchwork.ozlabs.org/patch/88060/
> 
> I could also repost rebased patch if needed.

Not necessary, thanks.
--
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
Marc Haber April 3, 2011, 7:05 p.m. UTC | #8
Hi,

On Mon, Mar 21, 2011 at 03:37:08AM -0700, Amit Kumar Salecha wrote:
> netxen_nic_set_flags() rejects data if other flag than ETH_FLAG_LRO is set.
> Driver also supports NETIF_F_HW_VLAN_TX.
> Now compare data with ethtool_op_get_flags(), to get all supported features.

Could that be the cause for packet loss on kernel 2.6.38.2 if:

  - receiving card is NX3031 [4040:0100]
  - frames are received with VLAN tags
  - large received offload is on.

Packet Loss of this kind is noticed when doing TCP data transfers
towards the host with the Netxen Interface and the TCP session is
terminated on the Netxen host itself. TCP sessions routed through the
Netxen host are not affected.

My ethtool doesn't allow me to influence the LRO setting alone - it is
disabled when I set rx off but doesn't come on again when rx is set to
on again. So, ethtool -K rx off, ethtool -K rx on fixes the issue.

Is this a known bug, maybe with an available patch?

Greetings
Marc
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= April 5, 2011, 10:38 a.m. UTC | #9
2011/4/5 Amit Salecha <amit.salecha@qlogic.com>:
>> On Mon, Mar 21, 2011 at 03:37:08AM -0700, Amit Kumar Salecha wrote:
>> > netxen_nic_set_flags() rejects data if other flag than ETH_FLAG_LRO
>> is set.
>> > Driver also supports NETIF_F_HW_VLAN_TX.
>> > Now compare data with ethtool_op_get_flags(), to get all supported
>> features.
[...]
>> My ethtool doesn't allow me to influence the LRO setting alone - it is
>> disabled when I set rx off but doesn't come on again when rx is set to
>> on again. So, ethtool -K rx off, ethtool -K rx on fixes the issue.
> If rx csum is disabled, LRO will be disable. LRO won't be enabled automatically if you enable rx csum.
> You need to explicitly enable LRO.

This will change once the driver is converted to hw_features.

Best Regards,
Michał Mirosław
--
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
Marc Haber April 5, 2011, 12:41 p.m. UTC | #10
On Tue, Apr 05, 2011 at 12:38:11AM -0500, Amit Salecha wrote:
> > On Mon, Mar 21, 2011 at 03:37:08AM -0700, Amit Kumar Salecha wrote:
> > > netxen_nic_set_flags() rejects data if other flag than ETH_FLAG_LRO
> > is set.
> > > Driver also supports NETIF_F_HW_VLAN_TX.
> > > Now compare data with ethtool_op_get_flags(), to get all supported
> > features.
> >
> > Could that be the cause for packet loss on kernel 2.6.38.2 if:
> >
> >   - receiving card is NX3031 [4040:0100]
> >   - frames are received with VLAN tags
> >   - large received offload is on.
> 
> If ip_forwarding or routing is enable ....then you may see packet loss.

The box is intended to route, so disabling routing is
contraproductive. I just would like to download software updates to
the box with decent speed as well.

> > Packet Loss of this kind is noticed when doing TCP data transfers
> > towards the host with the Netxen Interface and the TCP session is
> > terminated on the Netxen host itself. TCP sessions routed through the
> > Netxen host are not affected.
> >
> > My ethtool doesn't allow me to influence the LRO setting alone - it is
> > disabled when I set rx off but doesn't come on again when rx is set to
> > on again. So, ethtool -K rx off, ethtool -K rx on fixes the issue.
> >
> If rx csum is disabled, LRO will be disable. LRO won't be enabled automatically if you enable rx csum.
> You need to explicitly enable LRO.

Explicitly enabling LRO does not work ("invalid argument", if I recall
correctly).

> > Is this a known bug, maybe with an available patch?
> >
> You need to retest with this patch
> http://patchwork.ozlabs.org/patch/88060/. This patch got applied
> instead of mine.

Will that patch fix the behavior of the interface regarding the packet
loss, or only its connection to ethtool?

Greetings
Marc
amit salecha April 5, 2011, 1:15 p.m. UTC | #11
> On Tue, Apr 05, 2011 at 12:38:11AM -0500, Amit Salecha wrote:

> > > On Mon, Mar 21, 2011 at 03:37:08AM -0700, Amit Kumar Salecha wrote:

> > > > netxen_nic_set_flags() rejects data if other flag than

> ETH_FLAG_LRO

> > > is set.

> > > > Driver also supports NETIF_F_HW_VLAN_TX.

> > > > Now compare data with ethtool_op_get_flags(), to get all

> supported

> > > features.

> > >

> > > Could that be the cause for packet loss on kernel 2.6.38.2 if:

> > >

> > >   - receiving card is NX3031 [4040:0100]

> > >   - frames are received with VLAN tags

> > >   - large received offload is on.

> >

> > If ip_forwarding or routing is enable ....then you may see packet

> loss.

>

> The box is intended to route, so disabling routing is

> contraproductive. I just would like to download software updates to

> the box with decent speed as well.


What I meant, with LRO enable and routing, you may see packet loss.
>

> > > Packet Loss of this kind is noticed when doing TCP data transfers

> > > towards the host with the Netxen Interface and the TCP session is

> > > terminated on the Netxen host itself. TCP sessions routed through

> the

> > > Netxen host are not affected.

> > >

> > > My ethtool doesn't allow me to influence the LRO setting alone - it

> is

> > > disabled when I set rx off but doesn't come on again when rx is set

> to

> > > on again. So, ethtool -K rx off, ethtool -K rx on fixes the issue.

> > >

> > If rx csum is disabled, LRO will be disable. LRO won't be enabled

> automatically if you enable rx csum.

> > You need to explicitly enable LRO.

>

> Explicitly enabling LRO does not work ("invalid argument", if I recall

> correctly).

>


With below patch enabling/disabling LRO will work.

> > > Is this a known bug, maybe with an available patch?

> > >

> > You need to retest with this patch

> > http://patchwork.ozlabs.org/patch/88060/. This patch got applied

> > instead of mine.

>

> Will that patch fix the behavior of the interface regarding the packet

> loss, or only its connection to ethtool?

>

This will fix LRO configuration problem. Do you see packet loss with LRO disable ?

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.
Marc Haber April 5, 2011, 3:18 p.m. UTC | #12
On Tue, Apr 05, 2011 at 08:15:16AM -0500, Amit Salecha wrote:
> This will fix LRO configuration problem. Do you see packet loss with
> LRO disable ?

No, with LRO disabled things seem to be fine. LRO gets enabled by
default though.

Greetings
Marc
stephen hemminger April 5, 2011, 3:46 p.m. UTC | #13
On Tue, 5 Apr 2011 17:18:54 +0200
Marc Haber <mh+netdev@zugschlus.de> wrote:

> On Tue, Apr 05, 2011 at 08:15:16AM -0500, Amit Salecha wrote:
> > This will fix LRO configuration problem. Do you see packet loss with
> > LRO disable ?
> 
> No, with LRO disabled things seem to be fine. LRO gets enabled by
> default though.

LRO and routing are fundamentally incompatible, that is why
the kernel attempts to turn it off. When forwarding packets
should not be combined (end-to-end principle) and that is what
LRO does.

Therefore if doing anything like bridging or forwarding kernel
attempts to get driver to turn LRO off.
--
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
diff mbox

Patch

diff --git a/drivers/net/netxen/netxen_nic_ethtool.c b/drivers/net/netxen/netxen_nic_ethtool.c
index 653d308..31c41d6 100644
--- a/drivers/net/netxen/netxen_nic_ethtool.c
+++ b/drivers/net/netxen/netxen_nic_ethtool.c
@@ -871,7 +871,8 @@  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_op_get_flags(netdev) & ~ETH_FLAG_LRO) !=
+	    (data & ~ETH_FLAG_LRO))
 		return -EINVAL;
 
 	if (!(adapter->capabilities & NX_FW_CAPABILITY_HW_LRO))