net/usb/ax88179_178a driver broken in linux-3.12

The USB3 network adapter locks up consistently for me here in 3.12, but was working without issues in 3.11.x

Source of the problem is this patch:

   http://patchwork.ozlabs.org/patch/264021/

Reverting the patch fixes the adapter.

Quoted 7 lines · Mark Lord

The USB3 network adapter locks up consistently for me here in 3.12, but was working without issues in 3.11.x

Source of the problem is this patch:

   http://patchwork.ozlabs.org/patch/264021/

Reverting the patch fixes the adapter.

Okay, upon closer inspection, the bug appears to be a math error. Here's a simpler (non-revert) patch to fix the bug that was introduced in 3.12.

(un-mangled copy attached; reproduced below for ease of viewing).

Don't exceed more than the 8 bytes of reserved ("needed_headroom") space when calling pskb_expand_head. This fixes a bug introduced in linux-3.12.0.

Signed-off-by Mark Lord <[email protected]>

drivers/net/usb/ax88179_178a.c 2013-11-17 13:47:26.127971404 -0500 renamed from ax88179_178a.c.orig 2013-11-03 18:41:51.000000000 -0500 +0 -0

@@ -1183,10 +1183,10 @@
        if (((skb->len + 8) % frame_size) == 0)
                tx_hdr2 |= 0x80008000;  /* Enable padding */
 

Quoted 3 lines · Mark Lord

Quoted 2 lines

The USB3 network adapter locks up consistently for me here in 3.12, but was working without issues in 3.11.x

From: David Laight

Quoted 4 lines · Mark Lord

Quoted 3 lines · Mark Lord

Quoted 2 lines

The USB3 network adapter locks up consistently for me here in 3.12, but was working without issues in 3.11.x

The xhci driver is well broken in 3.12.

Quoted 7 lines

From: David Laight

Quoted 4 lines · Mark Lord

Quoted 3 lines · Mark Lord

Quoted 2 lines

The USB3 network adapter locks up consistently for me here in 3.12, but was working without issues in 3.11.x

The xhci driver is well broken in 3.12.

To correct myself...

The xhci driver has never correctly support scatter-gather requests. In 3.12 code was added to usbnet to generate SG transmits, and to the ax88179_178a driver to use them.
TCP segmentation offload was then enabled - with does generate SG transfers.

SG transfers for 'disks' almost certainly work because the fragment boundaries are 'adequately aligned'.

David

From: Mark Lord

Quoted 16 lines · David Laight

Quoted 7 lines

From: David Laight

Quoted 4 lines · Mark Lord

Quoted 3 lines · Mark Lord

Quoted 2 lines

The USB3 network adapter locks up consistently for me here in 3.12, but was working without issues in 3.11.x

The xhci driver is well broken in 3.12.

To correct myself...

The xhci driver has never correctly support scatter-gather requests. In 3.12 code was added to usbnet to generate SG transmits, and to the ax88179_178a driver to use them.
TCP segmentation offload was then enabled - with does generate SG transfers.

Quoted 5 lines

SG transfers for 'disks' almost certainly work because the fragment boundaries are 'adequately aligned'.

David

Well, that's all very nice and whatnot,
except the ax88179_178a driver still does not work in linux-3.12, whereas it works fine in all earlier kernels.

That's a regression.
And a simple revert (earlier in this thread) fixes it.

So.. let's revert it for now, until a proper xhci compatible patch is produced.

Which changes did you revert?

From: Mark Lord [mailto:[email protected]]

Quoted 2 lines · David Laight

Which changes did you revert?

Just the bits that changed how the headroom/tailroom sizes were checked and adjusted. See attachment for the revert patch I am using here. My mailer unfortunately likes to mangle inline patches.

=========snip===========
Revert USB 3.0 network driver changes that break the adapter (lockups) in 3.12. This just puts back the original code from previous kernels.

Signed-off-by Mark Lord <[email protected]>

drivers/net/usb/ax88179_178a.c 2013-11-17 13:23:39.525734277 -0500 renamed from drivers/net/usb/ax88179_178a.c.orig 2013-11-03 18:41:51.000000000 -0500 +12 -3

@@ -1177,18 +1177,31 @@
        int frame_size = dev->maxpacket;
        int mss = skb_shinfo(skb)->gso_size;
        int headroom;
+       int tailroom;
 
        tx_hdr1 = skb->len;
        tx_hdr2 = mss;
        if (((skb->len + 8) % frame_size) == 0)
                tx_hdr2 |= 0x80008000;  /* Enable padding */
 
-       headroom = skb_headroom(skb) - 8;
+       headroom = skb_headroom(skb);
+       tailroom = skb_tailroom(skb);
 
-       if ((skb_header_cloned(skb) || headroom < 0) &&
-           pskb_expand_head(skb, headroom < 0 ? 8 : 0, 0, GFP_ATOMIC)) {
+       if (!skb_header_cloned(skb) &&
+           !skb_cloned(skb) &&
+           (headroom + tailroom) >= 8) {
+               if (headroom < 8) {
+                       skb->data = memmove(skb->head + 8, skb->data, skb->len);
+                       skb_set_tail_pointer(skb, skb->len);
+               }
+       } else {
+               struct sk_buff *skb2;

Quoted 1 line

From: Mark Lord

Quoted 6 lines

except the ax88179_178a driver still does not work in linux-3.12, whereas it works fine in all earlier kernels.

That's a regression.
And a simple revert (earlier in this thread) fixes it.
So.. let's revert it for now, until a proper xhci compatible patch is produced.

There is a patch to xhci-ring.c that should fix the SG problem.
http://www.spinics.net/lists/linux-usb/msg97176.html

I think it should apply to the 3.12 sources.

Quoted 1 line · David Laight

Quoted 1 line

From: Mark Lord

..

Quoted 6 lines

Quoted 6 lines

except the ax88179_178a driver still does not work in linux-3.12, whereas it works fine in all earlier kernels.

That's a regression.
And a simple revert (earlier in this thread) fixes it.
So.. let's revert it for now, until a proper xhci compatible patch is produced.

...

Quoted 4 lines

There is a patch to xhci-ring.c that should fix the SG problem.
http://www.spinics.net/lists/linux-usb/msg97176.html

I think it should apply to the 3.12 sources.

I am running with that patch here now (thanks),
and it too appears to prevent the lockups.

But is this patch upstream already?
If yes, then it needs to get pushed out to -stable for 3.12 at least.

If not upstream, then the revert is probably safest for -stable, rather than new code that has never been upstream before.

Both patches are attached to this email.
One or the other is required for the USB 3.0 network adapters to function in 3.12.

Quoted 24 lines · Mark Lord

Quoted 1 line · David Laight

Quoted 1 line

From: Mark Lord

..

Quoted 6 lines

Quoted 6 lines

except the ax88179_178a driver still does not work in linux-3.12, whereas it works fine in all earlier kernels.

That's a regression.
And a simple revert (earlier in this thread) fixes it.
So.. let's revert it for now, until a proper xhci compatible patch is produced.

...

Quoted 4 lines

There is a patch to xhci-ring.c that should fix the SG problem.
http://www.spinics.net/lists/linux-usb/msg97176.html

I think it should apply to the 3.12 sources.

I am running with that patch here now (thanks),
and it too appears to prevent the lockups.

But is this patch upstream already?
If yes, then it needs to get pushed out to -stable for 3.12 at least.

If not upstream, then the revert is probably safest for -stable, rather than new code that has never been upstream before.

Quoted 2 lines

Both patches are attached to this email.
One or the other is required for the USB 3.0 network adapters to function in 3.12.

I do not see any error in commit f27070158d6754765f2 ("ax88179_178a: avoid copy of tx tcp packets") Quite the contrary in fact...

I suspect a TSO bug, and would rather disable TSO for this nic.

From: Eric Dumazet [mailto:[email protected]]

Quoted 5 lines · Mark Lord

Quoted 1 line · David Laight

Quoted 1 line

From: Mark Lord

..

Quoted 2 lines

Quoted 2 lines

except the ax88179_178a driver still does not work in linux-3.12, whereas it works fine in all earlier kernels.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

That's a regression.
And a simple revert (earlier in this thread) fixes it.
So.. let's revert it for now, until a proper xhci compatible patch is produced.

...

Quoted 3 lines

There is a patch to xhci-ring.c that should fix the SG problem.
http://www.spinics.net/lists/linux-usb/msg97176.html

Quoted 7 lines

Quoted 1 line

I think it should apply to the 3.12 sources.

I am running with that patch here now (thanks),
and it too appears to prevent the lockups.

But is this patch upstream already?
If yes, then it needs to get pushed out to -stable for 3.12 at least.

Quoted 6 lines

If not upstream, then the revert is probably safest for -stable, rather than new code that has never been upstream before.

Both patches are attached to this email.
One or the other is required for the USB 3.0 network adapters to function in 3.12.

I do not see any error in commit f27070158d6754765f2 ("ax88179_178a: avoid copy of tx tcp packets")

Quite the contrary in fact...

I suspect a TSO bug, and would rather disable TSO for this nic.

Have you tried to revert 3804fad45411b482
("USBNET: ax88179_178a: enable tso if usb host supports sg dma")

It isn't directly a TSO problem.
There has always been a bug in the xhci driver for fragmented buffers. TSO just means it is given a lot of fragmented buffers.

As well as user-supplied fragmented buffers, the bug affects internal fragmentation that happens whenever a buffer crosses a 64k byte boundary (please hw engineers - stop doing this!)

I'm not sure whether usbnet would ever pass buffers that cross 64k boundaries. I've not seen one - even with TSO. But the rx buffers are 20k (doesn't seem ideal!) and could also be problematical.

USB mass storage has used SG for ages, the buffers must all be adequately aligned for the hardware - they won't meet the constraint for USB3 itself, but the documented restriction may be more severe than the actual one.

From: Eric Dumazet [mailto:[email protected]]

Quoted 7 lines · David Laight

It isn't directly a TSO problem.
There has always been a bug in the xhci driver for fragmented buffers. TSO just means it is given a lot of fragmented buffers.

As well as user-supplied fragmented buffers, the bug affects internal fragmentation that happens whenever a buffer crosses a 64k byte boundary (please hw engineers - stop doing this!)

TCP stack uses order-3 allocations.

This means 32KB for x86 (PAGE_SIZE=4096)

What is PAGE_SIZE for your arches ?

If there is a 6KB limit, we might adapt TCP to make sure we do not cross a 64KB boundary.

Other strategy would be to detect this case in the driver and split a problematic segment into two parts.

There is a patch to xhci-ring.c that should fix the SG problem.
http://www.spinics.net/lists/linux-usb/msg97176.html

I think it should apply to the 3.12 sources.

From: Of David Miller

Quoted 7 lines

From: "David Laight" <[email protected]>
Date: Tue, 19 Nov 2013 10:04:11 -0000

There is a patch to xhci-ring.c that should fix the SG problem.
http://www.spinics.net/lists/linux-usb/msg97176.html

I think it should apply to the 3.12 sources.

David, please get this into Linus's tree and queued up for -stable as soon as possible, thank you.

Quoted 7 lines

From: Of David Miller

Quoted 6 lines

From: "David Laight" <[email protected]>
Date: Tue, 19 Nov 2013 10:04:11 -0000

There is a patch to xhci-ring.c that should fix the SG problem.
http://www.spinics.net/lists/linux-usb/msg97176.html

Quoted 4 lines

Quoted 1 line

I think it should apply to the 3.12 sources.

David, please get this into Linus's tree and queued up for -stable as soon as possible, thank you.

What do I have to do to expedite it?
I thought that Sarah would have to push the patch through.

It might be necessary to limit the number of SG fragments that the mass storage (etc) can generate to 63 (from unlimited). That should be a 1 line change somewhere.

Quoted 2 lines · David Laight

Which changes did you revert?

Just the bits that changed how the headroom/tailroom sizes were checked and adjusted. See attachment for the revert patch I am using here. My mailer unfortunately likes to mangle inline patches.

=========snip===========
Revert USB 3.0 network driver changes that break the adapter (lockups) in 3.12. This just puts back the original code from previous kernels.

Signed-off-by Mark Lord <[email protected]>

drivers/net/usb/ax88179_178a.c 2013-11-17 13:23:39.525734277 -0500 renamed from drivers/net/usb/ax88179_178a.c.orig 2013-11-03 18:41:51.000000000 -0500 +12 -3

@@ -1177,18 +1177,31 @@
        int frame_size = dev->maxpacket;
        int mss = skb_shinfo(skb)->gso_size;
        int headroom;
+       int tailroom;
 
        tx_hdr1 = skb->len;
        tx_hdr2 = mss;
        if (((skb->len + 8) % frame_size) == 0)
                tx_hdr2 |= 0x80008000;  /* Enable padding */
 
-       headroom = skb_headroom(skb) - 8;
+       headroom = skb_headroom(skb);
+       tailroom = skb_tailroom(skb);
 
-       if ((skb_header_cloned(skb) || headroom < 0) &&
-           pskb_expand_head(skb, headroom < 0 ? 8 : 0, 0, GFP_ATOMIC)) {
+       if (!skb_header_cloned(skb) &&
+           !skb_cloned(skb) &&
+           (headroom + tailroom) >= 8) {
+               if (headroom < 8) {
+                       skb->data = memmove(skb->head + 8, skb->data, skb->len);
+                       skb_set_tail_pointer(skb, skb->len);
+               }
+       } else {
+               struct sk_buff *skb2;

Quoted 16 lines

From: Mark Lord
Sent: 30 November 2013 02:59
To: Sarah Sharp; David Laight
Cc: David Miller; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: net/usb/ax88179_178a driver broken in linux-3.12

Quoted 8 lines · Mark Lord

Quoted 2 lines · David Laight

Which changes did you revert?

Just the bits that changed how the headroom/tailroom sizes were checked and adjusted. See attachment for the revert patch I am using here. My mailer unfortunately likes to mangle inline patches.

...

Quoted 3 lines

Two kernels later, and this regression has still not been fixed.

Quoted 1 line

A simple revert, folks.

Reverting the ax88179_178a driver doesn't fix the problem. I'm seen tx/rx issues with it on much older kernels that don't appear with a current kernel and the fixed xhci code.

Sarah needs to feed the xhci_ring.c fix through into stable.

Quoted 26 lines · David Laight

Quoted 16 lines

From: Mark Lord
Sent: 30 November 2013 02:59
To: Sarah Sharp; David Laight
Cc: David Miller; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: net/usb/ax88179_178a driver broken in linux-3.12

Quoted 8 lines · Mark Lord

Quoted 2 lines · David Laight

Which changes did you revert?

Just the bits that changed how the headroom/tailroom sizes were checked and adjusted. See attachment for the revert patch I am using here. My mailer unfortunately likes to mangle inline patches.

...

Quoted 4 lines

Two kernels later, and this regression has still not been fixed.

A simple revert, folks.

Reverting the ax88179_178a driver doesn't fix the problem. I'm seen tx/rx issues with it on much older kernels that don't appear with a current kernel and the fixed xhci code.

Quoted 1 line

Sarah needs to feed the xhci_ring.c fix through into stable.

Oh, I agree. But Linus makes a MASSIVE distinction between things that have always been broken, and REGRESSIONS from the immediately prior kernel that can be tracked to a single update.

Let's see some action, here folks!

Quoted 1 line · David Laight

Quoted 1 line

Quoted 1 line

Sarah needs to feed the xhci_ring.c fix through into stable.

I'm working on it. You will probably have to wait for -rc3, depending on when Greg sends his next pull request. I will Cc you on the pull request and patch, which should be sent out today.

Quoted 33 lines · Mark Lord

Quoted 20 lines · David Laight

Quoted 16 lines

From: Mark Lord
Sent: 30 November 2013 02:59
To: Sarah Sharp; David Laight
Cc: David Miller; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: net/usb/ax88179_178a driver broken in linux-3.12

Quoted 8 lines · Mark Lord

Quoted 2 lines · David Laight

Which changes did you revert?

Just the bits that changed how the headroom/tailroom sizes were checked and adjusted. See attachment for the revert patch I am using here. My mailer unfortunately likes to mangle inline patches.

...

Quoted 3 lines

Two kernels later, and this regression has still not been fixed.

Quoted 7 lines

Quoted 1 line

A simple revert, folks.

Reverting the ax88179_178a driver doesn't fix the problem. I'm seen tx/rx issues with it on much older kernels that don't appear with a current kernel and the fixed xhci code.

Sarah needs to feed the xhci_ring.c fix through into stable.

Oh, I agree. But Linus makes a MASSIVE distinction between things that have always been broken, and REGRESSIONS from the immediately prior kernel that can be tracked to a single update.

Quoted 1 line

Let's see some action, here folks!

I'm working on it. You will probably have to wait for -rc3, depending on when Greg sends his next pull request. I will Cc you on the pull request and patch, which should be sent out today.