Hans de Goede wrote:
> This patch makes it possible for userspace to reliable submit large bulk
> transfers to scatter-gather capable host controllers in one go, by using a
> scatterlist to break the transfer up in managable chunks.
The last part of this sentence sounds like userspace is supposed to
use a scatterlist, and simple typos. Suggest:
This patch makes it possible for userspace to reliably submit large bulk
(and interrupt?) transfers to scatter-gather capable host controllers as
a single transfer. usbdevfs will then use a scatterlist to break the
transfer up in manageable chunks.
> @@ -55,6 +56,7 @@
>
> #define USB_MAXBUS 64
> #define USB_DEVICE_MAX USB_MAXBUS * 128
> +#define USB_SG_SIZE 16384 /* split-size for large txs */
Would it make sense to base this on PAGE_SIZE?
> +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;
> + }
> +}
I don't know.. Are you sure about dumping all the data? That's what
usbmon is for. I understand that this was helpful for development,
but perhaps it isn't needed in the patch?
And in general, is there really no sg support code that can do the
iterations? It seems like there will be a lot of this boilerplate
code in all sg users otherwise, which would kinda suck.
If no, perhaps at least combine the two cases num_sgs==0 and >0 into
a single code path. It doesn't take much in either place. Pretend
that num_sgs==1 when it's ==0, and special case size calculation,
then the loops work for both cases.
> +static int copy_urb_data_to_user(u8 __user *userbuffer, struct urb *urb)
> +{
..
> + if (urb->num_sgs == 0) {
> + if (copy_to_user(userbuffer, urb->transfer_buffer, len))
..
> + for (i = 0; i < urb->num_sgs && len; i++) {
> + size = (len > USB_SG_SIZE) ? USB_SG_SIZE : len;
> + if (copy_to_user(userbuffer, sg_virt(&urb->sg[i]), size))
Is there no easy way to do zero copy already from the start?
CPU copying data back and forth is not very 2012. Scatter-gather is
more 2012. Zero copy matches that.
> +++ b/include/linux/usbdevice_fs.h
> @@ -129,6 +129,7 @@ struct usbdevfs_hub_portinfo {
> #define USBDEVFS_CAP_ZERO_PACKET 0x01
> #define USBDEVFS_CAP_BULK_CONTINUATION 0x02
> #define USBDEVFS_CAP_NO_PACKET_SIZE_LIM 0x04
> +#define USBDEVFS_CAP_BULK_SCATTER_GATHER 0x08
Is it wise to put bulk in the cap name? Isoc seems like a good other
candidate.
//Peter
--
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
