On Sat, 5 Dec 2015, Steinar H. Gunderson wrote:
> On Sat, Dec 05, 2015 at 12:36:51PM -0500, Alan Stern wrote:
> > I meant that this "if" statement should test only uurb_start. If the
> > test succeeds, a nested "if" statement should test buffer_length and
> > return an error if it is too big.
> >
> > That's as opposed to the way you have it now, where if buffer_length is
> > too big you return NULL. The system would then try to treat the
> > coherent memory as a normal memory buffer, which may not be a good
> > idea.
>
> OK. Why is it a problem to treat it as a normal memory buffer? (I understand
> it would be slower, of course.)
To tell the truth, I'm not sure whether it would be a problem or not.
That's why I said it "may" not be a good idea.
Mixing the usages would mean mapping the memory region for normal
streaming DMA when it might already have been mapped for coherent DMA.
Maybe that's okay; I don't know. It seems safest to avoid the issue.
Besides, if the user program went to the trouble of mmap'ing a memory
region and then ...:
... tried to present a USB buffer that overflowed the end of
the region, we should point out the error.
... created a control or interrupt transfer with its USB buffer
in the region, we should honor the program's request and
perform a zerocopy I/O operation.
> > You don't really need to do it earlier. An unnecessary calculation of
> > num_sgs won't hurt.
>
> Is it unnecessary? I thought the test was really to force num_sgs==0 for the
> DMA case, not to save a little arithmetic.
Well, if you calculate num_sgs and then force it to 0 anyway, the
calculation was unnecessary, right?
> > Comments below. I'll skip much of the patch because it looks pretty
> > good.
>
> Good, I guess we're finally converting on something acceptable to all parties :-)
Definitely.
BTW, in the future, could you put your patches in the body of the
emails instead of making them attachments? That's how we normally do
things on this mailing list; it makes replying and commenting on
patches easier.
> +static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
> +{
> + struct usb_dev_state *ps = usbm->ps;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ps->lock, flags);
> + --*count;
> + if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
> + list_del_init(&usbm->memlist);
This can be list_del(), since you're about to deallocate usbm.
> + spin_unlock_irqrestore(&ps->lock, flags);
> +
> + usb_free_coherent(ps->dev, usbm->size, usbm->mem,
> + usbm->dma_handle);
> + usbfs_decrease_memory_usage(
> + usbm->size + sizeof(struct usb_memory));
> + kfree(usbm);
> + } else {
> + spin_unlock_irqrestore(&ps->lock, flags);
> + }
> +}
...
> +static struct usb_memory *
> +find_memory_area(struct usb_dev_state *ps, const struct usbdevfs_urb *uurb)
> +{
> + struct usb_memory *usbm = NULL, *iter;
> + unsigned long flags;
> + unsigned long uurb_start = (unsigned long)uurb->buffer;
> +
> + spin_lock_irqsave(&ps->lock, flags);
> + list_for_each_entry(iter, &ps->memory_list, memlist) {
> + if (uurb_start >= iter->vm_start &&
> + uurb_start < iter->vm_start + iter->size) {
> + if (uurb->buffer_length > iter->vm_start + iter->size -
> + uurb_start) {
> + usbm = ERR_PTR(-EINVAL);
> + } else {
> + usbm = iter;
> + usbm->urb_use_count++;
> + break;
> + }
The "break" belongs here, not where it is.
> + }
> + }
> + spin_unlock_irqrestore(&ps->lock, flags);
> + return usbm;
> +}
> +
> static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb,
> struct usbdevfs_iso_packet_desc __user *iso_frame_desc,
> void __user *arg)
> @@ -1372,9 +1525,17 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
> uurb->type = USBDEVFS_URB_TYPE_INTERRUPT;
> goto interrupt_urb;
> }
> - num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
> - if (num_sgs == 1 || num_sgs > ps->dev->bus->sg_tablesize)
> - num_sgs = 0;
> + /* do not use SG buffers when memory mapped segments
> + * are in use
> + */
> + if (as->usbm) {
> + num_sgs = DIV_ROUND_UP(uurb->buffer_length,
> + USB_SG_SIZE);
> + if (num_sgs == 1 ||
> + num_sgs > ps->dev->bus->sg_tablesize) {
> + num_sgs = 0;
> + }
> + }
No, no. Leave this part of the code unchanged. as hasn't even been
allocated yet.
> if (ep->streams)
> stream_id = uurb->stream_id;
> break;
> @@ -1445,10 +1606,15 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
> if (ret)
> goto error;
> as->mem_usage = u;
> + as->usbm = find_memory_area(ps, uurb);
> + if (IS_ERR_VALUE(as->usbm)) {
> + ret = PTR_ERR(as->usbm);
> + goto error;
> + }
As pointed out repeatedly by the kbuild test robot, this should be
IS_ERR, not IS_ERR_VALUE. Also, you need to set as->usbm back to NULL
before jumping.
At this point, set num_sgs to 0 if as->usbm is non-NULL. Actually,
now that I look at the code in context, this whole section needs to go
up a little bit, before before the calculation of u. If num_sgs is
going to be forced to 0, we want to do it before the memory usage is
computed.
> if (num_sgs) {
> as->urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist),
> - GFP_KERNEL);
> + GFP_KERNEL);
Not relevant to the patch.
> if (!as->urb->sg) {
> ret = -ENOMEM;
> goto error;
> @@ -1476,21 +1642,26 @@ 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,
> + if (as->usbm) {
> + as->urb->transfer_buffer = as->usbm->mem;
> + } else {
> + as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
> GFP_KERNEL);
> + }
> if (!as->urb->transfer_buffer) {
> ret = -ENOMEM;
> goto error;
> }
This test may as well be part of the "else" clause. We know
as->usbm->mem is always a valid address.
>
> - if (!is_in) {
> + if (!is_in && as->usbm == NULL) {
> if (copy_from_user(as->urb->transfer_buffer,
> uurb->buffer,
> uurb->buffer_length)) {
> ret = -EFAULT;
> goto error;
> }
> - } else if (uurb->type == USBDEVFS_URB_TYPE_ISO) {
> + } else if (uurb->type == USBDEVFS_URB_TYPE_ISO &&
> + as->usbm == NULL) {
My preferred style for avoiding these repeated tests goes like this:
if (as->usbm) {
; /* Transfer buffer is okay as it is */
} else if (!is_in) { ...
You don't have to copy my style, though.
> /*
> * Isochronous input data may end up being
> * discontiguous if some of the packets are short.
> @@ -1545,10 +1716,18 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
> isopkt = NULL;
> as->ps = ps;
> as->userurb = arg;
> - if (is_in && uurb->buffer_length > 0)
> + if (is_in && uurb->buffer_length > 0 && as->usbm == NULL) {
> as->userbuffer = uurb->buffer;
> - else
> + } else {
> as->userbuffer = NULL;
> + }
The braces aren't needed. In fact, the NULL assignment isn't needed
either, since as was allocated by kzalloc.
> + if (as->usbm != NULL) {
> + unsigned long uurb_start = (unsigned long)uurb->buffer;
> +
> + as->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> + as->urb->transfer_dma = as->usbm->dma_handle +
> + (uurb_start - as->usbm->vm_start);
You also should add (uurb_start - as->usbm->vm_start) to
as->urb->transfer_buffer. Or if you want, you can make that
adjustment where as->urb->transfer_buffer is set originally.
Everything else looks okay.
Alan Stern
--
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
