On Mon, 7 Dec 2015, Steinar H. Gunderson wrote:
> On Mon, Dec 07, 2015 at 10:45:55AM -0500, Alan Stern wrote:
> >> Here we are. Lightly tested, I believe all comments should be addressed.
> > This looks quite good. I have only a couple of comments.
>
> Excellent. I'm sorry we've needed so many rounds; this is what happens when
> you pick up someone else's patch against a code base you've never touched
> before. :-)
Of course. And this was not a particularly simple patch.
> +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(&usbm->memlist);
> + 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 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;
> +
> + ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> + if (ret) {
> + return ret;
> + }
> +
> + usbm = kzalloc(sizeof(struct usb_memory), GFP_KERNEL);
> + if (!usbm) {
> + usbfs_decrease_memory_usage(
> + size + sizeof(struct usb_memory));
> + return -ENOMEM;
> + }
> +
> + mem = usb_alloc_coherent(ps->dev, size, GFP_KERNEL, &dma_handle);
> + if (!mem) {
> + usbfs_decrease_memory_usage(
> + 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;
> + usbm->vm_start = vma->vm_start;
> + usbm->vma_use_count = 1;
> +
> + 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, &usbm->vma_use_count);
> + return -EAGAIN;
> + }
I just noticed this -- usbm->memlist needs to be initialized before
dec_usb_memory_use_count() can be called. INIT_LIST_HEAD before the
"if" statement will fix the problem.
The patch could use one more ingredient. In
include/uapi/linux/usbdevicefs_h, add a #define for USBDEVFS_CAP_MMAP
and fix up proc_get_capabilities() to report the new capability. In
theory, user programs could just try an mmap() call and see if it
works, but this seems cleaner.
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
