[PATCH] staging: comedi: use dma_mmap_coherent for DMA-able buffer mmap

Christoph Hellwig hch at lst.de
Tue Jun 25 12:06:37 UTC 2019


>From the DMA point of view this looks good:

Reviewed-by: Christoph Hellwig <hch at lst.de>

I still think that doing that SetPageReserved + remap_pfn_range
dance for the normal memory allocations is a bad idea.  Just use
vm_insert_page on the page, in which case it doesn't need to be
marked as Reserved.


On Tue, Jun 25, 2019 at 12:26:59PM +0100, Ian Abbott wrote:
> Comedi's acquisition buffer allocation code can allocate the buffer from
> normal kernel memory or from DMA coherent memory depending on the
> `dma_async_dir` value in the comedi subdevice.  (A value of `DMA_NONE`
> causes the buffer to be allocated from normal kernel memory.  Other
> values cause the buffer to be allocated from DMA coherent memory.)   The
> buffer currently consists of a bunch of page-sized blocks that are
> vmap'ed into a contiguous range of virtual addresses. The pages are also
> mmap'able into user address space.  For DMA'able buffers, these
> page-sized blocks are allocated by `dma_alloc_coherent()`.
> 
> For DMA-able buffers, the DMA API is currently abused in various ways,
> the most serious abuse being the calling of `virt_to_page()` on the
> blocks allocated by `dma_alloc_coherent()` and passing those pages to
> `vmap()` (for mapping to the kernels vmalloc address space) and via
> `page_to_pfn()` to `remap_pfn_range()` (for mmap'ing to user space).  it
> also uses the `__GFP_COMP` flag when allocating the blocks, and marks
> the allocated pages as reserved (which is unnecessary for DMA coherent
> allocations).
> 
> The code can be changed to get rid of the vmap'ed address altogether if
> necessary, since there are only a few places in the comedi code that use
> the vmap'ed address directly and we also keep a list of the kernel
> addresses for the individual pages prior to the vmap operation. This
> would add some run-time overhead to buffer accesses.  The real killer is
> the mmap operation.
> 
> For mmap, the address range specified in the VMA needs to be mmap'ed to
> the individually allocated page-sized blocks.  That is not a problem
> when the pages are allocated from normal kernel memory as the individual
> pages can be remapped by `remap_pfn_range()`, but it is a problem when
> the page-sized blocks are allocated by `dma_alloc_coherent()` because
> the DMA API currently has no support for splitting a VMA across multiple
> blocks of DMA coherent memory (or rather, no support for mapping part of
> a VMA range to a single block of DMA coherent memory).
> 
> In order to comply with the DMA API and allow the buffer to be mmap'ed,
> the buffer needs to be allocated as a single block by a single call to
> `dma_alloc_coherent()`, freed by a single call to `dma_free_coherent()`,
> and mmap'ed to user space by a single call to `dma_mmap_coherent()`.
> This patch changes the buffer allocation, freeing, and mmap'ing code to
> do that, with the unfortunate consequence that buffer allocation is more
> likely to fail.  It also no longer uses the `__GFP_COMP` flag when
> allocating DMA coherent memory, no longer marks the
> allocated pages of DMA coherent memory as reserved, and no longer vmap's
> the DMA coherent memory pages (since they are contiguous anyway).
> 
> Cc: Christoph Hellwig <hch at lst.de>
> Signed-off-by: Ian Abbott <abbotti at mev.co.uk>
> ---
>  drivers/staging/comedi/comedi_buf.c  | 150 ++++++++++++++++++---------
>  drivers/staging/comedi/comedi_fops.c |  39 ++++---
>  2 files changed, 125 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/staging/comedi/comedi_buf.c b/drivers/staging/comedi/comedi_buf.c
> index d2c8cc72a99d..3ef3ddabf139 100644
> --- a/drivers/staging/comedi/comedi_buf.c
> +++ b/drivers/staging/comedi/comedi_buf.c
> @@ -27,18 +27,19 @@ static void comedi_buf_map_kref_release(struct kref *kref)
>  	unsigned int i;
>  
>  	if (bm->page_list) {
> -		for (i = 0; i < bm->n_pages; i++) {
> -			buf = &bm->page_list[i];
> -			clear_bit(PG_reserved,
> -				  &(virt_to_page(buf->virt_addr)->flags));
> -			if (bm->dma_dir != DMA_NONE) {
> -#ifdef CONFIG_HAS_DMA
> -				dma_free_coherent(bm->dma_hw_dev,
> -						  PAGE_SIZE,
> -						  buf->virt_addr,
> -						  buf->dma_addr);
> -#endif
> -			} else {
> +		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);
> +		} else {
> +			for (i = 0; i < bm->n_pages; i++) {
> +				buf = &bm->page_list[i];
> +				ClearPageReserved(virt_to_page(buf->virt_addr));
>  				free_page((unsigned long)buf->virt_addr);
>  			}
>  		}
> @@ -57,7 +58,8 @@ static void __comedi_buf_free(struct comedi_device *dev,
>  	unsigned long flags;
>  
>  	if (async->prealloc_buf) {
> -		vunmap(async->prealloc_buf);
> +		if (s->async_dma_dir == DMA_NONE)
> +			vunmap(async->prealloc_buf);
>  		async->prealloc_buf = NULL;
>  		async->prealloc_bufsz = 0;
>  	}
> @@ -69,6 +71,72 @@ static void __comedi_buf_free(struct comedi_device *dev,
>  	comedi_buf_map_put(bm);
>  }
>  
> +static struct comedi_buf_map *
> +comedi_buf_map_alloc(struct comedi_device *dev, enum dma_data_direction dma_dir,
> +		     unsigned int n_pages)
> +{
> +	struct comedi_buf_map *bm;
> +	struct comedi_buf_page *buf;
> +	unsigned int i;
> +
> +	bm = kzalloc(sizeof(*bm), GFP_KERNEL);
> +	if (!bm)
> +		return NULL;
> +
> +	kref_init(&bm->refcount);
> +	bm->dma_dir = dma_dir;
> +	if (bm->dma_dir != DMA_NONE) {
> +		/* Need ref to hardware device to free buffer later. */
> +		bm->dma_hw_dev = get_device(dev->hw_dev);
> +	}
> +
> +	bm->page_list = vzalloc(sizeof(*buf) * n_pages);
> +	if (!bm->page_list)
> +		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);
> +		}
> +
> +		bm->n_pages = i;
> +	} else {
> +		for (i = 0; i < n_pages; i++) {
> +			buf = &bm->page_list[i];
> +			buf->virt_addr = (void *)get_zeroed_page(GFP_KERNEL);
> +			if (!buf->virt_addr)
> +				break;
> +
> +			SetPageReserved(virt_to_page(buf->virt_addr));
> +		}
> +
> +		bm->n_pages = i;
> +		if (i < n_pages)
> +			goto err;
> +	}
> +
> +	return bm;
> +
> +err:
> +	comedi_buf_map_put(bm);
> +	return NULL;
> +}
> +
>  static void __comedi_buf_alloc(struct comedi_device *dev,
>  			       struct comedi_subdevice *s,
>  			       unsigned int n_pages)
> @@ -86,57 +154,37 @@ static void __comedi_buf_alloc(struct comedi_device *dev,
>  		return;
>  	}
>  
> -	bm = kzalloc(sizeof(*async->buf_map), GFP_KERNEL);
> +	bm = comedi_buf_map_alloc(dev, s->async_dma_dir, n_pages);
>  	if (!bm)
>  		return;
>  
> -	kref_init(&bm->refcount);
>  	spin_lock_irqsave(&s->spin_lock, flags);
>  	async->buf_map = bm;
>  	spin_unlock_irqrestore(&s->spin_lock, flags);
> -	bm->dma_dir = s->async_dma_dir;
> -	if (bm->dma_dir != DMA_NONE)
> -		/* Need ref to hardware device to free buffer later. */
> -		bm->dma_hw_dev = get_device(dev->hw_dev);
>  
> -	bm->page_list = vzalloc(sizeof(*buf) * n_pages);
> -	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];
> +		async->prealloc_buf = buf->virt_addr;
> +	} else {
>  		pages = vmalloc(sizeof(struct page *) * n_pages);
> +		if (!pages)
> +			return;
>  
> -	if (!pages)
> -		return;
> -
> -	for (i = 0; i < n_pages; i++) {
> -		buf = &bm->page_list[i];
> -		if (bm->dma_dir != DMA_NONE)
> -#ifdef CONFIG_HAS_DMA
> -			buf->virt_addr = dma_alloc_coherent(bm->dma_hw_dev,
> -							    PAGE_SIZE,
> -							    &buf->dma_addr,
> -							    GFP_KERNEL |
> -							    __GFP_COMP);
> -#else
> -			break;
> -#endif
> -		else
> -			buf->virt_addr = (void *)get_zeroed_page(GFP_KERNEL);
> -		if (!buf->virt_addr)
> -			break;
> -
> -		set_bit(PG_reserved, &(virt_to_page(buf->virt_addr)->flags));
> -
> -		pages[i] = virt_to_page(buf->virt_addr);
> -	}
> -	spin_lock_irqsave(&s->spin_lock, flags);
> -	bm->n_pages = i;
> -	spin_unlock_irqrestore(&s->spin_lock, flags);
> +		for (i = 0; i < n_pages; i++) {
> +			buf = &bm->page_list[i];
> +			pages[i] = virt_to_page(buf->virt_addr);
> +		}
>  
> -	/* vmap the prealloc_buf if all the pages were allocated */
> -	if (i == n_pages)
> +		/* vmap the pages to prealloc_buf */
>  		async->prealloc_buf = vmap(pages, n_pages, VM_MAP,
>  					   COMEDI_PAGE_PROTECTION);
>  
> -	vfree(pages);
> +		vfree(pages);
> +	}
>  }
>  
>  void comedi_buf_map_get(struct comedi_buf_map *bm)
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index f6d1287c7b83..08d1bbbebf2d 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2301,11 +2301,12 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
>  	struct comedi_subdevice *s;
>  	struct comedi_async *async;
>  	struct comedi_buf_map *bm = NULL;
> +	struct comedi_buf_page *buf;
>  	unsigned long start = vma->vm_start;
>  	unsigned long size;
>  	int n_pages;
>  	int i;
> -	int retval;
> +	int retval = 0;
>  
>  	/*
>  	 * 'trylock' avoids circular dependency with current->mm->mmap_sem
> @@ -2361,24 +2362,36 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
>  		retval = -EINVAL;
>  		goto done;
>  	}
> -	for (i = 0; i < n_pages; ++i) {
> -		struct comedi_buf_page *buf = &bm->page_list[i];
> +	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];
> +		retval = dma_mmap_coherent(bm->dma_hw_dev, vma, buf->virt_addr,
> +					   buf->dma_addr, n_pages * PAGE_SIZE);
> +	} else {
> +		for (i = 0; i < n_pages; ++i) {
> +			unsigned long pfn;
> +
> +			buf = &bm->page_list[i];
> +			pfn = page_to_pfn(virt_to_page(buf->virt_addr));
> +			retval = remap_pfn_range(vma, start, pfn, PAGE_SIZE,
> +						 PAGE_SHARED);
> +			if (retval)
> +				break;
>  
> -		if (remap_pfn_range(vma, start,
> -				    page_to_pfn(virt_to_page(buf->virt_addr)),
> -				    PAGE_SIZE, PAGE_SHARED)) {
> -			retval = -EAGAIN;
> -			goto done;
> +			start += PAGE_SIZE;
>  		}
> -		start += PAGE_SIZE;
>  	}
>  
> -	vma->vm_ops = &comedi_vm_ops;
> -	vma->vm_private_data = bm;
> +	if (retval == 0) {
> +		vma->vm_ops = &comedi_vm_ops;
> +		vma->vm_private_data = bm;
>  
> -	vma->vm_ops->open(vma);
> +		vma->vm_ops->open(vma);
> +	}
>  
> -	retval = 0;
>  done:
>  	up_read(&dev->attach_lock);
>  	comedi_buf_map_put(bm);	/* put reference to buf map - okay if NULL */
> -- 
> 2.20.1
---end quoted text---


More information about the devel mailing list