xhci 1.0: Limit arbitrarily-aligned scatter gather.
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): baa32650c285a89671030370f638e2203171b3a0.1391544195.git.sarah.a.sharp@linux.intel.com
From: Sarah Sharp
Quoted 18 lines
xHCI 1.0 hosts have a set of requirements on how to align transfer buffers on the endpoint rings called "TD fragment" rules. When the
ax88179_178adriver added support for scatter gather in 3.12, with commit804fad45411b48233b48003e33a78f290d227c8"USBNET:ax88179_178a:
enable tso if usb host supports sg dma", it broke the device under xHCI 1.0 hosts. Under certain network loads, the device would see an unexpected short packet from the host, which would cause the device to stop sending ethernet packets, even through USB packets would still be sent.Commit
35773dac5f86"usb: xhci: Link TRB must not occur within a USB payload burst" attempted to fix this. It was a quick hack to partially implement the TD fragment rules. However, it caused regressions in the usb-storage layer and userspace USB drivers using libusb. The patches to attempt to fix this are too far reaching into the USB core, and we really need to implement the TD fragment rules correctly in the xHCI driver, instead of continuing to wallpaper over the issues.
Quoted 5 lines
Disable arbitrarily-aligned scatter-gather in the xHCI driver for 1.0 hosts. Only the
ax88179_178adriver checks theno_sg_constraintflag, so don't set it for 1.0 hosts. This should not impact usb-storage or usbfs behavior, since they pass down max packet sized aligned sg-list entries (512 for USB 2.0 and 1024 for USB 3.0).
I believe that this will cause the ax88179 driver to discard some receive packets on (at least) my panther point 1.00 controller.
I certainly saw bursts of lost packets in some testing I did before the scatter-gather transfers were enabled, and before any of my patches.
The problem is that the ax88179_178a driver submits receive URBs that cross 64k boundaries, and are not aligned (they start at an 0x40 boundary). Receive USB frames can contain multiple ethernet frames, by default they are 20kB (and sit in 24kB of memory).
I'm not entirely convinced that this is acceptable long term behaviour.
David
Quoted 19 lines · David Laight
From: Sarah Sharp
Quoted 18 lines
xHCI 1.0 hosts have a set of requirements on how to align transfer buffers on the endpoint rings called "TD fragment" rules. When the
ax88179_178adriver added support for scatter gather in 3.12, with commit804fad45411b48233b48003e33a78f290d227c8"USBNET:ax88179_178a:
enable tso if usb host supports sg dma", it broke the device under xHCI 1.0 hosts. Under certain network loads, the device would see an unexpected short packet from the host, which would cause the device to stop sending ethernet packets, even through USB packets would still be sent.Commit
35773dac5f86"usb: xhci: Link TRB must not occur within a USB payload burst" attempted to fix this. It was a quick hack to partially implement the TD fragment rules. However, it caused regressions in the usb-storage layer and userspace USB drivers using libusb. The patches to attempt to fix this are too far reaching into the USB core, and we really need to implement the TD fragment rules correctly in the xHCI driver, instead of continuing to wallpaper over the issues.
Quoted 8 lines
Quoted 5 lines
Disable arbitrarily-aligned scatter-gather in the xHCI driver for 1.0 hosts. Only the
ax88179_178adriver checks theno_sg_constraintflag, so don't set it for 1.0 hosts. This should not impact usb-storage or usbfs behavior, since they pass down max packet sized aligned sg-list entries (512 for USB 2.0 and 1024 for USB 3.0).I believe that this will cause the ax88179 driver to discard some receive packets on (at least) my panther point 1.00 controller.
Please go test with that branch, and see if you can trigger this issue. If you can reproduce this, please send me the exact instructions or commands to do so.
Quoted 2 lines
I certainly saw bursts of lost packets in some testing I did before the scatter-gather transfers were enabled, and before any of my patches.
What is the user impact of these lost packets? Does the device drop one particular transfer, and continue with the next transfer, or does the device get wedged and stop sending packets all together?
Ethernet protocols are designed to deal with packet loss. Does the user care about a dropped packet every once and while? Especially for USB ethernet devices, which really shouldn't be used in an enterprise setting?
I'm not saying this shouldn't be fixed. I'm simply trying to see how much of a priority it is to fix this. I really want to re-architect the code and do this right, and it will take some time.
Quoted 4 lines
The problem is that the
ax88179_178adriver submits receive URBs that cross 64k boundaries, and are not aligned (they start at an0x40boundary). Receive USB frames can contain multiple ethernet frames, by default they are 20kB (and sit in 24kB of memory).
Perhaps you should add printks when a TRB is split on 64KB boundaries and see if the device drops packets around that time?
Quoted 1 line
I'm not entirely convinced that this is acceptable long term behaviour.
If the 64KB boundaries are an issue, it will mean that bug has been in the xHCI driver for a very long time. In the kernel community, if a fix for something that has been historically broken causes regressions, we revert it.
Sarah Sharp
Quoted 9 lines · Sarah Sharp
Quoted 4 lines · David Laight
The problem is that the
ax88179_178adriver submits receive URBs that cross 64k boundaries, and are not aligned (they start at an0x40boundary). Receive USB frames can contain multiple ethernet frames, by default they are 20kB (and sit in 24kB of memory).Perhaps you should add printks when a TRB is split on 64KB boundaries and see if the device drops packets around that time?
This seems kind of puzzling.
In theory, any URB that's more than 1 byte long can cross a 64-KB boundary. I don't understand why this should cause problems for xhci-hcd. Sure, the transfer has to be split into multiple TDs where the boundary crossing occurs. But why would that be problematic?
Alan Stern
Quoted 12 lines · Alan Stern
Quoted 9 lines · Sarah Sharp
Quoted 4 lines · David Laight
The problem is that the
ax88179_178adriver submits receive URBs that cross 64k boundaries, and are not aligned (they start at an0x40boundary). Receive USB frames can contain multiple ethernet frames, by default they are 20kB (and sit in 24kB of memory).Perhaps you should add printks when a TRB is split on 64KB boundaries and see if the device drops packets around that time?
Quoted 6 lines
This seems kind of puzzling.
In theory, any URB that's more than 1 byte long can cross a 64-KB boundary. I don't understand why this should cause problems for xhci-hcd. Sure, the transfer has to be split into multiple TDs where the boundary crossing occurs. But why would that be problematic?
If we split a non-scatter gather URB into two TRBs because of the 64-KB boundary rule, and we have to put a link TRB in between them, that may violate the TD fragments rule for 1.0 hosts.
So the debugging should really only trigger when there's a link TRB in between two TRBs on the bulk ring.
Sarah Sharp
Sarah,
Quoted 3 lines · Sarah Sharp
I'm simply trying to see how much of a priority it is to fix this. I really want to re-architect the code and do this right, and it will take some time.
Awesome! I think wanting to do it right is very close to desire for perfection, or at the very least correctness. :)
I do appreciate having to prioritize.
//Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html