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: 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]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2585 bytes --]

> 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



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1674 bytes --]

> 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


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥