On Sat, 5 Dec 2015, Steinar H. Gunderson wrote:
> > (Yes, I did.) On further thought, testing uurb_start is sufficient.
> > If uurb->buffer_length then turns out to be too big, the function
> > should return an error (or rather, an ERR_PTR) and the URB submission
> > should fail.
>
> I don't understand “should” here. I read it as “you should change your
> function for this”, but this doesn't rhyme with that uurb_start should be
> sufficient. Do you want me to just remove the check? Is there something else
> that would guard against it?
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.
> >> Done. (I saw you used a plural; is there more than this one?)
> > Well, alloc_urb_memory() also can fail.
>
> I can't find this function. What do you mean?
Doesn't matter; it's not present in the current version of the patch.
> > I would allocate these two in the opposite order. Just because the
> > DMA routines involve more work than kzalloc/kfree.
>
> Done. My thought, by the way, was that the DMA routines were much more likely
> to fail.
No doubt that's true. The order doesn't really matter very much. But
there is a similar issue in the same spot; see below.
> >> + if (find_memory_area(ps, uurb) == NULL) {
> > Don't call find_memory_area() twice! Save this result and use it
> > later. Also, move this call up before the "switch" statement so that
> > it will apply to all transfer types, not just bulk.
> >
> > Suitable adjustments will be needed in the rest of this routine, but
> > nothing that requires additional review comments.
>
> This part I don't understand. You want to populate usbm (by calling
> find_memory_area()) unconditionally, also for control transfers?
I agree there's not much point in using this API for control or
interrupt transfers. Still, we have to allow for the possibility that
someone might do it, in order to avoid treating coherent memory like
normal memory as mentioned above.
> I can't see
> offhand another way to call it only once during the function, save for a bool
> that says if we called it or not.
Simply call it at the spot where you initialize as->usbm to NULL. Then
it will always run exactly once.
You don't really need to do it earlier. An unnecessary calculation of
num_sgs won't hurt.
> New patch attached. Note that I haven't had the chance to retest after some
> of the latest changes.
Comments below. I'll skip much of the patch because it looks pretty
good.
> +static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + struct usb_memory *usbm = NULL;
> + struct usb_dev_state *ps = file->private_data;
> + size_t size = vma->vm_end - vma->vm_start;
> + void *mem;
> + unsigned long flags;
> + dma_addr_t dma_handle;
> + int ret;
> +
> + usbm = kzalloc(sizeof(struct usb_memory), GFP_KERNEL);
> + if (!usbm)
> + return -ENOMEM;
> +
> + ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> + if (ret) {
> + kfree(usbm);
> + return ret;
> + }
This part should come first. If allocating usbm would put us over the
memory limit, we want to know before we do the allocation.
> +
> + mem = usb_alloc_coherent(ps->dev, size, GFP_KERNEL, &dma_handle);
> + if (!mem) {
> + usbfs_decrease_memory_usage(
> + usbm->size + sizeof(struct usb_memory));
> + kfree(usbm);
> + return -ENOMEM;
> + }
> +
> + memset(mem, 0, size);
> +
> + usbm->mem = mem;
> + usbm->dma_handle = dma_handle;
> + usbm->size = size;
> + usbm->ps = ps;
> +
> + if (remap_pfn_range(vma, vma->vm_start,
> + virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> + size, vma->vm_page_prot) < 0) {
> + dec_usb_memory_use_count(usbm, NULL);
> + return -EAGAIN;
> + }
> +
> + usbm->vm_start = vma->vm_start;
> + usbm->vma_use_count = 1;
I would move these two lines before the remap_pfn_range() call. Then
you can use &usbm->vma_use_count as the second argument to
dec_usb_memory_use_count() and avoid the awkward "if (count)" test at
the start of that routine.
> +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 &&
> + uurb->buffer_length <= iter->vm_start + iter->size -
> + uurb_start) {
> + usbm = iter;
> + usbm->urb_use_count++;
> + break;
> + }
So this would be:
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;
}
(That's with the file's convention for continuation lines.)
> + }
> + 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)
Much of this function is still subject to change, so I won't look at it
now.
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
