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

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

[edward_estabrook]

From: Edward Estabrook <Edward_Estabrook@agilent.com>

Here is a patch that adds the ability to dynamically allocate (and use) coherent DMA from userspace by extending the userspace IO driver.  This patch applies against 2.6.28-rc6.

The gist of this implementation is to overload uio's mmap functionality to allocate and map a new DMA region on demand.  The bus-specific DMA address as returned by dma_alloc_coherent is made available to userspace in the 1st long word of the newly created region (as well as through the conventional 'addr' file in sysfs).  

To allocate a DMA region you use the following:
/* Pass this magic number to mmap as offset to dynamically allocate a chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL

void* memory = mmap (NULL, size, PROT_READ | PROT_WRITE , MAP_SHARED, fd, DMA_MEM_ALLOCATE_MMAP_OFFSET); u_int64_t *addr = *(u_int64_t *) memory;

where 'size' is the size in bytes of the region you want and fd is the opened /dev/uioN file.

Allocation occurs in page sized pieces by design to ensure that buffers are page-aligned.

Memory is released when uio_unregister_device() is called.

I have used this extensively on a 2.6.21-based kernel and ported it to 2.6.28-rc6 for review / submission here.

Comments appreciated!

Signed-off-by: Edward Estabrook <Edward_Estabrook@agilent.com>
---
--- linux-2.6.28-rc6/include/linux/uio_driver.h.orig	2008-12-02 09:39:34.000000000 -0800
+++ linux-2.6.28-rc6/include/linux/uio_driver.h	2008-12-02 10:55:01.000000000 -0800
@@ -5,6 +5,7 @@
  * Copyright(C) 2005, Thomas Gleixner <tglx@linutronix.de>
  * Copyright(C) 2006, Hans J. Koch <hjk@linutronix.de>
  * Copyright(C) 2006, Greg Kroah-Hartman <greg@kroah.com>
+ * Copyright(C) 2008, Edward Estabrook <edward_estabrook@agilent.com>
  *
  * Userspace IO driver.
  *
@@ -26,7 +27,11 @@ struct uio_map;
  * @size:		size of IO
  * @memtype:		type of memory addr points to
  * @internal_addr:	ioremap-ped version of addr, for driver internal use
- * @map:		for use by the UIO core only.
+ * 			(for DMA memory regions the dma_handle is stored here)
+ * @map:		for use by the UIO core only
+ * @list:		DMA-capable buffers ; undefined if not a DMA buffer
+ * @index:		index number associated with this DMA-capable mapping
+ * 			(undefined if not a DMA buffer)
  */
 struct uio_mem {
 	unsigned long		addr;
@@ -34,9 +39,12 @@ struct uio_mem {
 	int			memtype;
 	void __iomem		*internal_addr;
 	struct uio_map		*map;
+	struct list_head	list;
+	unsigned		index;
 };
 
-#define MAX_UIO_MAPS	5
+/* Increased to accomodate many logical memory regions per BAR. */
+#define MAX_UIO_MAPS	100
 
 struct uio_device;
 
@@ -46,6 +54,8 @@ struct uio_device;
  * @name:		device name
  * @version:		device driver version
  * @mem:		list of mappable memory regions, size==0 for end of list
+ * @dma_mem_head	list of dynamically allocated DMA-capable memory regions
+ * @dma_mem_size	number of entries in dma_mem_head (used to speed up insertion)
  * @irq:		interrupt number or UIO_IRQ_CUSTOM
  * @irq_flags:		flags for request_irq()
  * @priv:		optional private data
@@ -60,6 +70,8 @@ struct uio_info {
 	char			*name;
 	char			*version;
 	struct uio_mem		mem[MAX_UIO_MAPS];
+	struct list_head	dma_mem;
+	unsigned		dma_mem_size;
 	long			irq;
 	unsigned long		irq_flags;
 	void			*priv;
@@ -82,6 +94,16 @@ static inline int __must_check  extern void uio_unregister_device(struct uio_info *info);  extern void uio_event_notify(struct uio_info *info);
 
+/* uio_dev_get_name - return the name associated with this uio device 
+*/ extern char *uio_dev_get_name(struct uio_device *idev);
+
+/* Starting index assigned to dynamically allocated regions. */ #define 
+UIO_DMA_MEM_BASE_INDEX 1000
+
+/* mmap dynamically allocates a DMA memory block if
+ * its offset parameter matches this value. */ #define 
+UIO_DMA_MEM_ALLOCATE_VM_PGOFF_MAGIC 0xFFFFFUL
+
 /* defines for uio_info->irq */
 #define UIO_IRQ_CUSTOM	-1
 #define UIO_IRQ_NONE	-2
--- linux-2.6.28-rc6/drivers/uio/uio.c.orig	2008-12-03 02:01:16.000000000 -0800
+++ linux-2.6.28-rc6/drivers/uio/uio.c	2008-12-03 02:14:08.000000000 -0800
@@ -5,6 +5,7 @@
  * Copyright(C) 2005, Thomas Gleixner <tglx@linutronix.de>
  * Copyright(C) 2006, Hans J. Koch <hjk@linutronix.de>
  * Copyright(C) 2006, Greg Kroah-Hartman <greg@kroah.com>
+ * Copyright(C) 2008, Edward Estabrook <edward_estabrook@agilent.com>
  *
  * Userspace IO
  *
@@ -21,6 +22,7 @@
 #include <linux/idr.h>
 #include <linux/string.h>
 #include <linux/kobject.h>
+#include <linux/dma-mapping.h>
 #include <linux/uio_driver.h>
 
 #define UIO_MAX_DEVICES 255
@@ -62,7 +64,10 @@ struct uio_map {
 
 static ssize_t map_addr_show(struct uio_mem *mem, char *buf)  {
-	return sprintf(buf, "0x%lx\n", mem->addr);
+	if (mem->index < UIO_DMA_MEM_BASE_INDEX)
+		return sprintf(buf, "0x%lx\n", mem->addr);
+	else
+		return sprintf(buf, "0x%p\n", mem->internal_addr);
 }
 
 static ssize_t map_size_show(struct uio_mem *mem, char *buf) @@ -171,14 +176,38 @@ static struct attribute_group uio_attr_g
 	.attrs = uio_attrs,
 };
 
+static int uio_mem_add_attribute(struct uio_device *idev, struct 
+uio_mem *umem) {
+	struct uio_map *map = kzalloc(sizeof(*map), GFP_KERNEL);
+	int ret = 0;
+	if (!map)
+		return -ENOMEM;
+
+	kobject_init(&map->kobj, &map_attr_type);
+	map->mem = umem;
+	umem->map = map;
+	ret = kobject_add(&map->kobj, idev->map_dir, "map%d", umem->index);
+	if (ret)
+		return ret;
+	return kobject_uevent(&map->kobj, KOBJ_ADD); }
+
+
 /*
  * device functions
  */
+
+/* uio_dev_get_name - return the name associated with this uio device 
+*/ char *uio_dev_get_name(struct uio_device *idev) {
+	return idev->dev->bus_id;
+}
+EXPORT_SYMBOL_GPL(uio_dev_get_name);
+
 static int uio_dev_add_attributes(struct uio_device *idev)  {
 	int ret;
 	int mi;
-	int map_found = 0;
 	struct uio_mem *mem;
 	struct uio_map *map;
 
@@ -186,27 +215,21 @@ static int uio_dev_add_attributes(struct
 	if (ret)
 		goto err_group;
 
+	/*
+	 * Always register 'maps' even if there are no static
+	 * memory regions so it's ready for dynamic DMA maps
+	 */
+	idev->map_dir = kobject_create_and_add("maps", &idev->dev->kobj);
+	if (!idev->map_dir)
+		goto err_remove_group;
+
 	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
 		mem = &idev->info->mem[mi];
 		if (mem->size == 0)
 			break;
-		if (!map_found) {
-			map_found = 1;
-			idev->map_dir = kobject_create_and_add("maps",
-							&idev->dev->kobj);
-			if (!idev->map_dir)
-				goto err;
-		}
-		map = kzalloc(sizeof(*map), GFP_KERNEL);
-		if (!map)
-			goto err;
-		kobject_init(&map->kobj, &map_attr_type);
-		map->mem = mem;
-		mem->map = map;
-		ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
-		if (ret)
-			goto err;
-		ret = kobject_uevent(&map->kobj, KOBJ_ADD);
+		mem->index = mi;
+
+		ret = uio_mem_add_attribute(idev, mem);
 		if (ret)
 			goto err;
 	}
@@ -214,12 +237,13 @@ static int uio_dev_add_attributes(struct
 	return 0;
 
 err:
-	for (mi--; mi>=0; mi--) {
+	for (mi--; mi >= 0; mi--) {
 		mem = &idev->info->mem[mi];
 		map = mem->map;
 		kobject_put(&map->kobj);
 	}
 	kobject_put(idev->map_dir);
+err_remove_group:
 	sysfs_remove_group(&idev->dev->kobj, &uio_attr_grp);
 err_group:
 	dev_err(idev->dev, "error creating sysfs files (%d)\n", ret); @@ -230,12 +254,16 @@ static void uio_dev_del_attributes(struc  {
 	int mi;
 	struct uio_mem *mem;
+
+	/* Unregister static maps */
 	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
 		mem = &idev->info->mem[mi];
 		if (mem->size == 0)
 			break;
 		kobject_put(&mem->map->kobj);
 	}
+
+	/* Unregister containers */
 	kobject_put(idev->map_dir);
 	sysfs_remove_group(&idev->dev->kobj, &uio_attr_grp);  } @@ -459,18 +487,39 @@ static ssize_t uio_write(struct file *fi
 	return retval ? retval : sizeof(s32);
 }
 
-static int uio_find_mem_index(struct vm_area_struct *vma)
+
+/**
+ * uio_find_mem_region - return the memory region identified by 
+vm->vm_pgoff
+ * @vma: vma descriptor as passed to mmap.
+ *
+ * returns memory region from either mem array or dma_mem list.
+ */
+static struct uio_mem *uio_find_mem_region(struct vm_area_struct *vma)
 {
-	int mi;
 	struct uio_device *idev = vma->vm_private_data;
+	struct uio_mem *umem;
 
-	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
-		if (idev->info->mem[mi].size == 0)
-			return -1;
-		if (vma->vm_pgoff == mi)
-			return mi;
+
+	if (vma->vm_pgoff < UIO_DMA_MEM_BASE_INDEX) {
+		if (vma->vm_pgoff < MAX_UIO_MAPS
+				&& idev->info->mem[vma->vm_pgoff].size != 0)
+			return &(idev->info->mem[vma->vm_pgoff]);
+	} else {
+		if (vma->vm_pgoff >= idev->info->dma_mem_size
+				+ UIO_DMA_MEM_BASE_INDEX)
+			return 0; /* Too big: fail automatically */
+
+		/* Iterate thru looking for entry in which vm_pgoff matches
+		 * index. Yes, this is slow, but it's only for test & debug
+		 * access anyway.
+		 */
+		list_for_each_entry(umem, &idev->info->dma_mem, list) {
+			if (umem->index == vma->vm_pgoff)
+				return umem;
+		}
 	}
-	return -1;
+
+	return 0;
 }
 
 static void uio_vma_open(struct vm_area_struct *vma) @@ -487,25 +536,24 @@ static void uio_vma_close(struct vm_area
 
 static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)  {
-	struct uio_device *idev = vma->vm_private_data;
 	struct page *page;
 	unsigned long offset;
 
-	int mi = uio_find_mem_index(vma);
-	if (mi < 0)
+	struct uio_mem *umem = uio_find_mem_region(vma);
+	if (!umem)
 		return VM_FAULT_SIGBUS;
 
 	/*
 	 * We need to subtract mi because userspace uses offset = N*PAGE_SIZE
 	 * to use mem[N].
 	 */
-	offset = (vmf->pgoff - mi) << PAGE_SHIFT;
+	offset = (vmf->pgoff - umem->index) << PAGE_SHIFT;
 
-	if (idev->info->mem[mi].memtype == UIO_MEM_LOGICAL)
-		page = virt_to_page(idev->info->mem[mi].addr + offset);
+	if (umem->memtype == UIO_MEM_LOGICAL)
+		page = virt_to_page(umem->addr + offset);
 	else
-		page = vmalloc_to_page((void *)idev->info->mem[mi].addr
-							+ offset);
+		page = vmalloc_to_page((void *)umem->addr + offset);
+
 	get_page(page);
 	vmf->page = page;
 	return 0;
@@ -519,9 +567,8 @@ 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 *umem = uio_find_mem_region(vma);
+	if (!umem)
 		return -EINVAL;
 
 	vma->vm_flags |= VM_IO | VM_RESERVED;
@@ -530,7 +577,7 @@ static int uio_mmap_physical(struct vm_a
 
 	return remap_pfn_range(vma,
 			       vma->vm_start,
-			       idev->info->mem[mi].addr >> PAGE_SHIFT,
+			       umem->addr >> PAGE_SHIFT,
 			       vma->vm_end - vma->vm_start,
 			       vma->vm_page_prot);
 }
@@ -543,12 +590,99 @@ static int uio_mmap_logical(struct vm_ar
 	return 0;
 }
 
+/**
+ * uio_mem_create_dma_region - allocate a DMA region of size bytes
+ * @umem:     the newly allocated uio_mem structure is returned here
+ * @size:     the number of bytes to allocate
+ * @idev:     the UIO device this region belongs to
+ *
+ * return 0 if success or negative error code on failure  */ static int 
+uio_mem_create_dma_region(struct uio_mem **umem,
+				     unsigned long size,
+				     struct uio_device *idev)
+{
+	int ret = 0;
+	unsigned long *addr;
+
+	*umem = kzalloc(sizeof(struct uio_mem), GFP_KERNEL);
+	if (!*umem) {
+		dev_err(idev->dev,
+			"Unable to allocate uio_mem structure.\n");
+		return -ENOMEM;
+	}
+
+	/* Allocate DMA-capable buffer */
+	addr = dma_alloc_coherent(idev->dev->parent, size,
+			(dma_addr_t *) &(*umem)->internal_addr,
+			GFP_KERNEL);
+	if (!addr) {
+		dev_warn(idev->dev, "Unable to allocate requested DMA-capable"
+			" block of size 0x%lx (%lu) during mmap in uio.\n",
+			size, size);
+		ret = -ENOMEM;
+		goto err_free_umem;
+	}
+
+	/* Store the physical address and index as the
+	 * first two long words for userspace access */
+	(*umem)->addr = (unsigned long) addr;
+	addr[0] = (unsigned long) (*umem)->internal_addr;
+	(*umem)->memtype = UIO_MEM_LOGICAL;
+	(*umem)->size = size;
+	(*umem)->index = idev->info->dma_mem_size + UIO_DMA_MEM_BASE_INDEX;
+	addr[1] = (unsigned long) (*umem)->index;
+
+	/* Register the mapping in sysfs */
+	ret = uio_mem_add_attribute(idev, *umem);
+	if (ret) {
+		dev_err(idev->dev, "Unable to register sysfs entry for "
+				"DMA mapping (%d) in uio.\n", ret);
+		goto err_free_addr;
+	}
+
+	idev->info->dma_mem_size++;
+	list_add_tail(&((*umem)->list), &(idev->info->dma_mem));
+
+	return 0;
+
+err_free_addr:
+	dma_free_coherent(idev->dev->parent, size, addr,
+			(dma_addr_t) (*umem)->internal_addr);
+err_free_umem:
+	kfree(*umem);
+	*umem = 0;
+	return ret;
+}
+
+/**
+ * uio_mem_destroy_dma_region - destroy dynamically created DMA region
+ * @umem:      UIO memory region to destroy
+ * @idev:      the UIO device this region belongs to
+ *
+ */
+static void uio_mem_destroy_dma_region(struct uio_mem *umem,
+				       struct uio_device *idev)
+{
+	/* Remove sysfs attributes */
+	struct uio_map *map = umem->map;
+	kobject_put(&map->kobj);
+
+	/* Free the buffer itself */
+	dma_free_coherent(idev->dev->parent, umem->size, (void *) umem->addr,
+			(dma_addr_t) umem->internal_addr);
+	list_del(&(umem->list));
+
+	kfree(umem);
+}
+
 static int uio_mmap(struct file *filep, struct vm_area_struct *vma)  {
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
-	int mi;
+	struct uio_mem *umem = 0;
 	unsigned long requested_pages, actual_pages;
+	unsigned long requested_size;
 	int ret = 0;
 
 	if (vma->vm_end < vma->vm_start)
@@ -556,12 +690,24 @@ static int uio_mmap(struct file *filep, 
 
 	vma->vm_private_data = idev;
 
-	mi = uio_find_mem_index(vma);
-	if (mi < 0)
-		return -EINVAL;
+	requested_size = vma->vm_end - vma->vm_start;
+	requested_pages = requested_size >> PAGE_SHIFT;
+
+	if (vma->vm_pgoff == UIO_DMA_MEM_ALLOCATE_VM_PGOFF_MAGIC) {
+		/* Create a memory region and change
+		 * vm_pgoff to the newly created index */
+		ret = uio_mem_create_dma_region(&umem, requested_size, idev);
+		if (ret)
+			return ret;
+
+		vma->vm_pgoff = umem->index;
+	} else {
+		umem = uio_find_mem_region(vma);
+		if (!umem)
+			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 = (umem->size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	if (requested_pages > actual_pages)
 		return -EINVAL;
 
@@ -570,14 +716,14 @@ static int uio_mmap(struct file *filep, 
 		return ret;
 	}
 
-	switch (idev->info->mem[mi].memtype) {
-		case UIO_MEM_PHYS:
-			return uio_mmap_physical(vma);
-		case UIO_MEM_LOGICAL:
-		case UIO_MEM_VIRTUAL:
-			return uio_mmap_logical(vma);
-		default:
-			return -EINVAL;
+	switch (umem->memtype) {
+	case UIO_MEM_PHYS:
+		return uio_mmap_physical(vma);
+	case UIO_MEM_LOGICAL:
+	case UIO_MEM_VIRTUAL:
+		return uio_mmap_logical(vma);
+	default:
+		return -EINVAL;
 	}
 }
 
@@ -658,6 +804,50 @@ static void uio_class_destroy(void)
 		kref_put(&uio_class->kref, release_uio_class);  }
 
+void uio_remove_all_dma_regions(struct uio_info *info) {
+	struct uio_mem *mem, *pos;
+
+	list_for_each_entry_safe(pos, mem, &info->dma_mem, list) {
+		uio_mem_destroy_dma_region(pos, info->uio_dev);
+	}
+}
+
+/**
+ * uio_validate_mem - ensure memory regions are page-aligned
+ * 		      and unused regions are zeroed
+ * @idev: The UIO device containing memory regions to check
+ *
+ * returns 0 on success or -EINVAL if misaligned.
+ */
+static int uio_validate_mem(struct uio_device *idev) {
+	int ret;
+	int mi;
+	struct uio_mem *mem;
+
+	for (mi = 0; mi < MAX_UIO_MAPS && idev->info->mem[mi].size != 0; mi++) {
+		mem = &idev->info->mem[mi];
+
+		if ((mem->addr & PAGE_MASK) != mem->addr) {
+			ret = -EINVAL;
+			goto err_misaligned;
+		}
+	}
+
+	/* zero out mem->size for unused regions for faster/safer lookup */
+	for (; mi < MAX_UIO_MAPS; mi++)
+		idev->info->mem[mi].size = 0;
+
+	return 0;
+err_misaligned:
+	dev_err(
+		idev->dev,
+		"mapable memory regions must be page-aligned (%d)\n",
+		ret);
+	return ret;
+}
+
 /**
  * uio_register_device - register a new userspace IO device
  * @owner:	module that creates the new device
@@ -677,6 +867,8 @@ int __uio_register_device(struct module 
 		return -EINVAL;
 
 	info->uio_dev = NULL;
+	INIT_LIST_HEAD(&info->dma_mem);
+	info->dma_mem_size = 0;
 
 	ret = init_uio_class();
 	if (ret)
@@ -706,6 +898,10 @@ int __uio_register_device(struct module 
 		goto err_device_create;
 	}
 
+	ret = uio_validate_mem(idev);
+	if (ret)
+		goto err_validate_mem;
+
 	ret = uio_dev_add_attributes(idev);
 	if (ret)
 		goto err_uio_dev_add_attributes;
@@ -714,7 +910,8 @@ int __uio_register_device(struct module 
 
 	if (idev->info->irq >= 0) {
 		ret = request_irq(idev->info->irq, uio_interrupt,
-				  idev->info->irq_flags, idev->info->name, idev);
+				  idev->info->irq_flags, idev->info->name,
+				  idev);
 		if (ret)
 			goto err_request_irq;
 	}
@@ -723,6 +920,7 @@ int __uio_register_device(struct module 
 
 err_request_irq:
 	uio_dev_del_attributes(idev);
+err_validate_mem:
 err_uio_dev_add_attributes:
 	device_destroy(uio_class->class, MKDEV(uio_major, idev->minor));
 err_device_create:
@@ -754,6 +952,8 @@ void uio_unregister_device(struct uio_in
 	if (info->irq >= 0)
 		free_irq(info->irq, idev);
 
+	uio_remove_all_dma_regions(info);
+
 	uio_dev_del_attributes(idev);
 
 	dev_set_drvdata(idev->dev, NULL);

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

[Leon Woestenberg]

Hello Edward,

On Wed, Dec 3, 2008 at 10:39 PM,  <edward_estabrook@agilent.com> wrote:
> From: Edward Estabrook <Edward_Estabrook@agilent.com>
>
> Here is a patch that adds the ability to dynamically allocate (and use) coherent DMA from userspace by extending the userspace IO driver.  This patch applies against 2.6.28-rc6.
>
You are using a magic value to mmap(), just to keep options open:

Would this in the future allow us to also add support for streaming
non-coherent DMA directly into user space buffers?

Regards,
---
Leon

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

[Greg KH]

On Wed, Dec 03, 2008 at 02:39:44PM -0700, edward_estabrook@agilent.com wrote:
> From: Edward Estabrook <Edward_Estabrook@agilent.com>
> 
> Here is a patch that adds the ability to dynamically allocate (and
> use) coherent DMA from userspace by extending the userspace IO driver.
> This patch applies against 2.6.28-rc6.

Hm, as this patch changes the UIO userspace/kernelspace interface, how
about also documenting it in the existing UIO documentation so that
people know it is there, and so that we can properly review if it is a
sane change.

ALso, for kernel/user api changes, you should cc: the
linux-api@vger.kernel.org mailing list so that it can be reviewed there
by the people trying to maintain the kernel documentation.

thanks,

greg k-h

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

[Greg KH]

On Wed, Dec 03, 2008 at 02:39:44PM -0700, edward_estabrook@agilent.com wrote:
> @@ -82,6 +94,16 @@ static inline int __must_check  extern void uio_unregister_device(struct uio_info *info);  extern void uio_event_notify(struct uio_info *info);
>  
> +/* uio_dev_get_name - return the name associated with this uio device 
> +*/ extern char *uio_dev_get_name(struct uio_device *idev);
> +
> +/* Starting index assigned to dynamically allocated regions. */ #define 
> +UIO_DMA_MEM_BASE_INDEX 1000
> +
> +/* mmap dynamically allocates a DMA memory block if
> + * its offset parameter matches this value. */ #define 
> +UIO_DMA_MEM_ALLOCATE_VM_PGOFF_MAGIC 0xFFFFFUL

Something looks wrong with this patch, is it line wrapped somehow?

odd.

thanks,

greg k-h

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

[Edward Estabrook]

> Something looks wrong with this patch, is it line wrapped somehow?

You are right-- I'll resend using a different mailer.

Cheers,
Ed

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

[Edward Estabrook]

> You are using a magic value to mmap(), just to keep options open:
>
> Would this in the future allow us to also add support for streaming
> non-coherent DMA directly into user space buffers?

Yes, but additional userspace control is also required.
Specifically:
 1) The specification of direction at the time of buffer creation,
    unless force DMA_BIDIRECTIONAL is assumed.
 2) An API to faciliate userspace control over dma_unmap_page() /
    dma_sync_single_for_cpu() calls (perhaps by further overloading
    of the write method).

The use of this magic value does not prevent the future addition of
streaming DMA support, but does not provide sufficient controls.

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

[Peter Zijlstra]

On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> From: Edward Estabrook <Edward_Estabrook@agilent.com>
> 
> Here is a patch that adds the ability to dynamically allocate (and
> use) coherent DMA from userspace by extending the userspace IO driver.
> This patch applies against 2.6.28-rc6.
> 
> The gist of this implementation is to overload uio's mmap
> functionality to allocate and map a new DMA region on demand.  The
> bus-specific DMA address as returned by dma_alloc_coherent is made
> available to userspace in the 1st long word of the newly created
> region (as well as through the conventional 'addr' file in sysfs).  
> 
> To allocate a DMA region you use the following:
> /* Pass this magic number to mmap as offset to dynamically allocate a
> chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> 
> void* memory = mmap (NULL, size, PROT_READ | PROT_WRITE , MAP_SHARED,
> fd, DMA_MEM_ALLOCATE_MMAP_OFFSET); u_int64_t *addr = *(u_int64_t *)
> memory;
> 
> where 'size' is the size in bytes of the region you want and fd is the
> opened /dev/uioN file.
> 
> Allocation occurs in page sized pieces by design to ensure that
> buffers are page-aligned.
> 
> Memory is released when uio_unregister_device() is called.
>
> I have used this extensively on a 2.6.21-based kernel and ported it to
> 2.6.28-rc6 for review / submission here.
> 
> Comments appreciated!

Yuck!

Why not create another special device that will give you DMA memory when
you mmap it? That would also allow you to obtain the physical address
without this utter horrid hack of writing it in the mmap'ed memory.

/dev/uioN-dma would seem like a fine name for that.

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

[Peter Zijlstra]

On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> From: Edward Estabrook <Edward_Estabrook@agilent.com>
> 
> Here is a patch that adds the ability to dynamically allocate (and
> use) coherent DMA from userspace by extending the userspace IO driver.
> This patch applies against 2.6.28-rc6.
> 
> The gist of this implementation is to overload uio's mmap
> functionality to allocate and map a new DMA region on demand.  The
> bus-specific DMA address as returned by dma_alloc_coherent is made
> available to userspace in the 1st long word of the newly created
> region (as well as through the conventional 'addr' file in sysfs).  
> 
> To allocate a DMA region you use the following:
> /* Pass this magic number to mmap as offset to dynamically allocate a
> chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> 
> void* memory = mmap (NULL, size, PROT_READ | PROT_WRITE , MAP_SHARED,
> fd, DMA_MEM_ALLOCATE_MMAP_OFFSET); u_int64_t *addr = *(u_int64_t *)
> memory;
> 
> where 'size' is the size in bytes of the region you want and fd is the
> opened /dev/uioN file.
> 
> Allocation occurs in page sized pieces by design to ensure that
> buffers are page-aligned.
> 
> Memory is released when uio_unregister_device() is called.
>
> I have used this extensively on a 2.6.21-based kernel and ported it to
> 2.6.28-rc6 for review / submission here.
> 
> Comments appreciated!

Yuck!

Why not create another special device that will give you DMA memory when
you mmap it? That would also allow you to obtain the physical address
without this utter horrid hack of writing it in the mmap'ed memory.

/dev/uioN-dma would seem like a fine name for that.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hugh Dickins]

On Thu, 4 Dec 2008, Peter Zijlstra wrote:
> On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> > 
> > The gist of this implementation is to overload uio's mmap
> > functionality to allocate and map a new DMA region on demand.  The
> > bus-specific DMA address as returned by dma_alloc_coherent is made
> > available to userspace in the 1st long word of the newly created
> > region (as well as through the conventional 'addr' file in sysfs).  
> > 
> > To allocate a DMA region you use the following:
> > /* Pass this magic number to mmap as offset to dynamically allocate a
> > chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> > ...
> > Comments appreciated!
> 
> Yuck!
> 
> Why not create another special device that will give you DMA memory when
> you mmap it? That would also allow you to obtain the physical address
> without this utter horrid hack of writing it in the mmap'ed memory.
> 
> /dev/uioN-dma would seem like a fine name for that.

I couldn't agree more.  It sounds fine as a local hack for Edward to
try out some functionality he needed in a hurry; but as something
that should enter the mainline kernel in that form - no.

Hugh

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

[Hugh Dickins]

On Thu, 4 Dec 2008, Peter Zijlstra wrote:
> On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> > 
> > The gist of this implementation is to overload uio's mmap
> > functionality to allocate and map a new DMA region on demand.  The
> > bus-specific DMA address as returned by dma_alloc_coherent is made
> > available to userspace in the 1st long word of the newly created
> > region (as well as through the conventional 'addr' file in sysfs).  
> > 
> > To allocate a DMA region you use the following:
> > /* Pass this magic number to mmap as offset to dynamically allocate a
> > chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> > ...
> > Comments appreciated!
> 
> Yuck!
> 
> Why not create another special device that will give you DMA memory when
> you mmap it? That would also allow you to obtain the physical address
> without this utter horrid hack of writing it in the mmap'ed memory.
> 
> /dev/uioN-dma would seem like a fine name for that.

I couldn't agree more.  It sounds fine as a local hack for Edward to
try out some functionality he needed in a hurry; but as something
that should enter the mainline kernel in that form - no.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hans J. Koch]

On Thu, Dec 04, 2008 at 09:39:02AM +0100, Peter Zijlstra wrote:
> On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> > From: Edward Estabrook <Edward_Estabrook@agilent.com>
> > 
> > Here is a patch that adds the ability to dynamically allocate (and
> > use) coherent DMA from userspace by extending the userspace IO driver.
> > This patch applies against 2.6.28-rc6.
> > 
> > The gist of this implementation is to overload uio's mmap
> > functionality to allocate and map a new DMA region on demand.  The
> > bus-specific DMA address as returned by dma_alloc_coherent is made
> > available to userspace in the 1st long word of the newly created
> > region (as well as through the conventional 'addr' file in sysfs).  
> > 
> > To allocate a DMA region you use the following:
> > /* Pass this magic number to mmap as offset to dynamically allocate a
> > chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> > 
> > void* memory = mmap (NULL, size, PROT_READ | PROT_WRITE , MAP_SHARED,
> > fd, DMA_MEM_ALLOCATE_MMAP_OFFSET); u_int64_t *addr = *(u_int64_t *)
> > memory;
> > 
> > where 'size' is the size in bytes of the region you want and fd is the
> > opened /dev/uioN file.
> > 
> > Allocation occurs in page sized pieces by design to ensure that
> > buffers are page-aligned.
> > 
> > Memory is released when uio_unregister_device() is called.
> >
> > I have used this extensively on a 2.6.21-based kernel and ported it to
> > 2.6.28-rc6 for review / submission here.
> > 
> > Comments appreciated!
> 
> Yuck!
> 
> Why not create another special device that will give you DMA memory when
> you mmap it? That would also allow you to obtain the physical address
> without this utter horrid hack of writing it in the mmap'ed memory.
> 
> /dev/uioN-dma would seem like a fine name for that.

I don't like to have a separate device for DMA memory. It would completely
break the current concept of userspace drivers if you had to get normal
memory from one device and DMA memory from another. Note that one driver
can have both.

But I agree that it's confusing if the physical address is stored somewhere
in the mapped memory. That should simply be omitted, we have that information
in sysfs anyway - like for any other memory mappings. But I guess we need
some kind of "type" or "flags" attribute for the mappings so that userspace
can find out if a mapping is DMA capable or not.

Thanks,
Hans

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

[Hans J. Koch]

On Thu, Dec 04, 2008 at 09:39:02AM +0100, Peter Zijlstra wrote:
> On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> > From: Edward Estabrook <Edward_Estabrook@agilent.com>
> > 
> > Here is a patch that adds the ability to dynamically allocate (and
> > use) coherent DMA from userspace by extending the userspace IO driver.
> > This patch applies against 2.6.28-rc6.
> > 
> > The gist of this implementation is to overload uio's mmap
> > functionality to allocate and map a new DMA region on demand.  The
> > bus-specific DMA address as returned by dma_alloc_coherent is made
> > available to userspace in the 1st long word of the newly created
> > region (as well as through the conventional 'addr' file in sysfs).  
> > 
> > To allocate a DMA region you use the following:
> > /* Pass this magic number to mmap as offset to dynamically allocate a
> > chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> > 
> > void* memory = mmap (NULL, size, PROT_READ | PROT_WRITE , MAP_SHARED,
> > fd, DMA_MEM_ALLOCATE_MMAP_OFFSET); u_int64_t *addr = *(u_int64_t *)
> > memory;
> > 
> > where 'size' is the size in bytes of the region you want and fd is the
> > opened /dev/uioN file.
> > 
> > Allocation occurs in page sized pieces by design to ensure that
> > buffers are page-aligned.
> > 
> > Memory is released when uio_unregister_device() is called.
> >
> > I have used this extensively on a 2.6.21-based kernel and ported it to
> > 2.6.28-rc6 for review / submission here.
> > 
> > Comments appreciated!
> 
> Yuck!
> 
> Why not create another special device that will give you DMA memory when
> you mmap it? That would also allow you to obtain the physical address
> without this utter horrid hack of writing it in the mmap'ed memory.
> 
> /dev/uioN-dma would seem like a fine name for that.

I don't like to have a separate device for DMA memory. It would completely
break the current concept of userspace drivers if you had to get normal
memory from one device and DMA memory from another. Note that one driver
can have both.

But I agree that it's confusing if the physical address is stored somewhere
in the mapped memory. That should simply be omitted, we have that information
in sysfs anyway - like for any other memory mappings. But I guess we need
some kind of "type" or "flags" attribute for the mappings so that userspace
can find out if a mapping is DMA capable or not.

Thanks,
Hans

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Peter Zijlstra]

On Thu, 2008-12-04 at 19:08 +0100, Hans J. Koch wrote:
> On Thu, Dec 04, 2008 at 09:39:02AM +0100, Peter Zijlstra wrote:
> > On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> > > From: Edward Estabrook <Edward_Estabrook@agilent.com>
> > > 
> > > Here is a patch that adds the ability to dynamically allocate (and
> > > use) coherent DMA from userspace by extending the userspace IO driver.
> > > This patch applies against 2.6.28-rc6.
> > > 
> > > The gist of this implementation is to overload uio's mmap
> > > functionality to allocate and map a new DMA region on demand.  The
> > > bus-specific DMA address as returned by dma_alloc_coherent is made
> > > available to userspace in the 1st long word of the newly created
> > > region (as well as through the conventional 'addr' file in sysfs).  
> > > 
> > > To allocate a DMA region you use the following:
> > > /* Pass this magic number to mmap as offset to dynamically allocate a
> > > chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> > > 
> > > void* memory = mmap (NULL, size, PROT_READ | PROT_WRITE , MAP_SHARED,
> > > fd, DMA_MEM_ALLOCATE_MMAP_OFFSET); u_int64_t *addr = *(u_int64_t *)
> > > memory;
> > > 
> > > where 'size' is the size in bytes of the region you want and fd is the
> > > opened /dev/uioN file.
> > > 
> > > Allocation occurs in page sized pieces by design to ensure that
> > > buffers are page-aligned.
> > > 
> > > Memory is released when uio_unregister_device() is called.
> > >
> > > I have used this extensively on a 2.6.21-based kernel and ported it to
> > > 2.6.28-rc6 for review / submission here.
> > > 
> > > Comments appreciated!
> > 
> > Yuck!
> > 
> > Why not create another special device that will give you DMA memory when
> > you mmap it? That would also allow you to obtain the physical address
> > without this utter horrid hack of writing it in the mmap'ed memory.
> > 
> > /dev/uioN-dma would seem like a fine name for that.
> 
> I don't like to have a separate device for DMA memory. It would completely
> break the current concept of userspace drivers if you had to get normal
> memory from one device and DMA memory from another. Note that one driver
> can have both.

How would that break anything, the one driver can simply open both
files.

> But I agree that it's confusing if the physical address is stored somewhere
> in the mapped memory. That should simply be omitted, we have that information
> in sysfs anyway - like for any other memory mappings. But I guess we need
> some kind of "type" or "flags" attribute for the mappings so that userspace
> can find out if a mapping is DMA capable or not.

We have that, different file.

I'll NAK any attempt that rapes the mmap interface like proposed - that
is just not an option.

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

[Peter Zijlstra]

On Thu, 2008-12-04 at 19:08 +0100, Hans J. Koch wrote:
> On Thu, Dec 04, 2008 at 09:39:02AM +0100, Peter Zijlstra wrote:
> > On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
> > > From: Edward Estabrook <Edward_Estabrook@agilent.com>
> > > 
> > > Here is a patch that adds the ability to dynamically allocate (and
> > > use) coherent DMA from userspace by extending the userspace IO driver.
> > > This patch applies against 2.6.28-rc6.
> > > 
> > > The gist of this implementation is to overload uio's mmap
> > > functionality to allocate and map a new DMA region on demand.  The
> > > bus-specific DMA address as returned by dma_alloc_coherent is made
> > > available to userspace in the 1st long word of the newly created
> > > region (as well as through the conventional 'addr' file in sysfs).  
> > > 
> > > To allocate a DMA region you use the following:
> > > /* Pass this magic number to mmap as offset to dynamically allocate a
> > > chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
> > > 
> > > void* memory = mmap (NULL, size, PROT_READ | PROT_WRITE , MAP_SHARED,
> > > fd, DMA_MEM_ALLOCATE_MMAP_OFFSET); u_int64_t *addr = *(u_int64_t *)
> > > memory;
> > > 
> > > where 'size' is the size in bytes of the region you want and fd is the
> > > opened /dev/uioN file.
> > > 
> > > Allocation occurs in page sized pieces by design to ensure that
> > > buffers are page-aligned.
> > > 
> > > Memory is released when uio_unregister_device() is called.
> > >
> > > I have used this extensively on a 2.6.21-based kernel and ported it to
> > > 2.6.28-rc6 for review / submission here.
> > > 
> > > Comments appreciated!
> > 
> > Yuck!
> > 
> > Why not create another special device that will give you DMA memory when
> > you mmap it? That would also allow you to obtain the physical address
> > without this utter horrid hack of writing it in the mmap'ed memory.
> > 
> > /dev/uioN-dma would seem like a fine name for that.
> 
> I don't like to have a separate device for DMA memory. It would completely
> break the current concept of userspace drivers if you had to get normal
> memory from one device and DMA memory from another. Note that one driver
> can have both.

How would that break anything, the one driver can simply open both
files.

> But I agree that it's confusing if the physical address is stored somewhere
> in the mapped memory. That should simply be omitted, we have that information
> in sysfs anyway - like for any other memory mappings. But I guess we need
> some kind of "type" or "flags" attribute for the mappings so that userspace
> can find out if a mapping is DMA capable or not.

We have that, different file.

I'll NAK any attempt that rapes the mmap interface like proposed - that
is just not an option.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hans J. Koch]

On Fri, Dec 05, 2008 at 08:10:59AM +0100, Peter Zijlstra wrote:
> > I don't like to have a separate device for DMA memory. It would completely
> > break the current concept of userspace drivers if you had to get normal
> > memory from one device and DMA memory from another. Note that one driver
> > can have both.
> 
> How would that break anything, the one driver can simply open both
> files.

I agree there's not much breakage, but it's a difference in handling things.
People who wrote libraries that generically find mappable memory of a UIO
device need to change their handling.

> 
> > But I agree that it's confusing if the physical address is stored somewhere
> > in the mapped memory. That should simply be omitted, we have that information
> > in sysfs anyway - like for any other memory mappings. But I guess we need
> > some kind of "type" or "flags" attribute for the mappings so that userspace
> > can find out if a mapping is DMA capable or not.
> 
> We have that, different file.
> 
> I'll NAK any attempt that rapes the mmap interface like proposed - that
> is just not an option.

Well, UIO already rapes the mmap interface by using the "offset" parameter to
pass in the number of the mapping.
But I'll NAK the current concept, too. It's a UIO kernel driver's task to tell
userspace which memory a device has to offer. The UIO core prevents userspace
as much as possible from mapping anything different. And it should stay that
way.

Thanks,
Hans

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

[Hans J. Koch]

On Fri, Dec 05, 2008 at 08:10:59AM +0100, Peter Zijlstra wrote:
> > I don't like to have a separate device for DMA memory. It would completely
> > break the current concept of userspace drivers if you had to get normal
> > memory from one device and DMA memory from another. Note that one driver
> > can have both.
> 
> How would that break anything, the one driver can simply open both
> files.

I agree there's not much breakage, but it's a difference in handling things.
People who wrote libraries that generically find mappable memory of a UIO
device need to change their handling.

> 
> > But I agree that it's confusing if the physical address is stored somewhere
> > in the mapped memory. That should simply be omitted, we have that information
> > in sysfs anyway - like for any other memory mappings. But I guess we need
> > some kind of "type" or "flags" attribute for the mappings so that userspace
> > can find out if a mapping is DMA capable or not.
> 
> We have that, different file.
> 
> I'll NAK any attempt that rapes the mmap interface like proposed - that
> is just not an option.

Well, UIO already rapes the mmap interface by using the "offset" parameter to
pass in the number of the mapping.
But I'll NAK the current concept, too. It's a UIO kernel driver's task to tell
userspace which memory a device has to offer. The UIO core prevents userspace
as much as possible from mapping anything different. And it should stay that
way.

Thanks,
Hans

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Edward Estabrook]

> Well, UIO already rapes the mmap interface by using the "offset" parameter to
> pass in the number of the mapping.

Exactly.

> But I'll NAK the current concept, too. It's a UIO kernel driver's task to tell
> userspace which memory a device has to offer. The UIO core prevents userspace
> as much as possible from mapping anything different. And it should stay that
> way.

The ultimate purpose (I thought) of the UIO driver is to simplify
driver development
by pushing device control into userspace.  There is a very real need
for efficient
dynamic control over the DMA allocation of a device.  Why not 'allow' this to
happen in userspace if it can be done safely and without breaking anything else?

Remember that for devices employing ring buffers it is not a question of
'how much memory a device has to offer' but rather 'how much system
memory would the
driver like to configure that device to use'.

I don't want to stop my DMA engine and reload the driver to create
more buffers (and I don't
want to pre-allocate more than I need as contingency).

Cheers,
Ed

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

[Edward Estabrook]

> Well, UIO already rapes the mmap interface by using the "offset" parameter to
> pass in the number of the mapping.

Exactly.

> But I'll NAK the current concept, too. It's a UIO kernel driver's task to tell
> userspace which memory a device has to offer. The UIO core prevents userspace
> as much as possible from mapping anything different. And it should stay that
> way.

The ultimate purpose (I thought) of the UIO driver is to simplify
driver development
by pushing device control into userspace.  There is a very real need
for efficient
dynamic control over the DMA allocation of a device.  Why not 'allow' this to
happen in userspace if it can be done safely and without breaking anything else?

Remember that for devices employing ring buffers it is not a question of
'how much memory a device has to offer' but rather 'how much system
memory would the
driver like to configure that device to use'.

I don't want to stop my DMA engine and reload the driver to create
more buffers (and I don't
want to pre-allocate more than I need as contingency).

Cheers,
Ed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Peter Zijlstra]

On Fri, 2008-12-05 at 16:32 -0800, Edward Estabrook wrote:
> > Well, UIO already rapes the mmap interface by using the "offset" parameter to
> > pass in the number of the mapping.
> 
> Exactly.

Had I known about it then, I'd NAK'd it, but I guess now that its
already merged changing it will be hard :/

Also, having done something bad in the past doesn't mean you can
continue doing the wrong thing.

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

[Hans J. Koch]

On Fri, Dec 12, 2008 at 06:25:12PM +0100, Peter Zijlstra wrote:
> On Fri, 2008-12-05 at 16:32 -0800, Edward Estabrook wrote:
> > > Well, UIO already rapes the mmap interface by using the "offset" parameter to
> > > pass in the number of the mapping.
> > 
> > Exactly.
> 
> Had I known about it then, I'd NAK'd it, but I guess now that its
> already merged changing it will be hard :/

It was in -mm for half a year before it went to mainline in 2.6.23, the
documentation being present all the time. It was discussed intensively
on lkml, and several core kernel developers reviewed it. The special use
of the mmap() offset parameter was never even mentioned by anybody. I
remember that so well because I actually expected critizism, but everybody
was fine with it. And to be honest, even though it's unusual, I still find
it a good solution.

Thanks,
Hans

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

[Max Krasnyansky]

Hugh Dickins wrote:
> On Thu, 4 Dec 2008, Peter Zijlstra wrote:
>> On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
>>> The gist of this implementation is to overload uio's mmap
>>> functionality to allocate and map a new DMA region on demand.  The
>>> bus-specific DMA address as returned by dma_alloc_coherent is made
>>> available to userspace in the 1st long word of the newly created
>>> region (as well as through the conventional 'addr' file in sysfs).  
>>>
>>> To allocate a DMA region you use the following:
>>> /* Pass this magic number to mmap as offset to dynamically allocate a
>>> chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
>>> ...
>>> Comments appreciated!
>> Yuck!
>>
>> Why not create another special device that will give you DMA memory when
>> you mmap it? That would also allow you to obtain the physical address
>> without this utter horrid hack of writing it in the mmap'ed memory.
>>
>> /dev/uioN-dma would seem like a fine name for that.
> 
> I couldn't agree more.  It sounds fine as a local hack for Edward to
> try out some functionality he needed in a hurry; but as something
> that should enter the mainline kernel in that form - no.

Agree with Peter and Hugh here. Also I have a use case where I need to share
DMA buffers between two or more devices. So I think we need a generic DMA
device that does operations like alloc, mmap, etc. Mmapped regions can then be
used with UIO devices.
I'll put together a prototype of that some time early next year.

Max

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

[Max Krasnyansky]

Hugh Dickins wrote:
> On Thu, 4 Dec 2008, Peter Zijlstra wrote:
>> On Wed, 2008-12-03 at 14:39 -0700, edward_estabrook@agilent.com wrote:
>>> The gist of this implementation is to overload uio's mmap
>>> functionality to allocate and map a new DMA region on demand.  The
>>> bus-specific DMA address as returned by dma_alloc_coherent is made
>>> available to userspace in the 1st long word of the newly created
>>> region (as well as through the conventional 'addr' file in sysfs).  
>>>
>>> To allocate a DMA region you use the following:
>>> /* Pass this magic number to mmap as offset to dynamically allocate a
>>> chunk of memory */ #define DMA_MEM_ALLOCATE_MMAP_OFFSET 0xFFFFF000UL
>>> ...
>>> Comments appreciated!
>> Yuck!
>>
>> Why not create another special device that will give you DMA memory when
>> you mmap it? That would also allow you to obtain the physical address
>> without this utter horrid hack of writing it in the mmap'ed memory.
>>
>> /dev/uioN-dma would seem like a fine name for that.
> 
> I couldn't agree more.  It sounds fine as a local hack for Edward to
> try out some functionality he needed in a hurry; but as something
> that should enter the mainline kernel in that form - no.

Agree with Peter and Hugh here. Also I have a use case where I need to share
DMA buffers between two or more devices. So I think we need a generic DMA
device that does operations like alloc, mmap, etc. Mmapped regions can then be
used with UIO devices.
I'll put together a prototype of that some time early next year.

Max




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Earl Chew]

I'm taking another look at the changes that were submitted in

http://lkml.org/lkml/2008/12/3/453

to see if they can be made more palatable.


In http://lkml.org/lkml/2008/12/4/64 you wrote:

> Why not create another special device that will give you DMA memory when
> you mmap it? That would also allow you to obtain the physical address
> without this utter horrid hack of writing it in the mmap'ed memory.
> 
> /dev/uioN-dma would seem like a fine name for that.


I understand the main objection was the hack to return the physical
address of the allocated DMA buffer within the buffer itself amongst
some other things.

Your suggestion was to create /dev/uioN-dma for the purpose of
allocating DMA memory.

I'm having trouble figuring out how this would help to return the
physical (bus) address of the DMA memory in a more elegant manner.

What idea did you have for the userspace program to obtain
the physical (bus) of the allocated DMA memory buffer?


Earl

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

[Earl Chew]

I'm taking another look at the changes that were submitted in

http://lkml.org/lkml/2008/12/3/453

to see if they can be made more palatable.


In http://lkml.org/lkml/2008/12/4/64 you wrote:

> Why not create another special device that will give you DMA memory when
> you mmap it? That would also allow you to obtain the physical address
> without this utter horrid hack of writing it in the mmap'ed memory.
> 
> /dev/uioN-dma would seem like a fine name for that.


I understand the main objection was the hack to return the physical
address of the allocated DMA buffer within the buffer itself amongst
some other things.

Your suggestion was to create /dev/uioN-dma for the purpose of
allocating DMA memory.

I'm having trouble figuring out how this would help to return the
physical (bus) address of the DMA memory in a more elegant manner.

What idea did you have for the userspace program to obtain
the physical (bus) of the allocated DMA memory buffer?


Earl


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hans J. Koch]

On Fri, Dec 11, 2009 at 04:02:17PM -0800, Earl Chew wrote:
> I'm taking another look at the changes that were submitted in
> 
> http://lkml.org/lkml/2008/12/3/453
> 
> to see if they can be made more palatable.
> 
> 
> In http://lkml.org/lkml/2008/12/4/64 you wrote:
> 
> > Why not create another special device that will give you DMA memory when
> > you mmap it? That would also allow you to obtain the physical address
> > without this utter horrid hack of writing it in the mmap'ed memory.
> > 
> > /dev/uioN-dma would seem like a fine name for that.
> 
> 
> I understand the main objection was the hack to return the physical
> address of the allocated DMA buffer within the buffer itself amongst
> some other things.

The general thing is this: The UIO core supports only static mappings.
The possible number of mappings is usually set at compile time or module
load time and is currently limited to MAX_UIO_MAPS (== 5). This number
is usually sufficient for devices like PCI cards, which have a limited
number of mappings, too. All drivers currently in the kernel only need
one or two.

The current implementation of the UIO core is simply not made for
dynamic allocation of an unlimited amount of new mappings at runtime. As
we have seen in this patch, it needs raping of a documented kernel
interface to userspace. This is not acceptable.

So the easiest correct solution is to create a new device (e.g.
/dev/uioN-dma, as Peter suggested). It should only be created for a UIO
driver if it has a certain flag set, something like UIO_NEEDS_DYN_DMA_ALLOC.

> 
> Your suggestion was to create /dev/uioN-dma for the purpose of
> allocating DMA memory.
> 
> I'm having trouble figuring out how this would help to return the
> physical (bus) address of the DMA memory in a more elegant manner.

If you create this new device, you can invent any (reasonable) interface you
like. It should probably be something in sysfs, where you can write to a
file to allocate a new buffer, and read the address from some other.
It should also be possible to free a buffer again.

Thanks,
Hans

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

[Hans J. Koch]

On Fri, Dec 11, 2009 at 04:02:17PM -0800, Earl Chew wrote:
> I'm taking another look at the changes that were submitted in
> 
> http://lkml.org/lkml/2008/12/3/453
> 
> to see if they can be made more palatable.
> 
> 
> In http://lkml.org/lkml/2008/12/4/64 you wrote:
> 
> > Why not create another special device that will give you DMA memory when
> > you mmap it? That would also allow you to obtain the physical address
> > without this utter horrid hack of writing it in the mmap'ed memory.
> > 
> > /dev/uioN-dma would seem like a fine name for that.
> 
> 
> I understand the main objection was the hack to return the physical
> address of the allocated DMA buffer within the buffer itself amongst
> some other things.

The general thing is this: The UIO core supports only static mappings.
The possible number of mappings is usually set at compile time or module
load time and is currently limited to MAX_UIO_MAPS (== 5). This number
is usually sufficient for devices like PCI cards, which have a limited
number of mappings, too. All drivers currently in the kernel only need
one or two.

The current implementation of the UIO core is simply not made for
dynamic allocation of an unlimited amount of new mappings at runtime. As
we have seen in this patch, it needs raping of a documented kernel
interface to userspace. This is not acceptable.

So the easiest correct solution is to create a new device (e.g.
/dev/uioN-dma, as Peter suggested). It should only be created for a UIO
driver if it has a certain flag set, something like UIO_NEEDS_DYN_DMA_ALLOC.

> 
> Your suggestion was to create /dev/uioN-dma for the purpose of
> allocating DMA memory.
> 
> I'm having trouble figuring out how this would help to return the
> physical (bus) address of the DMA memory in a more elegant manner.

If you create this new device, you can invent any (reasonable) interface you
like. It should probably be something in sysfs, where you can write to a
file to allocate a new buffer, and read the address from some other.
It should also be possible to free a buffer again.

Thanks,
Hans

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Earl Chew]

Hans,

Thanks for the considered reply.


Hans J. Koch wrote:
> The general thing is this: The UIO core supports only static mappings.
> The possible number of mappings is usually set at compile time or module
> load time and is currently limited to MAX_UIO_MAPS (== 5). This number
> is usually sufficient for devices like PCI cards, which have a limited
> number of mappings, too. All drivers currently in the kernel only need
> one or two.


I'd like to proceed by changing struct uio_mem [MAX_UIO_MAPS] to a
linked list.

The driver code in uio_find_mem_index(), uio_dev_add_attributes(), etc,
iterate through the (small) array anyway, and the list space and
performance overhead is not significant for the cases mentioned.

Such a change would make it easier to track dynamically allocated
regions as well as pre-allocated mapping regions in the same data
structure.

It also plays more nicely into the next part ...

> The current implementation of the UIO core is simply not made for
> dynamic allocation of an unlimited amount of new mappings at runtime. As
> we have seen in this patch, it needs raping of a documented kernel
> interface to userspace. This is not acceptable.
> 
> So the easiest correct solution is to create a new device (e.g.
> /dev/uioN-dma, as Peter suggested). It should only be created for a UIO
> driver if it has a certain flag set, something like UIO_NEEDS_DYN_DMA_ALLOC.

An approach which would play better with our existing codebase would
be to introduce a two-step ioctl-mmap.

a. Use an ioctl() to allocate the DMA buffer. The ioctl returns two
   things:

	1.  A mapping (page) number
	2.  A physical (bus) address


b. Use the existing mmap() interface to gain access to the
   DMA buffer allocated in (a). Clients would invoke mmap()
   and use the mapping (page) number returned in (a) to
   obtain userspace access to the DMA buffer.


I think that the second step (b) would play nicely with the existing
mmap() interface exposed by the UIO driver.


Using an ioctl() provides a cleaner way to return the physical
(bus) address of the DMA buffer.


Existing client code that is not interested in DMA buffers do
not incur a penalty because it will not invoke the new ioctl().


Earl

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

[Earl Chew]

Hans,

Thanks for the considered reply.


Hans J. Koch wrote:
> The general thing is this: The UIO core supports only static mappings.
> The possible number of mappings is usually set at compile time or module
> load time and is currently limited to MAX_UIO_MAPS (== 5). This number
> is usually sufficient for devices like PCI cards, which have a limited
> number of mappings, too. All drivers currently in the kernel only need
> one or two.


I'd like to proceed by changing struct uio_mem [MAX_UIO_MAPS] to a
linked list.

The driver code in uio_find_mem_index(), uio_dev_add_attributes(), etc,
iterate through the (small) array anyway, and the list space and
performance overhead is not significant for the cases mentioned.

Such a change would make it easier to track dynamically allocated
regions as well as pre-allocated mapping regions in the same data
structure.

It also plays more nicely into the next part ...

> The current implementation of the UIO core is simply not made for
> dynamic allocation of an unlimited amount of new mappings at runtime. As
> we have seen in this patch, it needs raping of a documented kernel
> interface to userspace. This is not acceptable.
> 
> So the easiest correct solution is to create a new device (e.g.
> /dev/uioN-dma, as Peter suggested). It should only be created for a UIO
> driver if it has a certain flag set, something like UIO_NEEDS_DYN_DMA_ALLOC.

An approach which would play better with our existing codebase would
be to introduce a two-step ioctl-mmap.

a. Use an ioctl() to allocate the DMA buffer. The ioctl returns two
   things:

	1.  A mapping (page) number
	2.  A physical (bus) address


b. Use the existing mmap() interface to gain access to the
   DMA buffer allocated in (a). Clients would invoke mmap()
   and use the mapping (page) number returned in (a) to
   obtain userspace access to the DMA buffer.


I think that the second step (b) would play nicely with the existing
mmap() interface exposed by the UIO driver.


Using an ioctl() provides a cleaner way to return the physical
(bus) address of the DMA buffer.


Existing client code that is not interested in DMA buffers do
not incur a penalty because it will not invoke the new ioctl().


Earl


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Earl Chew]

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].

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)

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

[Earl Chew]

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].

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)


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hans J. Koch]

On Tue, Dec 15, 2009 at 05:34:19AM -0800, Earl Chew wrote:
> Hans,

Hi Earl,

> 
> Thanks for the considered reply.
> 
> 
> Hans J. Koch wrote:
> > The general thing is this: The UIO core supports only static mappings.
> > The possible number of mappings is usually set at compile time or module
> > load time and is currently limited to MAX_UIO_MAPS (== 5). This number
> > is usually sufficient for devices like PCI cards, which have a limited
> > number of mappings, too. All drivers currently in the kernel only need
> > one or two.
> 
> 
> I'd like to proceed by changing struct uio_mem [MAX_UIO_MAPS] to a
> linked list.
> 
> The driver code in uio_find_mem_index(), uio_dev_add_attributes(), etc,
> iterate through the (small) array anyway, and the list space and
> performance overhead is not significant for the cases mentioned.
> 
> Such a change would make it easier to track dynamically allocated
> regions as well as pre-allocated mapping regions in the same data
> structure.

Sorry, I think I wasn't clear enough: The current interface for static
mappings shouldn't be changed. Dynamically added mappings need a new
interface.

> 
> It also plays more nicely into the next part ...
> 
> > The current implementation of the UIO core is simply not made for
> > dynamic allocation of an unlimited amount of new mappings at runtime. As
> > we have seen in this patch, it needs raping of a documented kernel
> > interface to userspace. This is not acceptable.
> > 
> > So the easiest correct solution is to create a new device (e.g.
> > /dev/uioN-dma, as Peter suggested). It should only be created for a UIO
> > driver if it has a certain flag set, something like UIO_NEEDS_DYN_DMA_ALLOC.
> 
> An approach which would play better with our existing codebase would
> be to introduce a two-step ioctl-mmap.
> 
> a. Use an ioctl() to allocate the DMA buffer. The ioctl returns two
>    things:

No. We don't want any new ioctls in the kernel.

> 
> 	1.  A mapping (page) number
> 	2.  A physical (bus) address
> 
> 
> b. Use the existing mmap() interface to gain access to the
>    DMA buffer allocated in (a). Clients would invoke mmap()
>    and use the mapping (page) number returned in (a) to
>    obtain userspace access to the DMA buffer.
> 
> 
> I think that the second step (b) would play nicely with the existing
> mmap() interface exposed by the UIO driver.

The existing interface is for static mappings only.

> 
> 
> Using an ioctl() provides a cleaner way to return the physical
> (bus) address of the DMA buffer.

ioctl() is out of fashion today. We have sysfs. Note that ioctls are neither
typesafe nor much faster than sysfs.

> 
> 
> Existing client code that is not interested in DMA buffers do
> not incur a penalty because it will not invoke the new ioctl().

What about userspace tools that rely on the fact that the number of mappings
for a UIO device cannot change? This is a documented property of UIO.

Dynamically allocated mappings really call for a new device as Peter suggested.
In fact, that would make life much easier for you. Since your the one who
implements that stuff, your free to define a new interface. Surely that new
interface will be discussed and rejected two or three times, but in the end
we'll have a nice interface that allows UIO to use DMA, even with dyynamically
allocated buffers.

Use that freedom and create a new device with a new interface. There's no
point in trying to change existing and well documented interfaces to userspace.

Thanks,
Hans

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

[Hans J. Koch]

On Tue, Dec 15, 2009 at 05:34:19AM -0800, Earl Chew wrote:
> Hans,

Hi Earl,

> 
> Thanks for the considered reply.
> 
> 
> Hans J. Koch wrote:
> > The general thing is this: The UIO core supports only static mappings.
> > The possible number of mappings is usually set at compile time or module
> > load time and is currently limited to MAX_UIO_MAPS (== 5). This number
> > is usually sufficient for devices like PCI cards, which have a limited
> > number of mappings, too. All drivers currently in the kernel only need
> > one or two.
> 
> 
> I'd like to proceed by changing struct uio_mem [MAX_UIO_MAPS] to a
> linked list.
> 
> The driver code in uio_find_mem_index(), uio_dev_add_attributes(), etc,
> iterate through the (small) array anyway, and the list space and
> performance overhead is not significant for the cases mentioned.
> 
> Such a change would make it easier to track dynamically allocated
> regions as well as pre-allocated mapping regions in the same data
> structure.

Sorry, I think I wasn't clear enough: The current interface for static
mappings shouldn't be changed. Dynamically added mappings need a new
interface.

> 
> It also plays more nicely into the next part ...
> 
> > The current implementation of the UIO core is simply not made for
> > dynamic allocation of an unlimited amount of new mappings at runtime. As
> > we have seen in this patch, it needs raping of a documented kernel
> > interface to userspace. This is not acceptable.
> > 
> > So the easiest correct solution is to create a new device (e.g.
> > /dev/uioN-dma, as Peter suggested). It should only be created for a UIO
> > driver if it has a certain flag set, something like UIO_NEEDS_DYN_DMA_ALLOC.
> 
> An approach which would play better with our existing codebase would
> be to introduce a two-step ioctl-mmap.
> 
> a. Use an ioctl() to allocate the DMA buffer. The ioctl returns two
>    things:

No. We don't want any new ioctls in the kernel.

> 
> 	1.  A mapping (page) number
> 	2.  A physical (bus) address
> 
> 
> b. Use the existing mmap() interface to gain access to the
>    DMA buffer allocated in (a). Clients would invoke mmap()
>    and use the mapping (page) number returned in (a) to
>    obtain userspace access to the DMA buffer.
> 
> 
> I think that the second step (b) would play nicely with the existing
> mmap() interface exposed by the UIO driver.

The existing interface is for static mappings only.

> 
> 
> Using an ioctl() provides a cleaner way to return the physical
> (bus) address of the DMA buffer.

ioctl() is out of fashion today. We have sysfs. Note that ioctls are neither
typesafe nor much faster than sysfs.

> 
> 
> Existing client code that is not interested in DMA buffers do
> not incur a penalty because it will not invoke the new ioctl().

What about userspace tools that rely on the fact that the number of mappings
for a UIO device cannot change? This is a documented property of UIO.

Dynamically allocated mappings really call for a new device as Peter suggested.
In fact, that would make life much easier for you. Since your the one who
implements that stuff, your free to define a new interface. Surely that new
interface will be discussed and rejected two or three times, but in the end
we'll have a nice interface that allows UIO to use DMA, even with dyynamically
allocated buffers.

Use that freedom and create a new device with a new interface. There's no
point in trying to change existing and well documented interfaces to userspace.

Thanks,
Hans

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hans J. Koch]

(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)
> 

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

[Hans J. Koch]

(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)
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Earl Chew]

Hans J. Koch wrote:
> Sorry, I think I wasn't clear enough: The current interface for static
> mappings shouldn't be changed. Dynamically added mappings need a new
> interface.

Thanks for the quick reply.

Are you ok with changes to the (internal) struct uio_device ?

This content of this structure is only known to uio.c and only its
name is exposed through the client visible uio_driver.h.

Earl

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

[Earl Chew]

Hans J. Koch wrote:
> Sorry, I think I wasn't clear enough: The current interface for static
> mappings shouldn't be changed. Dynamically added mappings need a new
> interface.

Thanks for the quick reply.

Are you ok with changes to the (internal) struct uio_device ?

This content of this structure is only known to uio.c and only its
name is exposed through the client visible uio_driver.h.

Earl



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hans J. Koch]

On Tue, Dec 15, 2009 at 01:47:04PM -0800, Earl Chew wrote:
> Hans J. Koch wrote:
> > Sorry, I think I wasn't clear enough: The current interface for static
> > mappings shouldn't be changed. Dynamically added mappings need a new
> > interface.
> 
> Thanks for the quick reply.
> 
> Are you ok with changes to the (internal) struct uio_device ?

Hey, we live in a free world :)
Anything can be changed as long as it's a technically sensible solution and
doesn't break existing interfaces to userspace.

The DMA-for-UIO thing is something that shouldn't be taken lightly. If we
define an interface for that, it should cover all possible applications.
There are not only devices that need to allocate new DMA buffers at runtime,
but also devices which could very well live with one or two statically
allocated DMA buffers. We need to cover all these cases.

One example: An A/D converter has an on-chip 32k buffer. It causes an
interrupt as soon as the buffer is filled up to a certain high-water mark.
Such cases would easily fit into the current UIO system. The UIO core could
simply DMA the data to one of the mappings. A new flag for that mapping and
a few other changes are all it takes. After the DMA transfer is complete, the
interrupt is passed on to userspace, which would find the buffer already
filled with the desired data. Just a thought, unfortunately I haven't got
such hardware to try it.

When it comes to dynamically allocated DMA buffers, it might well be possible
to add a new directory in sysfs besides the "mem" directory, e.g. something
like /sys/class/uio/uioN/dma-mem/. This would save us the trouble of creating
a new device. Maybe the example above would better fit in here, too. Who knows.

These are only some thoughts, I haven't got any DMA capable hardware to deal
with ATM.

You certainly notice that there are important design decisions to make.
Remember that once a kernel interface to userspace exists, it is etched in
stone forever.

Thanks,
Hans

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

[Hans J. Koch]

On Tue, Dec 15, 2009 at 01:47:04PM -0800, Earl Chew wrote:
> Hans J. Koch wrote:
> > Sorry, I think I wasn't clear enough: The current interface for static
> > mappings shouldn't be changed. Dynamically added mappings need a new
> > interface.
> 
> Thanks for the quick reply.
> 
> Are you ok with changes to the (internal) struct uio_device ?

Hey, we live in a free world :)
Anything can be changed as long as it's a technically sensible solution and
doesn't break existing interfaces to userspace.

The DMA-for-UIO thing is something that shouldn't be taken lightly. If we
define an interface for that, it should cover all possible applications.
There are not only devices that need to allocate new DMA buffers at runtime,
but also devices which could very well live with one or two statically
allocated DMA buffers. We need to cover all these cases.

One example: An A/D converter has an on-chip 32k buffer. It causes an
interrupt as soon as the buffer is filled up to a certain high-water mark.
Such cases would easily fit into the current UIO system. The UIO core could
simply DMA the data to one of the mappings. A new flag for that mapping and
a few other changes are all it takes. After the DMA transfer is complete, the
interrupt is passed on to userspace, which would find the buffer already
filled with the desired data. Just a thought, unfortunately I haven't got
such hardware to try it.

When it comes to dynamically allocated DMA buffers, it might well be possible
to add a new directory in sysfs besides the "mem" directory, e.g. something
like /sys/class/uio/uioN/dma-mem/. This would save us the trouble of creating
a new device. Maybe the example above would better fit in here, too. Who knows.

These are only some thoughts, I haven't got any DMA capable hardware to deal
with ATM.

You certainly notice that there are important design decisions to make.
Remember that once a kernel interface to userspace exists, it is etched in
stone forever.

Thanks,
Hans

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Earl Chew]

Hans J. Koch wrote:
> One example: An A/D converter has an on-chip 32k buffer. It causes an
> interrupt as soon as the buffer is filled up to a certain high-water mark.
> Such cases would easily fit into the current UIO system. The UIO core could
> simply DMA the data to one of the mappings. A new flag for that mapping and
> a few other changes are all it takes. After the DMA transfer is complete, the
> interrupt is passed on to userspace, which would find the buffer already
> filled with the desired data. Just a thought, unfortunately I haven't got
> such hardware to try it.

Hans,

Is this case already covered by the pre-existing UIO_MEM_LOGICAL
option ?

I'm thinking that since the memory is statically defined, it can be
described using one of the existing struct uio_mem mem[] slots in
struct uio_info and marked as UIO_MEM_LOGICAL.

The userspace program can map that into its process space using the
existing mmap() interface.

What am I missing?

> When it comes to dynamically allocated DMA buffers, it might well be possible
> to add a new directory in sysfs besides the "mem" directory, e.g. something
> like /sys/class/uio/uioN/dma-mem/. This would save us the trouble of creating
> a new device. Maybe the example above would better fit in here, too. Who knows.

I looked at the 2.6.32 source at

http://lxr.linux.no/#linux+v2.6.32/drivers/uio/uio.c

and didn't see any reference to /sys/class/uio/uioN/mem .  Perhaps
you're referring to something new.

In any case, I think you're describing adding

/sys/class/uio/uioN/dma-mem

as a means to control /dev/uioN .  Presumably writing to
/sys/class/uio/uioN/dma-mem would create additional dynamic
DMA buffers.

I can't yet see a way to make this request-response. When requesting
a dynamic buffer I need to indicate the size that I want, and in
return I need to obtain a handle to the buffer (either its mapping
number, address, etc). Once I have that, I can query other
interesting information (eg its bus address).


Earl

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

[Earl Chew]

Hans J. Koch wrote:
> One example: An A/D converter has an on-chip 32k buffer. It causes an
> interrupt as soon as the buffer is filled up to a certain high-water mark.
> Such cases would easily fit into the current UIO system. The UIO core could
> simply DMA the data to one of the mappings. A new flag for that mapping and
> a few other changes are all it takes. After the DMA transfer is complete, the
> interrupt is passed on to userspace, which would find the buffer already
> filled with the desired data. Just a thought, unfortunately I haven't got
> such hardware to try it.

Hans,

Is this case already covered by the pre-existing UIO_MEM_LOGICAL
option ?

I'm thinking that since the memory is statically defined, it can be
described using one of the existing struct uio_mem mem[] slots in
struct uio_info and marked as UIO_MEM_LOGICAL.

The userspace program can map that into its process space using the
existing mmap() interface.

What am I missing?

> When it comes to dynamically allocated DMA buffers, it might well be possible
> to add a new directory in sysfs besides the "mem" directory, e.g. something
> like /sys/class/uio/uioN/dma-mem/. This would save us the trouble of creating
> a new device. Maybe the example above would better fit in here, too. Who knows.

I looked at the 2.6.32 source at

http://lxr.linux.no/#linux+v2.6.32/drivers/uio/uio.c

and didn't see any reference to /sys/class/uio/uioN/mem .  Perhaps
you're referring to something new.

In any case, I think you're describing adding

/sys/class/uio/uioN/dma-mem

as a means to control /dev/uioN .  Presumably writing to
/sys/class/uio/uioN/dma-mem would create additional dynamic
DMA buffers.

I can't yet see a way to make this request-response. When requesting
a dynamic buffer I need to indicate the size that I want, and in
return I need to obtain a handle to the buffer (either its mapping
number, address, etc). Once I have that, I can query other
interesting information (eg its bus address).


Earl



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hans J. Koch]

On Tue, Dec 15, 2009 at 04:20:56PM -0800, Earl Chew wrote:
> Hans J. Koch wrote:
> > One example: An A/D converter has an on-chip 32k buffer. It causes an
> > interrupt as soon as the buffer is filled up to a certain high-water mark.
> > Such cases would easily fit into the current UIO system. The UIO core could
> > simply DMA the data to one of the mappings. A new flag for that mapping and
> > a few other changes are all it takes. After the DMA transfer is complete, the
> > interrupt is passed on to userspace, which would find the buffer already
> > filled with the desired data. Just a thought, unfortunately I haven't got
> > such hardware to try it.
> 
> Hans,
> 
> Is this case already covered by the pre-existing UIO_MEM_LOGICAL
> option ?
> 
> I'm thinking that since the memory is statically defined, it can be
> described using one of the existing struct uio_mem mem[] slots in
> struct uio_info and marked as UIO_MEM_LOGICAL.
> 
> The userspace program can map that into its process space using the
> existing mmap() interface.
> 
> What am I missing?

Nothing. The UIO core can map all kinds of memory. I thought about a generic
DMA routine that does the transfer if a flag like UIO_DMA_FROM_DEVICE_PRE_USER
is set.

> 
> > When it comes to dynamically allocated DMA buffers, it might well be possible
> > to add a new directory in sysfs besides the "mem" directory, e.g. something
> > like /sys/class/uio/uioN/dma-mem/. This would save us the trouble of creating
> > a new device. Maybe the example above would better fit in here, too. Who knows.
> 
> I looked at the 2.6.32 source at
> 
> http://lxr.linux.no/#linux+v2.6.32/drivers/uio/uio.c
> 
> and didn't see any reference to /sys/class/uio/uioN/mem .  Perhaps
> you're referring to something new.

Just look at the sysfs files you get when creating a UIO device, then look at
uio.c to see how it is done.

> 
> In any case, I think you're describing adding
> 
> /sys/class/uio/uioN/dma-mem
> 
> as a means to control /dev/uioN .  Presumably writing to
> /sys/class/uio/uioN/dma-mem would create additional dynamic
> DMA buffers.

No, dma-mem would be a directory containing some more attributes. Maybe one
called "create" that allocates a new buffer.

> 
> I can't yet see a way to make this request-response. When requesting
> a dynamic buffer I need to indicate the size that I want, and in
> return I need to obtain a handle to the buffer (either its mapping
> number, address, etc). Once I have that, I can query other
> interesting information (eg its bus address).

Writing the size to that supposed "create" attribute could allocate the
buffer and and create more attributes that contain the information you need.

Thanks,
Hans

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

[Hans J. Koch]

On Tue, Dec 15, 2009 at 04:20:56PM -0800, Earl Chew wrote:
> Hans J. Koch wrote:
> > One example: An A/D converter has an on-chip 32k buffer. It causes an
> > interrupt as soon as the buffer is filled up to a certain high-water mark.
> > Such cases would easily fit into the current UIO system. The UIO core could
> > simply DMA the data to one of the mappings. A new flag for that mapping and
> > a few other changes are all it takes. After the DMA transfer is complete, the
> > interrupt is passed on to userspace, which would find the buffer already
> > filled with the desired data. Just a thought, unfortunately I haven't got
> > such hardware to try it.
> 
> Hans,
> 
> Is this case already covered by the pre-existing UIO_MEM_LOGICAL
> option ?
> 
> I'm thinking that since the memory is statically defined, it can be
> described using one of the existing struct uio_mem mem[] slots in
> struct uio_info and marked as UIO_MEM_LOGICAL.
> 
> The userspace program can map that into its process space using the
> existing mmap() interface.
> 
> What am I missing?

Nothing. The UIO core can map all kinds of memory. I thought about a generic
DMA routine that does the transfer if a flag like UIO_DMA_FROM_DEVICE_PRE_USER
is set.

> 
> > When it comes to dynamically allocated DMA buffers, it might well be possible
> > to add a new directory in sysfs besides the "mem" directory, e.g. something
> > like /sys/class/uio/uioN/dma-mem/. This would save us the trouble of creating
> > a new device. Maybe the example above would better fit in here, too. Who knows.
> 
> I looked at the 2.6.32 source at
> 
> http://lxr.linux.no/#linux+v2.6.32/drivers/uio/uio.c
> 
> and didn't see any reference to /sys/class/uio/uioN/mem .  Perhaps
> you're referring to something new.

Just look at the sysfs files you get when creating a UIO device, then look at
uio.c to see how it is done.

> 
> In any case, I think you're describing adding
> 
> /sys/class/uio/uioN/dma-mem
> 
> as a means to control /dev/uioN .  Presumably writing to
> /sys/class/uio/uioN/dma-mem would create additional dynamic
> DMA buffers.

No, dma-mem would be a directory containing some more attributes. Maybe one
called "create" that allocates a new buffer.

> 
> I can't yet see a way to make this request-response. When requesting
> a dynamic buffer I need to indicate the size that I want, and in
> return I need to obtain a handle to the buffer (either its mapping
> number, address, etc). Once I have that, I can query other
> interesting information (eg its bus address).

Writing the size to that supposed "create" attribute could allocate the
buffer and and create more attributes that contain the information you need.

Thanks,
Hans

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Earl Chew]

Hans J. Koch wrote:
> No, dma-mem would be a directory containing some more attributes. Maybe one
> called "create" that allocates a new buffer.
> 
[ .. snip ..]
> Writing the size to that supposed "create" attribute could allocate the
> buffer and and create more attributes that contain the information you need.

Hmm ... I can't see how to make this into a transaction.

Suppose two threads write to /sys/.../create simultaneously (or
very close together) and further suppose that each call succeeds.

It's not clear to me how each can figure out where to find the
outcome of its operation because write() doesn't return anything
other than the number of octets written.

Writing "id, size" might work, but sorting out a good enough id
might be a little clunky. A process id wouldn't be good enough (with
different threads), and a thread id might get recycled.

Any other ideas ?

Earl

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

[Earl Chew]

Hans J. Koch wrote:
> No, dma-mem would be a directory containing some more attributes. Maybe one
> called "create" that allocates a new buffer.
> 
[ .. snip ..]
> Writing the size to that supposed "create" attribute could allocate the
> buffer and and create more attributes that contain the information you need.

Hmm ... I can't see how to make this into a transaction.

Suppose two threads write to /sys/.../create simultaneously (or
very close together) and further suppose that each call succeeds.

It's not clear to me how each can figure out where to find the
outcome of its operation because write() doesn't return anything
other than the number of octets written.

Writing "id, size" might work, but sorting out a good enough id
might be a little clunky. A process id wouldn't be good enough (with
different threads), and a thread id might get recycled.

Any other ideas ?

Earl



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>