On Sun, 6 Dec 2015, Steinar H. Gunderson wrote:
> On Sun, Dec 06, 2015 at 01:06:08AM +0100, Steinar H. Gunderson wrote:
> > I'll try to update your patch with the other suggestions tomorrow. Thanks!
>
> Here we are. Lightly tested, I believe all comments should be addressed.
This looks quite good. I have only a couple of comments.
> /* Steinar */
>
> From 73833276a4f359c35edffc2a741dba57f2e82a12 Mon Sep 17 00:00:00 2001
> From: "Steinar H. Gunderson" <se...@google.com>
> Date: Thu, 26 Nov 2015 01:19:13 +0100
> Subject: [PATCH] Add support for usbfs zerocopy.
>
> Add a new interface for userspace to preallocate memory that can be
> used with usbfs. This gives two primary benefits:
>
> - Zerocopy; data no longer needs to be copied between the userspace
> and the kernel, but can instead be read directly by the driver from
> userspace's buffers. This works for both bulk and isochronous transfers;
> isochronous also no longer need to memset() the buffer to zero to avoid
> leaking kernel data.
Actually it now works for control and interrupt too, although there's
not much point using it for them.
> - Once the buffers are allocated, USB transfers can no longer fail due to
> memory fragmentation; previously, long-running programs could run into
> problems finding a large enough contiguous memory chunk, especially on
> embedded systems or at high rates.
> @@ -1439,6 +1592,19 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
> struct usbdevfs_urb *uurb
> goto error;
> }
>
> + as->usbm = find_memory_area(ps, uurb);
> + if (IS_ERR(as->usbm)) {
> + ret = PTR_ERR(as->usbm);
> + goto error;
Hmmm, and then what will happen when the error-handling code does this:
+ if (as && as->usbm)
+ dec_usb_memory_use_count(as->usbm, &as->usbm->urb_use_count);
?
> + }
> +
> + /* do not use SG buffers when memory mapped segments
> + * are in use
> + */
> + if (!as->usbm) {
You mean "if (as->usbm)".
> + num_sgs = 0;
> + }
Braces aren't needed.
> +
> u += sizeof(struct async) + sizeof(struct urb) + uurb->buffer_length +
> num_sgs * sizeof(struct scatterlist);
> ret = usbfs_increase_memory_usage(u);
> @@ -1476,14 +1642,23 @@ static int proc_do_submiturb(struct usb_dev_state
> *ps, struct usbdevfs_urb *uurb
> totlen -= u;
> }
> } else if (uurb->buffer_length > 0) {
> - as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
> - GFP_KERNEL);
> - if (!as->urb->transfer_buffer) {
> - ret = -ENOMEM;
> - goto error;
> + if (as->usbm) {
> + unsigned long uurb_start = (unsigned long)uurb->buffer;
> +
> + as->urb->transfer_buffer = as->usbm->mem +
> + (uurb_start - as->usbm->vm_start);
> + } else {
> + as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
> + GFP_KERNEL);
> + if (!as->urb->transfer_buffer) {
> + ret = -ENOMEM;
> + goto error;
> + }
> }
>
> - if (!is_in) {
> + if (as->usbm) {
> + ; /* Transfer buffer is okay as it is */
> + } else if (!is_in) {
> if (copy_from_user(as->urb->transfer_buffer,
> uurb->buffer,
> uurb->buffer_length)) {
It looks odd repeating the "if (as->usbm)" test like this. You can merge
the stuff here into the "else" clause of the preceding code.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
