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.2On 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---
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
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 )=-
© 2016 - 2025 Red Hat, Inc.