[PATCH 0/4] asix: Fix support for C1 DUB-E100

[PATCH 0/4] asix: Fix support for C1 DUB-E100

[Dean Jenkins]

A simple ping test shows that support for the C1 DUB-E100 is broken.

ping -c 200 -s 1965 -f 172.17.0.10

The -s 1965 forces the USB Ethernet adaptor to use a worse case 2049 octet
bulk transfer due to the transfer containing 2 Ethernet frames. Unfortunately
the URB buffer is 2048 bytes long so the last byte of the 2nd Ethernet frame
is lost.

The missing byte causes a "Bad Header" error message to be generated during 
the processing of the next socket buffer. However, the truncated frame has 1
byte erroneously appended from the next socket buffer and this now corrupted
RX frame is passed up to the network layer. Worse, the next socket buffer is
discarded with the probable loss of a good Ethernet frame.

The patchset consists of the following 4 patches which should apply cleanly
to the net-next git repo baseline at 5824d2d:

Dean Jenkins (4):
      asix: Rename remaining and size for clarity
      asix: Tidy-up asix_rx_fixup_internal() logic
      asix: On RX avoid creating bad Ethernet frames
      asix: C1 DUB-E100 double rx_urb_size to 4096

 drivers/net/usb/asix.h         |    2 +-
 drivers/net/usb/asix_common.c  |  104 ++++++++++++++++++++++++----------------
 drivers/net/usb/asix_devices.c |    5 +-
 3 files changed, 69 insertions(+), 42 deletions(-)

Summary of patches:

1. asix: Rename remaining and size for clarity

This patch effectively swaps the variables "remaining" and "size" to make the
code more understandable. A new local variable "copy_length" is added as the
transfer length from the socket buffer to the netdev buffer.

2. asix: Tidy-up asix_rx_fixup_internal() logic

This patch does a tidy-up of the asix_rx_fixup_internal() logic to remove
redundant logic. The tidy-up improves the implementation of the logic but
does not "fix" a logic flaw. In other words, the original code has redundant
logic.

3. asix: On RX avoid creating bad Ethernet frames

This patch prevents creation of a bad Ethernet frame in the netdev buffer
when the "Bad Header" error handling is triggered. The netdev buffer is
discarded to prevent consumption of the bad Ethernet frame. Also the current
socket buffer is not discarded so the next Ethernet frame can be processed.

Add a new error message to indicate a loss of synchronisation of the 32-bit
Data header word.

4. asix: C1 DUB-E100 double rx_urb_size to 4096

This patch prevents the truncation of the URB when the bulk transfer
contains 2 Ethernet frames by using a 4096 byte RX URB. In other words,
the 2048 URB size was too small.

[PATCH 1/4] asix: Rename remaining and size for clarity

[Dean Jenkins]

The Data header synchronisation is easier to understand
if the variables "remaining" and "size" are renamed.

Therefore, the lifetime of the "remaining" variable exists
outside of asix_rx_fixup_internal() and is used to indicate
any remaining pending bytes of the Ethernet frame that need
to be obtained from the next socket buffer. This allows an
Ethernet frame to span across multiple socket buffers.

"size" is now local to asix_rx_fixup_internal() and contains
the size read from the Data header 32-bit word.

Add "copy_length" to hold the number of the Ethernet frame
bytes (maybe a part of a full frame) that are to be copied
out of the socket buffer.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
 drivers/net/usb/asix.h        |    2 +-
 drivers/net/usb/asix_common.c |   41 +++++++++++++++++++++--------------------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
index 346c032..17a9b83 100644
--- a/drivers/net/usb/asix.h
+++ b/drivers/net/usb/asix.h
@@ -170,7 +170,7 @@ struct asix_data {
 struct asix_rx_fixup_info {
 	struct sk_buff *ax_skb;
 	u32 header;
-	u16 size;
+	u16 remaining;
 	bool split_head;
 };
 
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 577c72d..2a5b2f8 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -55,12 +55,13 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
 			   struct asix_rx_fixup_info *rx)
 {
 	int offset = 0;
+	u16 size;
 
 	while (offset + sizeof(u16) <= skb->len) {
-		u16 remaining = 0;
+		u16 copy_length;
 		unsigned char *data;
 
-		if (!rx->size) {
+		if (!rx->remaining) {
 			if ((skb->len - offset == sizeof(u16)) ||
 			    rx->split_head) {
 				if(!rx->split_head) {
@@ -82,42 +83,42 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
 				offset += sizeof(u32);
 			}
 
-			/* get the packet length */
-			rx->size = (u16) (rx->header & 0x7ff);
-			if (rx->size != ((~rx->header >> 16) & 0x7ff)) {
+			/* take frame length from Data header 32-bit word */
+			size = (u16) (rx->header & 0x7ff);
+			if (size != ((~rx->header >> 16) & 0x7ff)) {
 				netdev_err(dev->net, "asix_rx_fixup() Bad Header Length 0x%x, offset %d\n",
 					   rx->header, offset);
-				rx->size = 0;
 				return 0;
 			}
-			rx->ax_skb = netdev_alloc_skb_ip_align(dev->net,
-							       rx->size);
+			rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
 			if (!rx->ax_skb)
 				return 0;
+			rx->remaining = size;
 		}
 
-		if (rx->size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
+		if (rx->remaining > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
 			netdev_err(dev->net, "asix_rx_fixup() Bad RX Length %d\n",
-				   rx->size);
+				   rx->remaining);
 			kfree_skb(rx->ax_skb);
 			rx->ax_skb = NULL;
-			rx->size = 0U;
-
+			rx->remaining = 0;
 			return 0;
 		}
 
-		if (rx->size > skb->len - offset) {
-			remaining = rx->size - (skb->len - offset);
-			rx->size = skb->len - offset;
+		if (rx->remaining > skb->len - offset) {
+			copy_length = skb->len - offset;
+			rx->remaining -= copy_length;
+		} else {
+			copy_length = rx->remaining;
+			rx->remaining = 0;
 		}
 
-		data = skb_put(rx->ax_skb, rx->size);
-		memcpy(data, skb->data + offset, rx->size);
-		if (!remaining)
+		data = skb_put(rx->ax_skb, copy_length);
+		memcpy(data, skb->data + offset, copy_length);
+		if (!rx->remaining)
 			usbnet_skb_return(dev, rx->ax_skb);
 
-		offset += (rx->size + 1) & 0xfffe;
-		rx->size = remaining;
+		offset += (copy_length + 1) & 0xfffe;
 	}
 
 	if (skb->len != offset) {
-- 
1.7.9.5

[PATCH 2/4] asix: Tidy-up asix_rx_fixup_internal() logic

[Dean Jenkins]

Tidy-up the Data header 32-bit word synchronisation logic in
asix_rx_fixup_internal() by removing redundant logic tests.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
 drivers/net/usb/asix_common.c |   45 +++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 2a5b2f8..1d44c1e 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -62,21 +62,19 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
 		unsigned char *data;
 
 		if (!rx->remaining) {
-			if ((skb->len - offset == sizeof(u16)) ||
-			    rx->split_head) {
-				if(!rx->split_head) {
-					rx->header = get_unaligned_le16(
-							skb->data + offset);
-					rx->split_head = true;
-					offset += sizeof(u16);
-					break;
-				} else {
-					rx->header |= (get_unaligned_le16(
-							skb->data + offset)
-							<< 16);
-					rx->split_head = false;
-					offset += sizeof(u16);
-				}
+			if (skb->len - offset == sizeof(u16)) {
+				rx->header = get_unaligned_le16(
+						skb->data + offset);
+				rx->split_head = true;
+				offset += sizeof(u16);
+				break;
+			}
+
+			if (rx->split_head == true) {
+				rx->header |= (get_unaligned_le16(
+						skb->data + offset) << 16);
+				rx->split_head = false;
+				offset += sizeof(u16);
 			} else {
 				rx->header = get_unaligned_le32(skb->data +
 								offset);
@@ -93,16 +91,15 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
 			rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
 			if (!rx->ax_skb)
 				return 0;
-			rx->remaining = size;
-		}
 
-		if (rx->remaining > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
-			netdev_err(dev->net, "asix_rx_fixup() Bad RX Length %d\n",
-				   rx->remaining);
-			kfree_skb(rx->ax_skb);
-			rx->ax_skb = NULL;
-			rx->remaining = 0;
-			return 0;
+			if (size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
+				netdev_err(dev->net, "asix_rx_fixup() Bad RX Length %d\n",
+					   size);
+				kfree_skb(rx->ax_skb);
+				rx->ax_skb = NULL;
+				return 0;
+			}
+			rx->remaining = size;
 		}
 
 		if (rx->remaining > skb->len - offset) {
-- 
1.7.9.5

[PATCH 3/4] asix: On RX avoid creating bad Ethernet frames

[Dean Jenkins]

Note that the C1 variant of the DUB-E100 USB Ethernet
adapter produces "Bad Header" errors. This modification
will reduce the number of those error messages and instead
provides a more informative reason for the error condition.
The consequences of the error are reduced by not consuming
bad Ethernet frames and unnecessarily discarding the next
Ethernet frame that is probably good.

When RX Ethernet frames span multiple socket buffers,
the data stream can suffer a discontinuity which will cause
the current Ethernet frame in the netdev socket buffer
to be incomplete. This frame needs to be discarded instead
of appending unrelated data from the current socket buffer
to the Ethernet frame in the netdev socket buffer.

A discontinuity can occur when the previous socket buffer
held an incomplete Ethernet frame due to truncation or a
socket buffer containing the end of the Ethernet frame
was missing.

Therefore, add a sanity test for when an Ethernet frame
spans multiple socket buffers to check that the remaining
bytes of the currently received Ethernet frame point to
a good Data header 32-bit word of the next Ethernet
frame. Upon error, reset the remaining byte variable to
zero and discard the current netdev socket buffer.
Assume that the Data header is located at the start of
the current socket buffer and attempt to process the next
Ethernet frame from there. This avoids unnecessarily
discarding a good socket buffer that contains a new
Ethernet frame.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
 drivers/net/usb/asix_common.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 1d44c1e..93423e1 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -57,6 +57,32 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
 	int offset = 0;
 	u16 size;
 
+	/* When an Ethernet frame spans multiple socket buffers,
+	 * do a sanity test for the Data header synchronisation.
+	 * Attempt to detect the situation of the previous socket buffer having
+	 * been truncated or a socket buffer was missing. These situations
+	 * cause a discontinuity in the data stream and therefore need to avoid
+	 * appending bad data to the end of the current netdev socket buffer.
+	 * Also avoid unnecessarily discarding a good current socket buffer.
+	 */
+	if (rx->remaining && (rx->remaining + sizeof(u32) <= skb->len)) {
+		offset = ((rx->remaining + 1) & 0xfffe) + sizeof(u32);
+		rx->header = get_unaligned_le32(skb->data + offset);
+		offset = 0;
+
+		size = (u16) (rx->header & 0x7ff);
+		if (size != ((~rx->header >> 16) & 0x7ff)) {
+			netdev_err(dev->net, "asix_rx_fixup() Data Header synchronisation was lost, remaining %d\n",
+				   rx->remaining);
+			kfree_skb(rx->ax_skb);
+			rx->ax_skb = NULL;
+			/* Assume the Data header is at the start of
+			 * of the socket buffer.
+			 */
+			rx->remaining = 0;
+		}
+	}
+
 	while (offset + sizeof(u16) <= skb->len) {
 		u16 copy_length;
 		unsigned char *data;
-- 
1.7.9.5

[PATCH 4/4] asix: C1 DUB-E100 double rx_urb_size to 4096

[Dean Jenkins]

It seems that for the C1 hardware variant of the D-Link DUB-E100
that the rx_urb_size of 2048 is truncating IP fragmented packets due
to multiple Ethernet frames being present in a single URB.

A simple ping test shows a loss of ping responses
ping 172.17.0.10 -f -c 200 -s 1965
(172.17.0.1 is the IP address of the target)

In the worse case, this test requires a 2049 byte data buffer size
to hold the IN bulk transfer but the URB is 2048 in size so the last byte
of the Ethernet frame is lost and the Ethernet frame is truncated.

This modification resolves "asix_rx_fixup() Bad Header" errors caused by
truncation of the Ethernet frame due to the URB buffer being too small.

Therefore, increase the rx_urb_size to 4096 to accommodate
multiple Ethernet frames being present in a single URB.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
 drivers/net/usb/asix_devices.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 37de7db..cee3f8c 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -492,7 +492,10 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
 	if (dev->driver_info->flags & FLAG_FRAMING_AX) {
 		/* hard_mtu  is still the default - the device does not support
 		   jumbo eth frames */
-		dev->rx_urb_size = 2048;
+		/* Increased to 4K for the C1 DUB-E100 to avoid
+		 * "asix_rx_fixup() Bad Header" errors because the 2K
+		 * urb buffer was too small which caused frame truncation */
+		dev->rx_urb_size = 4096;
 	}
 
 	dev->driver_priv = kzalloc(sizeof(struct asix_common_private), GFP_KERNEL);
-- 
1.7.9.5

RE: [PATCH 4/4] asix: C1 DUB-E100 double rx_urb_size to 4096

[David Laight]

> From: Dean Jenkins
> It seems that for the C1 hardware variant of the D-Link DUB-E100
> that the rx_urb_size of 2048 is truncating IP fragmented packets due
> to multiple Ethernet frames being present in a single URB.
> 
> A simple ping test shows a loss of ping responses
> ping 172.17.0.10 -f -c 200 -s 1965
> (172.17.0.1 is the IP address of the target)
> 
> In the worse case, this test requires a 2049 byte data buffer size
> to hold the IN bulk transfer but the URB is 2048 in size so the last byte
> of the Ethernet frame is lost and the Ethernet frame is truncated.
> 
> This modification resolves "asix_rx_fixup() Bad Header" errors caused by
> truncation of the Ethernet frame due to the URB buffer being too small.
> 
> Therefore, increase the rx_urb_size to 4096 to accommodate
> multiple Ethernet frames being present in a single URB.

I don't think this will help - you've only moved the error.
If the hardware manages to feed over 4k of ethernet frame data
into a single usb bulk data frame it will still go wrong.

The rx code needs to keep the partial ethernet frame until the
next usb bulk data urb arrives.
(This only applies if the urb is a multiple of the usb packet size
and isn't known to have been terminated by a zero length block.)

This should work provided the urb size is a multiple of the usb
packet size - I don't know if a 1536 (3*512) skb fits into a 4k page.
If it doesn't you should probably allocate a 5k skb (8k memory).
(I don't believe there is any point allocating a size inbetween?)

USB3 devices (on USB3 ports) need to allocate buffers that are
multiples of 1k so that they can process very long bulk rx usb data.

	David

Re: [PATCH 4/4] asix: C1 DUB-E100 double rx_urb_size to 4096

[Dean Jenkins]

On 11/12/13 12:30, David Laight wrote:
>> From: Dean Jenkins
>> It seems that for the C1 hardware variant of the D-Link DUB-E100
>> that the rx_urb_size of 2048 is truncating IP fragmented packets due
>> to multiple Ethernet frames being present in a single URB.
>>
>> A simple ping test shows a loss of ping responses
>> ping 172.17.0.10 -f -c 200 -s 1965
>> (172.17.0.1 is the IP address of the target)
>>
>> In the worse case, this test requires a 2049 byte data buffer size
>> to hold the IN bulk transfer but the URB is 2048 in size so the last byte
>> of the Ethernet frame is lost and the Ethernet frame is truncated.
>>
>> This modification resolves "asix_rx_fixup() Bad Header" errors caused by
>> truncation of the Ethernet frame due to the URB buffer being too small.
>>
>> Therefore, increase the rx_urb_size to 4096 to accommodate
>> multiple Ethernet frames being present in a single URB.
> I don't think this will help - you've only moved the error.
> If the hardware manages to feed over 4k of ethernet frame data
> into a single usb bulk data frame it will still go wrong.
I agree in principle, however, I have not seen any evidence of the 4K 
buffer being exceeded. I did a ping test of ping payloads from 0 to 5K 
bytes with no errors. Without the patch, the test fails at ping payloads 
of 1965 (and higher). The test causes IP fragmentation.

The patch certainly helps to avoid failures but perhaps there is a 
better solution ?

I checked with a USB protocol analyser that the C1 DUB-E100 sent a IN 
bulk transfer containing 2049 octets despite the RX URB size being 2048 
bytes long. Therefore, the last octet is lost. The current code fails 
because it waits for the next socket buffer which does not contain the 
missing byte, that octet had already been sent over the wire.

Could this be a symptom of a bug in USB bulk transfer handling ?
> The rx code needs to keep the partial ethernet frame until the
> next usb bulk data urb arrives.
> (This only applies if the urb is a multiple of the usb packet size
> and isn't known to have been terminated by a zero length block.)
With the C1 DUB-E100 I have not seen any Ethernet frames spanning 
multiple URBs but my testing has been limited.
> This should work provided the urb size is a multiple of the usb
> packet size - I don't know if a 1536 (3*512) skb fits into a 4k page.
> If it doesn't you should probably allocate a 5k skb (8k memory).
> (I don't believe there is any point allocating a size inbetween?)
>
> USB3 devices (on USB3 ports) need to allocate buffers that are
> multiples of 1k so that they can process very long bulk rx usb data.
>
> 	David
>
>
>

RE: [PATCH 4/4] asix: C1 DUB-E100 double rx_urb_size to 4096

[David Laight]

> From: Dean Jenkins 
> On 11/12/13 12:30, David Laight wrote:
> >> From: Dean Jenkins
> >> It seems that for the C1 hardware variant of the D-Link DUB-E100
> >> that the rx_urb_size of 2048 is truncating IP fragmented packets due
> >> to multiple Ethernet frames being present in a single URB.
> >>
> >> A simple ping test shows a loss of ping responses
> >> ping 172.17.0.10 -f -c 200 -s 1965
> >> (172.17.0.1 is the IP address of the target)
> >>
> >> In the worse case, this test requires a 2049 byte data buffer size
> >> to hold the IN bulk transfer but the URB is 2048 in size so the last byte
> >> of the Ethernet frame is lost and the Ethernet frame is truncated.
> >>
> >> This modification resolves "asix_rx_fixup() Bad Header" errors caused by
> >> truncation of the Ethernet frame due to the URB buffer being too small.
> >>
> >> Therefore, increase the rx_urb_size to 4096 to accommodate
> >> multiple Ethernet frames being present in a single URB.
> > I don't think this will help - you've only moved the error.
> > If the hardware manages to feed over 4k of ethernet frame data
> > into a single usb bulk data frame it will still go wrong.

> I agree in principle, however, I have not seen any evidence of the 4K
> buffer being exceeded. I did a ping test of ping payloads from 0 to 5K
> bytes with no errors. Without the patch, the test fails at ping payloads
> of 1965 (and higher). The test causes IP fragmentation.

What happens if you force the USB speed below 480MHz?

> The patch certainly helps to avoid failures but perhaps there is a
> better solution ?
> 
> I checked with a USB protocol analyser that the C1 DUB-E100 sent a IN
> bulk transfer containing 2049 octets despite the RX URB size being 2048
> bytes long. Therefore, the last octet is lost. The current code fails
> because it waits for the next socket buffer which does not contain the
> missing byte, that octet had already been sent over the wire.
> 
> Could this be a symptom of a bug in USB bulk transfer handling ?

Looks like one.

My understanding (from trying to fix the xhci driver) is that a bulk
transfer is chopped into 512 byte chunks (for USB2). Transfers can
either be known fixed multiple of 512, or be terminated be a short block.
The hardware doesn't know which, but should advance to the next buffer
of a short block.
So the 2049 byte bulk transfer is four 512byte blocks and a 1 byte block.
If the receive URB are 512 bytes each, this should end up in 5 URB (etc).

Maybe the usb driver is 'cleverly' discarding the continuation blocks
when the rx urb are marked that short transfers are valid.

	David

RE: [PATCH 4/4] asix: C1 DUB-E100 double rx_urb_size to 4096

[Eric Dumazet]

On Wed, 2013-12-11 at 11:30 +0000, David Laight wrote:

> I don't think this will help - you've only moved the error.
> If the hardware manages to feed over 4k of ethernet frame data
> into a single usb bulk data frame it will still go wrong.

Yep, I feel this way too.

> 
> The rx code needs to keep the partial ethernet frame until the
> next usb bulk data urb arrives.
> (This only applies if the urb is a multiple of the usb packet size
> and isn't known to have been terminated by a zero length block.)
> 
> This should work provided the urb size is a multiple of the usb
> packet size - I don't know if a 1536 (3*512) skb fits into a 4k page.
> If it doesn't you should probably allocate a 5k skb (8k memory).
> (I don't believe there is any point allocating a size inbetween?)


skb allocated on RX path have skb->head as a fragment from a page.

We try to use high order pages, so using a size in between is totally
worth it.

Check __netdev_alloc_frag() for details.

For example, using 1536 bytes, we can fit 21 frames per 32 KB page.

If we were rounding do 2048 bytes, we would fit 16 frames per 32KB page.

I fear that doubling rx_urb_size is artificially doubling the skb
truesize (or not, and this would be a bug), and this has
an impact on general performance of TCP stack.

RE: [PATCH 4/4] asix: C1 DUB-E100 double rx_urb_size to 4096

[David Laight]

> From: Eric Dumazet
...
> I fear that doubling rx_urb_size is artificially doubling the skb
> truesize (or not, and this would be a bug), and this has
> an impact on general performance of TCP stack.

Which means that the code in the ax179_178a driver is really horrid.
It causes 20kB URB to be passed to the xhci driver.

I know that hardware supports tcp transmit segmentation offload.
I don't know it if supports receive offload.
Best way to test is probably running at Ge speeds at usb2 speeds
- connect to an xhci port with a USB2 extension cable.

I'm waiting for my existing xhci patches to get processed before
I write any more.

	David