Re: [PATCH 1/1] Userspace I/O (UIO): Add support for userspace DMA

Hans J. Koch
2009-12-15T21:34:17Z

(Deleted hugh <hugh@veritas.com> from Cc:, this address has permanent errors.)

On Tue, Dec 15, 2009 at 09:47:34AM -0800, Earl Chew wrote:
> Earl Chew wrote:
> > I'd like to proceed by changing struct uio_mem [MAX_UIO_MAPS] to a
> > linked list.
>
> Here's my first go at changing to a linked list. I mistakenly
> said that I would proceed by modifying struct uio_mem [MAX_UIO_MAPS].

NAK, for the reasons given in my other mail.

Thanks,
Hans

>
> In fact, I think that mem[MAX_UIO_MAPS] in struct uio_info can remain
> intact -- thus preserving the client facing API.
>
> The linked list change occurs in struct uio_device which is private
> to uio.c ... thus hiding the change from client facing code.
>
>
> Tying the regions into a linked list held by struct uio_device
> simplifies the search-modify code in the rest of uio.c.
>
>
> Earl
>
>
> diff -uNpw uio_driver.h.old uio_driver.h
> --- c:/tmp/makereview.AGgMh 2009-12-15 09:41:23.146608800 -0800
> +++ c:/tmp/makereview.oXYWW 2009-12-15 09:41:23.255983800 -0800
> @@ -20,6 +20,8 @@
>
> /**
> * struct uio_mem - description of a UIO memory region
> + * @list_node: list node for struct uio_device list
> + * @mapid: mapping number for this region
> * @kobj: kobject for this mapping
> * @addr: address of the device's memory
> * @size: size of IO
> @@ -27,6 +29,8 @@
> * @internal_addr: ioremap-ped version of addr, for driver internal use
> */
> struct uio_mem {
> + struct list_head list_node;
> + unsigned map_id;
> struct kobject kobj;
> unsigned long addr;
> unsigned long size;
>
> diff -uNpw uio.c.old uio.c
> --- c:/tmp/makereview.O27Xv 2009-12-15 09:41:26.740358800 -0800
> +++ c:/tmp/makereview.DAhCY 2009-12-15 09:41:26.834108800 -0800
> @@ -35,6 +35,7 @@ struct uio_device {
> int vma_count;
> struct uio_info *info;
> struct kset map_attr_kset;
> + struct list_head mem_list;
> };
>
> static int uio_major;
> @@ -153,7 +154,7 @@ static int uio_dev_add_attributes(struct
> if (ret)
> goto err_group;
>
> - for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> + for (mi = 0; mi < ARRAY_SIZE(idev->info->mem); mi++) {
> mem = &idev->info->mem[mi];
> if (mem->size == 0)
> break;
> @@ -166,6 +167,12 @@ static int uio_dev_add_attributes(struct
> if (ret)
> goto err_remove_group;
> }
> +
> + mem->map_id = mi;
> +
> + INIT_LIST_HEAD(&mem->list_node);
> + list_add(&mem->list_node, &idev->mem_list);
> +
> kobject_init(&mem->kobj);
> kobject_set_name(&mem->kobj,"map%d",mi);
> mem->kobj.parent = &idev->map_attr_kset.kobj;
> @@ -180,6 +187,7 @@ static int uio_dev_add_attributes(struct
> err_remove_maps:
> for (mi--; mi>=0; mi--) {
> mem = &idev->info->mem[mi];
> + list_del(&mem->list_node);
> kobject_unregister(&mem->kobj);
> }
> kset_unregister(&idev->map_attr_kset); /* Needed ? */
> @@ -194,10 +202,11 @@ static void uio_dev_del_attributes(struc
> {
> int mi;
> struct uio_mem *mem;
> - for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> + for (mi = 0; mi < ARRAY_SIZE(idev->info->mem); mi++) {
> mem = &idev->info->mem[mi];
> if (mem->size == 0)
> break;
> + list_del(&mem->list_node);
> kobject_unregister(&mem->kobj);
> }
> kset_unregister(&idev->map_attr_kset);
> @@ -386,18 +395,20 @@ static ssize_t uio_read(struct file *fil
> return retval;
> }
>
> -static int uio_find_mem_index(struct vm_area_struct *vma)
> +static struct uio_mem *uio_find_mem_index(struct vm_area_struct *vma)
> {
> - int mi;
> struct uio_device *idev = vma->vm_private_data;
> + struct list_head *mem_list;
> +
> + list_for_each(mem_list, &idev->mem_list) {
>
> - for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> - if (idev->info->mem[mi].size == 0)
> - return -1;
> - if (vma->vm_pgoff == mi)
> - return mi;
> + struct uio_mem *mem =
> + list_entry(mem_list, struct uio_mem, list_node);
> +
> + if (vma->vm_pgoff == mem->map_id)
> + return mem;
> }
> - return -1;
> + return NULL;
> }
>
> static void uio_vma_open(struct vm_area_struct *vma)
> @@ -415,17 +426,16 @@ static void uio_vma_close(struct vm_area
> static struct page *uio_vma_nopage(struct vm_area_struct *vma,
> unsigned long address, int *type)
> {
> - struct uio_device *idev = vma->vm_private_data;
> struct page* page = NOPAGE_SIGBUS;
>
> - int mi = uio_find_mem_index(vma);
> - if (mi < 0)
> + struct uio_mem *mem = uio_find_mem_index(vma);
> + if (mem == NULL)
> return page;
>
> - if (idev->info->mem[mi].memtype == UIO_MEM_LOGICAL)
> - page = virt_to_page(idev->info->mem[mi].addr);
> + if (mem->memtype == UIO_MEM_LOGICAL)
> + page = virt_to_page(mem->addr);
> else
> - page = vmalloc_to_page((void*)idev->info->mem[mi].addr);
> + page = vmalloc_to_page((void*)mem->addr);
> get_page(page);
> if (type)
> *type = VM_FAULT_MINOR;
> @@ -440,16 +450,15 @@ static struct vm_operations_struct uio_v
>
> static int uio_mmap_physical(struct vm_area_struct *vma)
> {
> - struct uio_device *idev = vma->vm_private_data;
> - int mi = uio_find_mem_index(vma);
> - if (mi < 0)
> + struct uio_mem *mem = uio_find_mem_index(vma);
> + if (mem == NULL)
> return -EINVAL;
>
> vma->vm_flags |= VM_IO | VM_RESERVED;
>
> return remap_pfn_range(vma,
> vma->vm_start,
> - idev->info->mem[mi].addr >> PAGE_SHIFT,
> + mem->addr >> PAGE_SHIFT,
> vma->vm_end - vma->vm_start,
> vma->vm_page_prot);
> }
> @@ -466,7 +475,7 @@ static int uio_mmap(struct file *filep,
> {
> struct uio_listener *listener = filep->private_data;
> struct uio_device *idev = listener->dev;
> - int mi;
> + struct uio_mem *mem;
> unsigned long requested_pages, actual_pages;
> int ret = 0;
>
> @@ -475,12 +484,12 @@ static int uio_mmap(struct file *filep,
>
> vma->vm_private_data = idev;
>
> - mi = uio_find_mem_index(vma);
> - if (mi < 0)
> + mem = uio_find_mem_index(vma);
> + if (mem == NULL)
> return -EINVAL;
>
> requested_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> - actual_pages = (idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT;
> + actual_pages = (mem->size + PAGE_SIZE -1) >> PAGE_SHIFT;
> if (requested_pages > actual_pages)
> return -EINVAL;
>
> @@ -492,7 +501,7 @@ static int uio_mmap(struct file *filep,
> return ret;
> }
>
> - switch (idev->info->mem[mi].memtype) {
> + switch (mem->memtype) {
> case UIO_MEM_PHYS:
> return uio_mmap_physical(vma);
> case UIO_MEM_LOGICAL:
> @@ -613,6 +622,7 @@ int __uio_register_device(struct module
> idev->info = info;
> init_waitqueue_head(&idev->wait);
> atomic_set(&idev->event, 0);
> + INIT_LIST_HEAD(&idev->mem_list);
>
> ret = uio_get_minor(idev);
> if (ret)
>