[PATCH 0/4] comedi: non-contiguous buffer pages changes

[PATCH 0/4] comedi: non-contiguous buffer pages changes

[Ian Abbott]

This patch series changes the way the Comedi core code accesses the
acquisition buffer contents, so that rather than use an overall, linear
mapping of the whole buffer, it uses existing per-buffer-page pointers.

(@Christoph, I'm mostly interested in your take on calling
`dma_mmap_coherent()` in a loop, as described in the final(-ish)
paragraph, and in patch 4.)

Currently, the buffer is either allocated a page at a time from normal
kernel memory, which is then vmap'ed to a linear address range for
convenience, or it is allocated as a single block from DMA coherent
memory.  (The low-level Comedi driver indicates the type of memory to be
used.)  In either case, an array of `struct comedi_buf_page` is filled
in with the underlying (non-vmap'ed) addresses of each page (and the
corresponding DMA addresses, and the `prealloc_buf` member of `struct
comedi_async` is set to the overall linear address (from `vmap` or
`dma_alloc_coherent` as appropriate).

For buffers in normal kernel memory, this patch series removes the
vmap'ing of the buffer pages.

For buffers in DMA coherent memory, patch 4 splits the allocation into
page-sized allocations.  We used to do that before commit e36472145aa7
("staging: comedi: use dma_mmap_coherent for DMA-able buffer mmap"), but
there was the problem of how to mmap a buffer consisting of
non-contiguous pages of DMA coherent memory.  At the time, I considered
calling `dma_mmap_coherent()` in a loop with manipulated `vm_start` and
`vm_end` values (see <https://lkml.org/lkml/2019/6/17/534>), and that is
what patch 4 does.  It seems to work (although I've only tested in on
x86_64 so far), and I'm not the only person in the wild to discover this
trick (see <https://stackoverflow.com/a/67220955/5264491>), although I
am not aware of any other use of this trick in the kernel source.  The
closest equivalent is the manipulation of the VMA's `vm_pgoff` value
before calling `dma_mmap_coherent()`, for example in the UIO driver.

Patch list:

1) comedi: ni_pcidio: Do not bother filling buffer with 0xaa byte values
2) comedi: access buffer data page-by-page
3) comedi: remove the mapping of the Comedi buffer in vmalloc address space
4) comedi: allocate DMA coherent buffer as individual pages

 drivers/comedi/comedi_buf.c        | 155 ++++++++++++++-----------------------
 drivers/comedi/comedi_fops.c       | 120 ++++++++++++++++++++--------
 drivers/comedi/drivers/ni_pcidio.c |   2 -
 include/linux/comedi/comedidev.h   |  10 +--
 4 files changed, 144 insertions(+), 143 deletions(-)

[PATCH 1/4] comedi: ni_pcidio: Do not bother filling buffer with 0xaa byte values

[Ian Abbott]

On buffer change, the driver informs the mite module about the buffer
change and then it fills the buffer with byte value `0xaa` using
`memset()`.  Do not bother filling the buffer.  None of the other Comedi
drivers do this.

The aim is to get rid of the `prealloc_buf` member of `struct
comedi_async` (which is from a `vmap()` covering the whole buffer in
those Comedi drivers that do not use DMA), and use the per-buffer-page
addresses from the `virt_addr` member of `struct comedi_buf_page` to
access the buffer contents instead.

(If necessary, we could add a `comedi_buf_memset()` function to fill the
buffer with a byte value, but it's not worth it in this case.)

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/comedi/drivers/ni_pcidio.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/comedi/drivers/ni_pcidio.c b/drivers/comedi/drivers/ni_pcidio.c
index 2d58e83420e8..2c7bb9c1ea5b 100644
--- a/drivers/comedi/drivers/ni_pcidio.c
+++ b/drivers/comedi/drivers/ni_pcidio.c
@@ -747,8 +747,6 @@ static int ni_pcidio_change(struct comedi_device *dev,
 	if (ret < 0)
 		return ret;
 
-	memset(s->async->prealloc_buf, 0xaa, s->async->prealloc_bufsz);
-
 	return 0;
 }
 
-- 
2.47.2

[PATCH 2/4] comedi: access buffer data page-by-page

[Ian Abbott]

The aim is to get rid of the `prealloc_buf` member of `struct
comedi_async` and access the buffer contents on a page-by-page basis
using the addresses in the `virt_addr` member of `struct
comedi_buf_page`.  This will allow us to eliminate a `vmap()` that maps
the whole buffer.

Since the buffer pages have non-consecutive `virt_addr` addresses in
virtual memory (except for drivers using DMA), change the loops that
access buffer data to access it page-by-page.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/comedi/comedi_buf.c  | 67 +++++++++++++++++--------------
 drivers/comedi/comedi_fops.c | 77 +++++++++++++++++++++++++++---------
 2 files changed, 96 insertions(+), 48 deletions(-)

diff --git a/drivers/comedi/comedi_buf.c b/drivers/comedi/comedi_buf.c
index 393966c09740..0d956dd40a2b 100644
--- a/drivers/comedi/comedi_buf.c
+++ b/drivers/comedi/comedi_buf.c
@@ -365,6 +365,7 @@ static unsigned int comedi_buf_munge(struct comedi_subdevice *s,
 				     unsigned int num_bytes)
 {
 	struct comedi_async *async = s->async;
+	struct comedi_buf_page *buf_page_list = async->buf_map->page_list;
 	unsigned int count = 0;
 	const unsigned int num_sample_bytes = comedi_bytes_per_sample(s);
 
@@ -376,15 +377,16 @@ static unsigned int comedi_buf_munge(struct comedi_subdevice *s,
 	/* don't munge partial samples */
 	num_bytes -= num_bytes % num_sample_bytes;
 	while (count < num_bytes) {
-		int block_size = num_bytes - count;
-		unsigned int buf_end;
-
-		buf_end = async->prealloc_bufsz - async->munge_ptr;
-		if (block_size > buf_end)
-			block_size = buf_end;
+		/*
+		 * Do not munge beyond page boundary.
+		 * Note: prealloc_bufsz is a multiple of PAGE_SIZE.
+		 */
+		unsigned int page = async->munge_ptr >> PAGE_SHIFT;
+		unsigned int offset = offset_in_page(async->munge_ptr);
+		unsigned int block_size =
+			     min(num_bytes - count, PAGE_SIZE - offset);
 
-		s->munge(s->device, s,
-			 async->prealloc_buf + async->munge_ptr,
+		s->munge(s->device, s, buf_page_list[page].virt_addr + offset,
 			 block_size, async->munge_chan);
 
 		/*
@@ -397,7 +399,8 @@ static unsigned int comedi_buf_munge(struct comedi_subdevice *s,
 		async->munge_chan %= async->cmd.chanlist_len;
 		async->munge_count += block_size;
 		async->munge_ptr += block_size;
-		async->munge_ptr %= async->prealloc_bufsz;
+		if (async->munge_ptr == async->prealloc_bufsz)
+			async->munge_ptr = 0;
 		count += block_size;
 	}
 
@@ -558,46 +561,52 @@ static void comedi_buf_memcpy_to(struct comedi_subdevice *s,
 				 const void *data, unsigned int num_bytes)
 {
 	struct comedi_async *async = s->async;
+	struct comedi_buf_page *buf_page_list = async->buf_map->page_list;
 	unsigned int write_ptr = async->buf_write_ptr;
 
 	while (num_bytes) {
-		unsigned int block_size;
-
-		if (write_ptr + num_bytes > async->prealloc_bufsz)
-			block_size = async->prealloc_bufsz - write_ptr;
-		else
-			block_size = num_bytes;
+		/*
+		 * Do not copy beyond page boundary.
+		 * Note: prealloc_bufsz is a multiple of PAGE_SIZE.
+		 */
+		unsigned int page = write_ptr >> PAGE_SHIFT;
+		unsigned int offset = offset_in_page(write_ptr);
+		unsigned int block_size = min(num_bytes, PAGE_SIZE - offset);
 
-		memcpy(async->prealloc_buf + write_ptr, data, block_size);
+		memcpy(buf_page_list[page].virt_addr + offset,
+		       data, block_size);
 
 		data += block_size;
 		num_bytes -= block_size;
-
-		write_ptr = 0;
+		write_ptr += block_size;
+		if (write_ptr == async->prealloc_bufsz)
+			write_ptr = 0;
 	}
 }
 
 static void comedi_buf_memcpy_from(struct comedi_subdevice *s,
 				   void *dest, unsigned int nbytes)
 {
-	void *src;
 	struct comedi_async *async = s->async;
+	struct comedi_buf_page *buf_page_list = async->buf_map->page_list;
 	unsigned int read_ptr = async->buf_read_ptr;
 
 	while (nbytes) {
-		unsigned int block_size;
-
-		src = async->prealloc_buf + read_ptr;
-
-		if (nbytes >= async->prealloc_bufsz - read_ptr)
-			block_size = async->prealloc_bufsz - read_ptr;
-		else
-			block_size = nbytes;
+		/*
+		 * Do not copy beyond page boundary.
+		 * Note: prealloc_bufsz is a multiple of PAGE_SIZE.
+		 */
+		unsigned int page = read_ptr >> PAGE_SHIFT;
+		unsigned int offset = offset_in_page(read_ptr);
+		unsigned int block_size = min(nbytes, PAGE_SIZE - offset);
 
-		memcpy(dest, src, block_size);
+		memcpy(dest, buf_page_list[page].virt_addr + offset,
+		       block_size);
 		nbytes -= block_size;
 		dest += block_size;
-		read_ptr = 0;
+		read_ptr += block_size;
+		if (read_ptr == async->prealloc_bufsz)
+			read_ptr = 0;
 	}
 }
 
diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index b9df9b19d4bd..37cfef36c1ad 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -2475,6 +2475,62 @@ static __poll_t comedi_poll(struct file *file, poll_table *wait)
 	return mask;
 }
 
+static unsigned int comedi_buf_copy_to_user(struct comedi_subdevice *s,
+	void __user *dest, unsigned int src_offset, unsigned int n)
+{
+	struct comedi_buf_map *bm = s->async->buf_map;
+	struct comedi_buf_page *buf_page_list = bm->page_list;
+	unsigned int page = src_offset >> PAGE_SHIFT;
+	unsigned int offset = offset_in_page(src_offset);
+
+	while (n) {
+		unsigned int copy_amount = min(n, PAGE_SIZE - offset);
+		unsigned int uncopied;
+
+		uncopied = copy_to_user(dest, buf_page_list[page].virt_addr +
+					offset, copy_amount);
+		copy_amount -= uncopied;
+		n -= copy_amount;
+		if (uncopied)
+			break;
+
+		dest += copy_amount;
+		page++;
+		if (page == bm->n_pages)
+			page = 0;	/* buffer wraparound */
+		offset = 0;
+	}
+	return n;
+}
+
+static unsigned int comedi_buf_copy_from_user(struct comedi_subdevice *s,
+	unsigned int dst_offset, const void __user *src, unsigned int n)
+{
+	struct comedi_buf_map *bm = s->async->buf_map;
+	struct comedi_buf_page *buf_page_list = bm->page_list;
+	unsigned int page = dst_offset >> PAGE_SHIFT;
+	unsigned int offset = offset_in_page(dst_offset);
+
+	while (n) {
+		unsigned int copy_amount = min(n, PAGE_SIZE - offset);
+		unsigned int uncopied;
+
+		uncopied = copy_from_user(buf_page_list[page].virt_addr +
+					  offset, src, copy_amount);
+		copy_amount -= uncopied;
+		n -= copy_amount;
+		if (uncopied)
+			break;
+
+		src += copy_amount;
+		page++;
+		if (page == bm->n_pages)
+			page = 0;	/* buffer wraparound */
+		offset = 0;
+	}
+	return n;
+}
+
 static ssize_t comedi_write(struct file *file, const char __user *buf,
 			    size_t nbytes, loff_t *offset)
 {
@@ -2516,7 +2572,6 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	add_wait_queue(&async->wait_head, &wait);
 	while (count == 0 && !retval) {
 		unsigned int runflags;
-		unsigned int wp, n1, n2;
 
 		set_current_state(TASK_INTERRUPTIBLE);
 
@@ -2555,14 +2610,7 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 		}
 
 		set_current_state(TASK_RUNNING);
-		wp = async->buf_write_ptr;
-		n1 = min(n, async->prealloc_bufsz - wp);
-		n2 = n - n1;
-		m = copy_from_user(async->prealloc_buf + wp, buf, n1);
-		if (m)
-			m += n2;
-		else if (n2)
-			m = copy_from_user(async->prealloc_buf, buf + n1, n2);
+		m = comedi_buf_copy_from_user(s, async->buf_write_ptr, buf, n);
 		if (m) {
 			n -= m;
 			retval = -EFAULT;
@@ -2651,8 +2699,6 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 
 	add_wait_queue(&async->wait_head, &wait);
 	while (count == 0 && !retval) {
-		unsigned int rp, n1, n2;
-
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		m = comedi_buf_read_n_available(s);
@@ -2689,14 +2735,7 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		}
 
 		set_current_state(TASK_RUNNING);
-		rp = async->buf_read_ptr;
-		n1 = min(n, async->prealloc_bufsz - rp);
-		n2 = n - n1;
-		m = copy_to_user(buf, async->prealloc_buf + rp, n1);
-		if (m)
-			m += n2;
-		else if (n2)
-			m = copy_to_user(buf + n1, async->prealloc_buf, n2);
+		m = comedi_buf_copy_to_user(s, buf, async->buf_read_ptr, n);
 		if (m) {
 			n -= m;
 			retval = -EFAULT;
-- 
2.47.2

[PATCH 3/4] comedi: remove the mapping of the Comedi buffer in vmalloc address space

[Ian Abbott]

Now that all the code that accesses the Comedi buffer data does so
page-by-page, using the `virt_addr` member of `struct comedi_buf_page`
to point to the data of each page, do not linearly map the buffer into
vmalloc address space (pointed to by the `prealloc_buf` member of
`struct comedi_async`).  That was only done for convenience, but was not
done for those drivers that need a DMA coherent buffer, which is
allocated in a single chunk.  Remove the `prealloc_buf` member as it is
no longer used.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/comedi/comedi_buf.c      | 45 +++-----------------------------
 include/linux/comedi/comedidev.h | 10 ++-----
 2 files changed, 6 insertions(+), 49 deletions(-)

diff --git a/drivers/comedi/comedi_buf.c b/drivers/comedi/comedi_buf.c
index 0d956dd40a2b..5807007bb3dd 100644
--- a/drivers/comedi/comedi_buf.c
+++ b/drivers/comedi/comedi_buf.c
@@ -56,13 +56,7 @@ static void __comedi_buf_free(struct comedi_device *dev,
 	struct comedi_buf_map *bm;
 	unsigned long flags;
 
-	if (async->prealloc_buf) {
-		if (s->async_dma_dir == DMA_NONE)
-			vunmap(async->prealloc_buf);
-		async->prealloc_buf = NULL;
-		async->prealloc_bufsz = 0;
-	}
-
+	async->prealloc_bufsz = 0;
 	spin_lock_irqsave(&s->spin_lock, flags);
 	bm = async->buf_map;
 	async->buf_map = NULL;
@@ -141,11 +135,8 @@ static void __comedi_buf_alloc(struct comedi_device *dev,
 			       unsigned int n_pages)
 {
 	struct comedi_async *async = s->async;
-	struct page **pages = NULL;
 	struct comedi_buf_map *bm;
-	struct comedi_buf_page *buf;
 	unsigned long flags;
-	unsigned int i;
 
 	if (!IS_ENABLED(CONFIG_HAS_DMA) && s->async_dma_dir != DMA_NONE) {
 		dev_err(dev->class_dev,
@@ -160,30 +151,7 @@ static void __comedi_buf_alloc(struct comedi_device *dev,
 	spin_lock_irqsave(&s->spin_lock, flags);
 	async->buf_map = bm;
 	spin_unlock_irqrestore(&s->spin_lock, flags);
-
-	if (bm->dma_dir != DMA_NONE) {
-		/*
-		 * DMA buffer was allocated as a single block.
-		 * Address is in page_list[0].
-		 */
-		buf = &bm->page_list[0];
-		async->prealloc_buf = buf->virt_addr;
-	} else {
-		pages = vmalloc(sizeof(struct page *) * n_pages);
-		if (!pages)
-			return;
-
-		for (i = 0; i < n_pages; i++) {
-			buf = &bm->page_list[i];
-			pages[i] = virt_to_page(buf->virt_addr);
-		}
-
-		/* vmap the pages to prealloc_buf */
-		async->prealloc_buf = vmap(pages, n_pages, VM_MAP,
-					   COMEDI_PAGE_PROTECTION);
-
-		vfree(pages);
-	}
+	async->prealloc_bufsz = n_pages << PAGE_SHIFT;
 }
 
 void comedi_buf_map_get(struct comedi_buf_map *bm)
@@ -264,7 +232,7 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 	new_size = (new_size + PAGE_SIZE - 1) & PAGE_MASK;
 
 	/* if no change is required, do nothing */
-	if (async->prealloc_buf && async->prealloc_bufsz == new_size)
+	if (async->prealloc_bufsz == new_size)
 		return 0;
 
 	/* deallocate old buffer */
@@ -275,14 +243,9 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 		unsigned int n_pages = new_size >> PAGE_SHIFT;
 
 		__comedi_buf_alloc(dev, s, n_pages);
-
-		if (!async->prealloc_buf) {
-			/* allocation failed */
-			__comedi_buf_free(dev, s);
+		if (!async->prealloc_bufsz)
 			return -ENOMEM;
-		}
 	}
-	async->prealloc_bufsz = new_size;
 
 	return 0;
 }
diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h
index c08416a7364b..4cb0400ad616 100644
--- a/include/linux/comedi/comedidev.h
+++ b/include/linux/comedi/comedidev.h
@@ -234,16 +234,12 @@ struct comedi_buf_page {
  *
  * A COMEDI data buffer is allocated as individual pages, either in
  * conventional memory or DMA coherent memory, depending on the attached,
- * low-level hardware device.  (The buffer pages also get mapped into the
- * kernel's contiguous virtual address space pointed to by the 'prealloc_buf'
- * member of &struct comedi_async.)
+ * low-level hardware device.
  *
  * The buffer is normally freed when the COMEDI device is detached from the
  * low-level driver (which may happen due to device removal), but if it happens
  * to be mmapped at the time, the pages cannot be freed until the buffer has
- * been munmapped.  That is what the reference counter is for.  (The virtual
- * address space pointed by 'prealloc_buf' is freed when the COMEDI device is
- * detached.)
+ * been munmapped.  That is what the reference counter is for.
  */
 struct comedi_buf_map {
 	struct device *dma_hw_dev;
@@ -255,7 +251,6 @@ struct comedi_buf_map {
 
 /**
  * struct comedi_async - Control data for asynchronous COMEDI commands
- * @prealloc_buf: Kernel virtual address of allocated acquisition buffer.
  * @prealloc_bufsz: Buffer size (in bytes).
  * @buf_map: Map of buffer pages.
  * @max_bufsize: Maximum allowed buffer size (in bytes).
@@ -344,7 +339,6 @@ struct comedi_buf_map {
  * less than or equal to UINT_MAX).
  */
 struct comedi_async {
-	void *prealloc_buf;
 	unsigned int prealloc_bufsz;
 	struct comedi_buf_map *buf_map;
 	unsigned int max_bufsize;
-- 
2.47.2

[PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

[Ian Abbott]

Depending on the driver, the acquisition buffer is allocated either from
normal memory, or from DMA coherent memory.  For normal memory, the
buffer is allocated as individual pages, but for DMA coherent memory, it
is allocated as a single block.  Prior to commit e36472145aa7 ("staging:
comedi: use dma_mmap_coherent for DMA-able buffer mmap"), the buffer was
allocated as individual pages for DMA coherent memory too, but that was
changed to allocate it as a single block to allow `dma_mmap_coherent()`
to be used to mmap it, because that requires the pages being mmap'ed to
be contiguous.

This patch allocates the buffer from DMA coherent memory a page at a
time again, and works around the limitation of `dma_mmap_coherent()` by
calling it in a loop for each page, with temporarily modified `vm_start`
and `vm_end` values in the VMA.  (The `vm_pgoff` value is 0.)

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/comedi/comedi_buf.c  | 43 ++++++++++++------------------------
 drivers/comedi/comedi_fops.c | 43 +++++++++++++++++++++++-------------
 2 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/drivers/comedi/comedi_buf.c b/drivers/comedi/comedi_buf.c
index 5807007bb3dd..002c0e76baff 100644
--- a/drivers/comedi/comedi_buf.c
+++ b/drivers/comedi/comedi_buf.c
@@ -27,14 +27,12 @@ static void comedi_buf_map_kref_release(struct kref *kref)
 
 	if (bm->page_list) {
 		if (bm->dma_dir != DMA_NONE) {
-			/*
-			 * DMA buffer was allocated as a single block.
-			 * Address is in page_list[0].
-			 */
-			buf = &bm->page_list[0];
-			dma_free_coherent(bm->dma_hw_dev,
-					  PAGE_SIZE * bm->n_pages,
-					  buf->virt_addr, buf->dma_addr);
+			for (i = 0; i < bm->n_pages; i++) {
+				buf = &bm->page_list[i];
+				dma_free_coherent(bm->dma_hw_dev, PAGE_SIZE,
+						  buf->virt_addr,
+						  buf->dma_addr);
+			}
 		} else {
 			for (i = 0; i < bm->n_pages; i++) {
 				buf = &bm->page_list[i];
@@ -88,26 +86,14 @@ comedi_buf_map_alloc(struct comedi_device *dev, enum dma_data_direction dma_dir,
 		goto err;
 
 	if (bm->dma_dir != DMA_NONE) {
-		void *virt_addr;
-		dma_addr_t dma_addr;
-
-		/*
-		 * Currently, the DMA buffer needs to be allocated as a
-		 * single block so that it can be mmap()'ed.
-		 */
-		virt_addr = dma_alloc_coherent(bm->dma_hw_dev,
-					       PAGE_SIZE * n_pages, &dma_addr,
-					       GFP_KERNEL);
-		if (!virt_addr)
-			goto err;
-
 		for (i = 0; i < n_pages; i++) {
 			buf = &bm->page_list[i];
-			buf->virt_addr = virt_addr + (i << PAGE_SHIFT);
-			buf->dma_addr = dma_addr + (i << PAGE_SHIFT);
+			buf->virt_addr =
+			    dma_alloc_coherent(bm->dma_hw_dev, PAGE_SIZE,
+					       &buf->dma_addr, GFP_KERNEL);
+			if (!buf->virt_addr)
+				break;
 		}
-
-		bm->n_pages = i;
 	} else {
 		for (i = 0; i < n_pages; i++) {
 			buf = &bm->page_list[i];
@@ -117,11 +103,10 @@ comedi_buf_map_alloc(struct comedi_device *dev, enum dma_data_direction dma_dir,
 
 			SetPageReserved(virt_to_page(buf->virt_addr));
 		}
-
-		bm->n_pages = i;
-		if (i < n_pages)
-			goto err;
 	}
+	bm->n_pages = i;
+	if (i < n_pages)
+		goto err;
 
 	return bm;
 
diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 37cfef36c1ad..3383a7ce27ff 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -2387,13 +2387,27 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
 		goto done;
 	}
 	if (bm->dma_dir != DMA_NONE) {
+		unsigned long vm_start = vma->vm_start;
+		unsigned long vm_end = vma->vm_end;
+
 		/*
-		 * DMA buffer was allocated as a single block.
-		 * Address is in page_list[0].
+		 * Buffer pages are not contiguous, so temporarily modify VMA
+		 * start and end addresses for each buffer page.
 		 */
-		buf = &bm->page_list[0];
-		retval = dma_mmap_coherent(bm->dma_hw_dev, vma, buf->virt_addr,
-					   buf->dma_addr, n_pages * PAGE_SIZE);
+		for (i = 0; i < n_pages; ++i) {
+			buf = &bm->page_list[i];
+			vma->vm_start = start;
+			vma->vm_end = start + PAGE_SIZE;
+			retval = dma_mmap_coherent(bm->dma_hw_dev, vma,
+						   buf->virt_addr,
+						   buf->dma_addr, PAGE_SIZE);
+			if (retval)
+				break;
+
+			start += PAGE_SIZE;
+		}
+		vma->vm_start = vm_start;
+		vma->vm_end = vm_end;
 	} else {
 		for (i = 0; i < n_pages; ++i) {
 			unsigned long pfn;
@@ -2407,19 +2421,18 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
 
 			start += PAGE_SIZE;
 		}
+	}
 
 #ifdef CONFIG_MMU
-		/*
-		 * Leaving behind a partial mapping of a buffer we're about to
-		 * drop is unsafe, see remap_pfn_range_notrack().
-		 * We need to zap the range here ourselves instead of relying
-		 * on the automatic zapping in remap_pfn_range() because we call
-		 * remap_pfn_range() in a loop.
-		 */
-		if (retval)
-			zap_vma_ptes(vma, vma->vm_start, size);
+	/*
+	 * Leaving behind a partial mapping of a buffer we're about to drop is
+	 * unsafe, see remap_pfn_range_notrack().  We need to zap the range
+	 * here ourselves instead of relying on the automatic zapping in
+	 * remap_pfn_range() because we call remap_pfn_range() in a loop.
+	 */
+	if (retval)
+		zap_vma_ptes(vma, vma->vm_start, size);
 #endif
-	}
 
 	if (retval == 0) {
 		vma->vm_ops = &comedi_vm_ops;
-- 
2.47.2

Re: [PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

[Christoph Hellwig]

On Tue, Apr 15, 2025 at 12:35:59PM +0100, Ian Abbott wrote:
> +			vma->vm_start = start;
> +			vma->vm_end = start + PAGE_SIZE;
> +			retval = dma_mmap_coherent(bm->dma_hw_dev, vma,
> +						   buf->virt_addr,
> +						   buf->dma_addr, PAGE_SIZE);

I'm not  fan of the vm_start/vm_end manipulation, but I've seen it in
other places.  In a perfect world we'd have a dma_mmap_coherent_offset
or similar helper that encapsulates it, and then maybe later replace
that hack with passing on the offset.

> +			if (retval)
> +				break;
> +
> +			start += PAGE_SIZE;
> +		}
> +		vma->vm_start = vm_start;
> +		vma->vm_end = vm_end;
>  	} else {
>  		for (i = 0; i < n_pages; ++i) {
>  			unsigned long pfn;
> @@ -2407,19 +2421,18 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
>  
>  			start += PAGE_SIZE;
>  		}
> +	}
>  
>  #ifdef CONFIG_MMU
> -		/*
> -		 * Leaving behind a partial mapping of a buffer we're about to
> -		 * drop is unsafe, see remap_pfn_range_notrack().
> -		 * We need to zap the range here ourselves instead of relying
> -		 * on the automatic zapping in remap_pfn_range() because we call
> -		 * remap_pfn_range() in a loop.
> -		 */
> -		if (retval)
> -			zap_vma_ptes(vma, vma->vm_start, size);
> +	/*
> +	 * Leaving behind a partial mapping of a buffer we're about to drop is
> +	 * unsafe, see remap_pfn_range_notrack().  We need to zap the range
> +	 * here ourselves instead of relying on the automatic zapping in
> +	 * remap_pfn_range() because we call remap_pfn_range() in a loop.
> +	 */
> +	if (retval)
> +		zap_vma_ptes(vma, vma->vm_start, size);
>  #endif
> -	}
>  
>  	if (retval == 0) {
>  		vma->vm_ops = &comedi_vm_ops;
> -- 
> 2.47.2
---end quoted text---

Re: [PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

[Marek Szyprowski]

On 28.04.2025 14:56, Christoph Hellwig wrote:
> On Tue, Apr 15, 2025 at 12:35:59PM +0100, Ian Abbott wrote:
>> +			vma->vm_start = start;
>> +			vma->vm_end = start + PAGE_SIZE;
>> +			retval = dma_mmap_coherent(bm->dma_hw_dev, vma,
>> +						   buf->virt_addr,
>> +						   buf->dma_addr, PAGE_SIZE);
> I'm not  fan of the vm_start/vm_end manipulation, but I've seen it in
> other places.  In a perfect world we'd have a dma_mmap_coherent_offset
> or similar helper that encapsulates it, and then maybe later replace
> that hack with passing on the offset.

Indeed the dma_mmap_*() makes too many assumptions about the vma. The 
case You mentioned is probably in drivers/infiniband/hw/hfi1/file_ops.c 
but I also see that the vma->vm_pgoff is being adjusted before most 
dma_mmap_*() calls, which proves that the current API is somehow 
limited. It would be great to fix this too while touching the 
dma_mmap_attrs() API.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

[Ian Abbott]

On 28/04/2025 16:33, Marek Szyprowski wrote:
> On 28.04.2025 14:56, Christoph Hellwig wrote:
>> On Tue, Apr 15, 2025 at 12:35:59PM +0100, Ian Abbott wrote:
>>> +			vma->vm_start = start;
>>> +			vma->vm_end = start + PAGE_SIZE;
>>> +			retval = dma_mmap_coherent(bm->dma_hw_dev, vma,
>>> +						   buf->virt_addr,
>>> +						   buf->dma_addr, PAGE_SIZE);
>> I'm not  fan of the vm_start/vm_end manipulation, but I've seen it in
>> other places.  In a perfect world we'd have a dma_mmap_coherent_offset
>> or similar helper that encapsulates it, and then maybe later replace
>> that hack with passing on the offset.
> 
> Indeed the dma_mmap_*() makes too many assumptions about the vma. The
> case You mentioned is probably in drivers/infiniband/hw/hfi1/file_ops.c
> but I also see that the vma->vm_pgoff is being adjusted before most
> dma_mmap_*() calls, which proves that the current API is somehow
> limited. It would be great to fix this too while touching the
> dma_mmap_attrs() API.

Drivers would probably have to continue manipulating vma->vm_pgoff 
anyway if they use its value in a special way, like 
drivers/infiniband/hw/hfil/file_ops.c or drivers/uio/uio.c.  The 
dma_mmap_*() calls already use vma->vm_pgoff as an offset into the VMA 
area, so I think all the new API would need is a parameter to restrict 
the number of pages being mapped, or something similar.

The new API doesn't necessarily have to be reflected all the way down to 
the dma_mmap_direct(), iommu_dma_mmap(), and ops->mmap() functions, as 
the new dma_mmap_*() function could modify vma->vm_end temporarily in 
order to restrict the number of pages being mapped by the lower-level 
functions.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-