| Message ID | 1386759028-3534-5-git-send-email-Dean_Jenkins@mentor.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --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);
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(-)