Re: net/usb/ax88179_178a driver broken in linux-3.12

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

On 13-11-17 01:35 PM, Mark Lord wrote:
> 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 <mlord@pobox.com>


--- ax88179_178a.c.orig 2013-11-03 18:41:51.000000000 -0500
+++ linux/drivers/net/usb/ax88179_178a.c        2013-11-17 13:47:26.127971404 -0500
@@ -1183,10 +1183,10 @@
        if (((skb->len + 8) % frame_size) == 0)
                tx_hdr2 |= 0x80008000;  /* Enable padding */

-       headroom = skb_headroom(skb) - 8;
+       headroom = skb_headroom(skb);

-       if ((skb_header_cloned(skb) || headroom < 0) &&
-           pskb_expand_head(skb, headroom < 0 ? 8 : 0, 0, GFP_ATOMIC)) {
+       if ((skb_header_cloned(skb) || headroom < 8) &&
+           pskb_expand_head(skb, headroom < 8 ? 8 - headroom : 0, 0, GFP_ATOMIC)) {
                dev_kfree_skb_any(skb);
                return NULL;
        }

-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[-- Attachment #2: 51_ax88179_178a_fix_3.12_lockups.patch --]
[-- Type: text/x-patch, Size: 811 bytes --]

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 <mlord@pobox.com>

--- ax88179_178a.c.orig	2013-11-03 18:41:51.000000000 -0500
+++ linux/drivers/net/usb/ax88179_178a.c	2013-11-17 13:47:26.127971404 -0500
@@ -1183,10 +1183,10 @@
 	if (((skb->len + 8) % frame_size) == 0)
 		tx_hdr2 |= 0x80008000;	/* Enable padding */
 
-	headroom = skb_headroom(skb) - 8;
+	headroom = skb_headroom(skb);
 
-	if ((skb_header_cloned(skb) || headroom < 0) &&
-	    pskb_expand_head(skb, headroom < 0 ? 8 : 0, 0, GFP_ATOMIC)) {
+	if ((skb_header_cloned(skb) || headroom < 8) &&
+	    pskb_expand_head(skb, headroom < 8 ? 8 - headroom : 0, 0, GFP_ATOMIC)) {
 		dev_kfree_skb_any(skb);
 		return NULL;
 	}

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

On 13-11-17 01:56 PM, Mark Lord wrote:
> On 13-11-17 01:35 PM, Mark Lord wrote:
>> 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 <mlord@pobox.com>
> 
> 
> --- ax88179_178a.c.orig 2013-11-03 18:41:51.000000000 -0500
> +++ linux/drivers/net/usb/ax88179_178a.c        2013-11-17 13:47:26.127971404 -0500
> @@ -1183,10 +1183,10 @@
>         if (((skb->len + 8) % frame_size) == 0)
>                 tx_hdr2 |= 0x80008000;  /* Enable padding */
> 
> -       headroom = skb_headroom(skb) - 8;
> +       headroom = skb_headroom(skb);
> 
> -       if ((skb_header_cloned(skb) || headroom < 0) &&
> -           pskb_expand_head(skb, headroom < 0 ? 8 : 0, 0, GFP_ATOMIC)) {
> +       if ((skb_header_cloned(skb) || headroom < 8) &&
> +           pskb_expand_head(skb, headroom < 8 ? 8 - headroom : 0, 0, GFP_ATOMIC)) {
>                 dev_kfree_skb_any(skb);
>                 return NULL;
>         }
> 

Dagnabbit.. that still locked up just after I sent the email,
despite working fine in testing prior to that.

So.. sticking with the full revert here for now.
Perhaps one of you chaps can spot the bug.

-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> On 13-11-17 01:56 PM, Mark Lord wrote:
> > On 13-11-17 01:35 PM, Mark Lord wrote:
> >> 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.

Try disabling GSO and TSO so that the xhci driver is never given
fragments.

	David

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> From: David Laight
> > On 13-11-17 01:56 PM, Mark Lord wrote:
> > > On 13-11-17 01:35 PM, Mark Lord wrote:
> > >> 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

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

On 13-11-18 08:32 AM, David Laight wrote:
>> From: David Laight
>>> On 13-11-17 01:56 PM, Mark Lord wrote:
>>>> On 13-11-17 01:35 PM, Mark Lord wrote:
>>>>> 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
> 

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.
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> From: Mark Lord
> On 13-11-18 08:32 AM, David Laight wrote:
> >> From: David Laight
> >>> On 13-11-17 01:56 PM, Mark Lord wrote:
> >>>> On 13-11-17 01:35 PM, Mark Lord wrote:
> >>>>> 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
> >
> 
> 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?
I think you must have gone right back to the version that doesn't so scatter-gather.

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.

Note that with older kernels the asix card I have will drop/discard
short bursts of tx/rx packets. I've not seen that with 3.12 and the
xhci-ring.c patch.

	David

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

On 13-11-19 05:04 AM, David Laight wrote:
>
> 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 <mlord@pobox.com>

--- linux/drivers/net/usb/ax88179_178a.c.orig   2013-11-03 18:41:51.000000000 -0500
+++ linux/drivers/net/usb/ax88179_178a.c        2013-11-17 13:23:39.525734277 -0500
@@ -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;
+
+               skb2 = skb_copy_expand(skb, 8, 0, flags);
                dev_kfree_skb_any(skb);
-               return NULL;
+               skb = skb2;
+               if (!skb)
+                       return NULL;
        }

        skb_push(skb, 4);
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[-- Attachment #2: 51_ax88179_178a_revert_3.12_lockups.patch --]
[-- Type: text/x-patch, Size: 1250 bytes --]

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 <mlord@pobox.com>

--- linux/drivers/net/usb/ax88179_178a.c.orig	2013-11-03 18:41:51.000000000 -0500
+++ linux/drivers/net/usb/ax88179_178a.c	2013-11-17 13:23:39.525734277 -0500
@@ -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;
+
+		skb2 = skb_copy_expand(skb, 8, 0, flags);
 		dev_kfree_skb_any(skb);
-		return NULL;
+		skb = skb2;
+		if (!skb)
+			return NULL;
 	}
 
 	skb_push(skb, 4);

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> From: Mark Lord [mailto:mlord@pobox.com]
> On 13-11-19 05:04 AM, David Laight wrote:
> >
> > 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 <mlord@pobox.com>
> 
> --- linux/drivers/net/usb/ax88179_178a.c.orig   2013-11-03 18:41:51.000000000 -0500
> +++ linux/drivers/net/usb/ax88179_178a.c        2013-11-17 13:23:39.525734277 -0500
> @@ -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;
> +
> +               skb2 = skb_copy_expand(skb, 8, 0, flags);
>                 dev_kfree_skb_any(skb);
> -               return NULL;
> +               skb = skb2;
> +               if (!skb)
> +                       return NULL;
>         }
> 
>         skb_push(skb, 4);

At a guess it will be a difference between pskb_expand_head() and
skb_copy_expand(). The latter probably generates a linear skb.
The extra check for skb_cloned() probably also applies - and forces
a data copy.

Have you tried the patch to xhci-ring.c?

	David

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 1035 bytes --]

On 13-11-19 05:04 AM, David Laight wrote:
>> From: Mark Lord
..
>> 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.

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.

Thanks
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[-- Attachment #2: 51_ax88179_178a_revert_3.12_lockups.patch --]
[-- Type: text/x-patch, Size: 1250 bytes --]

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 <mlord@pobox.com>

--- linux/drivers/net/usb/ax88179_178a.c.orig	2013-11-03 18:41:51.000000000 -0500
+++ linux/drivers/net/usb/ax88179_178a.c	2013-11-17 13:23:39.525734277 -0500
@@ -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;
+
+		skb2 = skb_copy_expand(skb, 8, 0, flags);
 		dev_kfree_skb_any(skb);
-		return NULL;
+		skb = skb2;
+		if (!skb)
+			return NULL;
 	}
 
 	skb_push(skb, 4);

[-- Attachment #3: 55_usb_xhci_TRB_scatter_gather_fix.patch --]
[-- Type: text/x-patch, Size: 3078 bytes --]

Section 4.11.7.1 of rev 1.0 of the xhci specification states that a link TRB
can only occur at a boundary between underlying USB frames (512 bytes for 480M).

If this isn't done the USB frames aren't formatted correctly and, for example,
the USB3 ethernet ax88179_178a card will stop sending (while still receiving)
when running a netperf tcp transmit test with (say) and 8k buffer.

This should be a candidate for stable, the ax88179_178a driver defaults to
gso and tso enabled so it passes a lot of fragmented skb to the USB stack.

Signed-off-by: David Laight &lt;david.laight@xxxxxxxxxx&gt;
---

Changes for v2:

1) Only act on bulk endpoints.
   While isoc endpoints could suffer from the same problem it is much less
   likely and can't be fixed by adding NOP TRBs (they would stop data being
   sent in the poll interval).

2) When writing the NOP TRB use the count of TRBs instead of scanning for
   the link TRB.

 drivers/usb/host/xhci-ring.c | 53 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 5480215..c1342dc 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2929,8 +2929,57 @@
 	}
 
 	while (1) {
-		if (room_on_ring(xhci, ep_ring, num_trbs))
-			break;
+		if (room_on_ring(xhci, ep_ring, num_trbs)) {
+			union xhci_trb *trb = ep_ring->enqueue;
+			unsigned int usable = ep_ring->enq_seg->trbs +
+					TRBS_PER_SEGMENT - 1 - trb;
+			u32 nop_cmd;
+
+			/*
+			 * Section 4.11.7.1 TD Fragments states that a link
+			 * TRB must only occur at the boundary between
+			 * data bursts (eg 512 bytes for 480M).
+			 * While it is possible to split a large fragment
+			 * we don't know the size yet.
+			 * Simplest solution is to fill the trb before the
+			 * LINK with nop commands.
+			 */
+			if (num_trbs == 1 || num_trbs <= usable || usable == 0)
+				break;
+
+			if (ep_ring->type != TYPE_BULK)
+				/*
+				 * While isoc transfers might have a buffer that
+				 * crosses a 64k boundary it is unlikely.
+				 * Since we can't add NOPs without generating
+				 * gaps in the traffic just hope it never
+				 * happens at the end of the ring.
+				 * This could be fixed by writing a LINK TRB
+				 * instead of the first NOP - however the
+				 * TRB_TYPE_LINK_LE32() calls would all need
+				 * changing to check the ring length. */
+				break;
+
+			if (num_trbs >= TRBS_PER_SEGMENT) {
+				xhci_err(xhci, "Too many fragments %d, max %d\n",
+						num_trbs, TRBS_PER_SEGMENT - 1);
+				return -ENOMEM;
+			}
+
+			nop_cmd = cpu_to_le32(TRB_TYPE(TRB_TR_NOOP) |
+					ep_ring->cycle_state);
+			ep_ring->num_trbs_free -= usable;
+			do {
+				trb->generic.field[0] = 0;
+				trb->generic.field[1] = 0;
+				trb->generic.field[2] = 0;
+				trb->generic.field[3] = nop_cmd;
+				trb++;
+			} while (--usable);
+			ep_ring->enqueue = trb;
+			if (room_on_ring(xhci, ep_ring, num_trbs))
+				break;
+		}
 
 		if (ep_ring == xhci->cmd_ring) {
 			xhci_err(xhci, "Do not support expand command ring\n");

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Eric Dumazet]

On Tue, 2013-11-19 at 09:02 -0500, Mark Lord wrote:
> On 13-11-19 05:04 AM, David Laight wrote:
> >> From: Mark Lord
> ..
> >> 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.
> 
> 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.

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

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

On 13-11-19 09:15 AM, Eric Dumazet wrote:
> On Tue, 2013-11-19 at 09:02 -0500, Mark Lord wrote:
>> On 13-11-19 05:04 AM, David Laight wrote:
>>>> From: Mark Lord
>> ..
>>>> 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.
>>
>> 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.
> 
> 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.

David's explanation for the XHCI issue seems to explain it nicely,
and the patch he linked to does indeed address/fix the issue,
without disabling TSO.

So on the evidence, probably NOT a TSO bug.
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> On Tue, 2013-11-19 at 09:02 -0500, Mark Lord wrote:
> > On 13-11-19 05:04 AM, David Laight wrote:
> > >> From: Mark Lord
> > ..
> > >> except the ax88179_178a driver still does not work in linux-3.12,
> > >> whereas it works fine in all earlier kernels.

I've seen lost packets in IIRC 3.2

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

Having someone else confirm that there is a bug, and that the
patch fixes it should help it get pushed to -stable.

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

	David

RE: net/usb/ax88179_178a driver broken in linux-3.12

[Eric Dumazet]

On Tue, 2013-11-19 at 14:43 +0000, David Laight wrote:

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

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

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> On Tue, 2013-11-19 at 14:43 +0000, David Laight wrote:
> 
> > 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.

The xhci code does all the checks for buffer fragments crossing 64k boundaries.
I've posted a patch that uses a conservative upper limit for the
number of fragments: 2 * number_of_sg_buffs + xfer_len/65536.
This saves scanning the sg list twice.

For those not reading linux-usb:

The xhci 'bug' is that an SG list may only cross the end of a ring
segment at an aligned length.
For USB2 devices this will be 512 bytes. For USB3 the documented alignment
could be 16k (depends on a burst size), but might only be 1k.
(This is another place where the hw engineers haven't made life easy.)
The only simple solution is to ensure that a SG list doesn't cross
the end of a ring segment.

	David

Re: net/usb/ax88179_178a driver broken in linux-3.12

[David Miller]

From: "David Laight" <David.Laight@ACULAB.COM>
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.

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> From: Of David Miller
> > From: "David Laight" <David.Laight@ACULAB.COM>
> > 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.

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.

	David

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Sarah Sharp]

On Wed, Nov 20, 2013 at 09:54:02AM -0000, David Laight wrote:
> > From: Of David Miller
> > > From: "David Laight" <David.Laight@ACULAB.COM>
> > > 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.
> 
> What do I have to do to expedite it?
> I thought that Sarah would have to push the patch through.

Yes, I think the patch should come through my tree.

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

Greg's USB tree is frozen until after 3.13-rc1.  The patch will have to
wait until then, but it will be marked for stable.

I have no objection to the methodology of the patch, but we do need to
figure out how to limit the number of scatter-gather list entries in the
mass storage driver.  We're still figuring out the exact limitation we
need to use, and then the patch can be queued in my for-usb-linus branch
for 3.13-rc1.

Note that the patch is a bandaid fix, and will only help bulk endpoints
that are submitting scatter-gather transfers.  Interrupt endpoints using
scatter-gather will still run into these issues, but with David's patch,
the URB submission will now fail, rather than allowing the transfer to
be silently corrupted.

The real fix for this will probably be too large for stable, and is
likely to take at least a kernel revision to fix.

Sarah Sharp

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

On 13-11-19 08:44 AM, Mark Lord wrote:
> On 13-11-19 05:04 AM, David Laight wrote:
>>
>> 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 <mlord@pobox.com>
> 
> --- linux/drivers/net/usb/ax88179_178a.c.orig   2013-11-03 18:41:51.000000000 -0500
> +++ linux/drivers/net/usb/ax88179_178a.c        2013-11-17 13:23:39.525734277 -0500
> @@ -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;
> +
> +               skb2 = skb_copy_expand(skb, 8, 0, flags);
>                 dev_kfree_skb_any(skb);
> -               return NULL;
> +               skb = skb2;
> +               if (!skb)
> +                       return NULL;
>         }
> 
>         skb_push(skb, 4);
> 


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

A simple revert, folks.
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

RE: net/usb/ax88179_178a driver broken in linux-3.12

[David Laight]

> From: Mark Lord
> Sent: 30 November 2013 02:59
> To: Sarah Sharp; David Laight
> Cc: David Miller; eric.dumazet@gmail.com; ming.lei@canonical.com; netdev@vger.kernel.org; linux-
> usb@vger.kernel.org
> Subject: Re: net/usb/ax88179_178a driver broken in linux-3.12
> 
> On 13-11-19 08:44 AM, Mark Lord wrote:
> > On 13-11-19 05:04 AM, David Laight wrote:
> >>
> >> 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.
> >
...
> 
> 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.

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

	David

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

On 13-12-02 04:30 AM, David Laight wrote:
>> From: Mark Lord
>> Sent: 30 November 2013 02:59
>> To: Sarah Sharp; David Laight
>> Cc: David Miller; eric.dumazet@gmail.com; ming.lei@canonical.com; netdev@vger.kernel.org; linux-
>> usb@vger.kernel.org
>> Subject: Re: net/usb/ax88179_178a driver broken in linux-3.12
>>
>> On 13-11-19 08:44 AM, Mark Lord wrote:
>>> On 13-11-19 05:04 AM, David Laight wrote:
>>>>
>>>> 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.
>>>
> ...
>>
>> 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.
> 
> 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!
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Sarah Sharp]

On Mon, Dec 02, 2013 at 10:05:54AM -0500, Mark Lord wrote:
> On 13-12-02 04:30 AM, David Laight wrote:
> >> From: Mark Lord
> >> Sent: 30 November 2013 02:59
> >> To: Sarah Sharp; David Laight
> >> Cc: David Miller; eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> >> usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: net/usb/ax88179_178a driver broken in linux-3.12
> >>
> >> On 13-11-19 08:44 AM, Mark Lord wrote:
> >>> On 13-11-19 05:04 AM, David Laight wrote:
> >>>>
> >>>> 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.
> >>>
> > ...
> >>
> >> 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.
> > 
> > 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!

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.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Mark Lord]

On 13-12-02 02:08 PM, Sarah Sharp wrote:
>> On 13-12-02 04:30 AM, David Laight wrote:
..
>>> 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.

Super, exactly what we need, thanks!

-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

Re: net/usb/ax88179_178a driver broken in linux-3.12

[Greg KH]

On Mon, Dec 02, 2013 at 11:08:36AM -0800, Sarah Sharp wrote:
> On Mon, Dec 02, 2013 at 10:05:54AM -0500, Mark Lord wrote:
> > On 13-12-02 04:30 AM, David Laight wrote:
> > >> From: Mark Lord
> > >> Sent: 30 November 2013 02:59
> > >> To: Sarah Sharp; David Laight
> > >> Cc: David Miller; eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> > >> usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > >> Subject: Re: net/usb/ax88179_178a driver broken in linux-3.12
> > >>
> > >> On 13-11-19 08:44 AM, Mark Lord wrote:
> > >>> On 13-11-19 05:04 AM, David Laight wrote:
> > >>>>
> > >>>> 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.
> > >>>
> > > ...
> > >>
> > >> 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.
> > > 
> > > 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!
> 
> 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.

It's impossible to get into -rc2 given that it was released last week :)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html