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

[Ian Abbott]

On 25/06/2019 12:47, Dan Carpenter wrote:
On Tue, Jun 25, 2019 at 12:26:59PM +0100, Ian Abbott wrote:
  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));

I think we need a NULL check here:

        if (!buf->virt_addr)
                continue;

                                free_page((unsigned long)buf->virt_addr);
                        }
                }

Hi Dan, I don't think that is strictly required because bm->n_pages gets 
set to the number of successfully allocated pages (not the number of 
required pages) by comedi_buf_map_alloc():

> +          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;

Here!           ^

> +          if (i < n_pages)
> +                  goto err;

> +err:
> +  comedi_buf_map_put(bm);
> +  return NULL;



--
-=( Ian Abbott <abbo...@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:    )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel