Hi,
On 06/28/2012 09:38 PM, Alan Stern wrote:
On Thu, 28 Jun 2012, Hans de Goede wrote:
When using urb->transfer_buffer we need to allocate physical contiguous buffers
for the entire transfer, which is pretty much guaranteed to fail with large
transfers.
Currently userspace works around this by breaking large transfers into
multiple urbs. This leads to all kind of complications. This patch makes
it possible for userspace to reliable submit large transfers to scatter-gather
capable host controllers in one go, by using a scatterlist to break the
transfer up in managable chunks.
This looks quite good.
I'm glad you like it.
You tested it with various sizes of URBs?
Yes, I used the test-case which triggered the original problem on the XHCI
controller, redirecting a sandisk cruzer u3 usb stick to a windows 7 vm
using Spice's USB redirection, where the spice-client uses libusb to access
the USB stick. This involves bulk and ctrl transfers of various sizes including
ones > 16k.
This reminds me once I finish the libusb support for this (the initial testing
was done with an ugly hack), I also need to test large isoc transfers.
One possible enhancement would be to try allocating storage in units
larger than 16 KB, with fallback to smaller sizes when the larger
allocations fail.
This puts us above order 2 (x86_64) / order 3 (i386) allocations, and high
order allocations are quite expensive (if we ask the kernel will do a lot of
work to try and satisfy this before it fails) this is one of the reasons why
the kernel stack was reduced to be 1 page.
So I think the costs may be higher then the gains while making the code more
complicated.
However your code is just as good as the existing
mechanism.
+static void snoop_urb_data(struct urb *urb, unsigned len)
+{
+ int i, size;
+
+ if (!usbfs_snoop)
+ return;
+
+ if (urb->num_sgs == 0) {
+ print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE, 32, 1,
+ urb->transfer_buffer, len, 1);
+ return;
+ }
+
+ for (i = 0; i < urb->num_sgs && len; i++) {
+ size = (len > USB_SG_SIZE) ? USB_SG_SIZE : len;
+ print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE, 32, 1,
+ sg_virt(&urb->sg[i]), size, 1);
+ len -= size;
+ }
+}
Why didn't you just merge this directly into snoop_urb?
Because snoop_urb is used in the synchronous ctrl / bulk ioctls too, and there
there is no struct urb. I could have added a struct urb parameter to snoop_urb
but that seemed more complicated / convoluted.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
