Re: [RFC PATCH] arm: dma-mapping: fallback allocation for cma failure

[Michal Nazarewicz <mina86@mina86.com>]

On Wed, May 21 2014, Gioh Kim <gioh.kim@lge.com> wrote:
> Date: Tue, 20 May 2014 14:16:20 +0900
> Subject: [PATCH] arm: dma-mapping: add checking cma area initialized
>
> If CMA is turned on and CMA size is set to zero, kernel should
> behave as if CMA was not enabled at compile time.
> Every dma allocation should check existence of cma area
> before requesting memory.
>
> Signed-off-by: Gioh Kim <gioh.kim@lge.com>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Some minor comments.  Also, I'd love for someone more experienced with
ARM to take a look at this as well.

> ---
>   arch/arm/mm/dma-mapping.c |   12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 18e98df..61f7b93 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -379,7 +379,7 @@ static int __init atomic_pool_init(void)
>          unsigned long *bitmap;
>          struct page *page;
>          struct page **pages;
> -       void *ptr;
> +       void *ptr = NULL;

This is unnecessary any more.

>          int bitmap_size = BITS_TO_LONGS(nr_pages) * sizeof(long);
>
>          bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> @@ -390,12 +390,13 @@ static int __init atomic_pool_init(void)
>          if (!pages)
>                  goto no_pages;
>
> -       if (IS_ENABLED(CONFIG_DMA_CMA))
> +       if (IS_ENABLED(CONFIG_DMA_CMA) && dma_contiguous_default_area)

+	if (dev_get_cma_area(NULL))

dev_get_cma_area returns NULL if !IS_ENABLED(CONFIG_DMA_CMA) so there's
no need to check it explicitly.  And with NULL argument,
deg_get_cma_area returns the default area.

>                  ptr = __alloc_from_contiguous(NULL, pool->size, prot, &page,
>                                                atomic_pool_init);
>          else
>                  ptr = __alloc_remap_buffer(NULL, pool->size, gfp, prot, &page,
>                                             atomic_pool_init);
> +
>          if (ptr) {
>                  int i;
>
> @@ -669,6 +670,7 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>          u64 mask = get_coherent_dma_mask(dev);
>          struct page *page = NULL;
>          void *addr;
> +       struct cma *cma = dev_get_cma_area(dev);
>
>   #ifdef CONFIG_DMA_API_DEBUG
>          u64 limit = (mask + 1) & ~mask;
> @@ -701,7 +703,7 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>                  addr = __alloc_simple_buffer(dev, size, gfp, &page);
>          else if (!(gfp & __GFP_WAIT))
>                  addr = __alloc_from_pool(size, &page);
> -       else if (!IS_ENABLED(CONFIG_DMA_CMA))
> +       else if (!IS_ENABLED(CONFIG_DMA_CMA) || !cma)

Like above, just do:

+	else if (!dev_get_cma_area(dev))

This will also allow to drop the “cma” variable above.

>                  addr = __alloc_remap_buffer(dev, size, gfp, prot, &page, caller);
>          else
>                  addr = __alloc_from_contiguous(dev, size, prot, &page, caller);
> @@ -780,6 +782,7 @@ static void __arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
>                             bool is_coherent)
>   {
>          struct page *page = pfn_to_page(dma_to_pfn(dev, handle));
> +       struct cma *cma = dev_get_cma_area(dev);
>
>          if (dma_release_from_coherent(dev, get_order(size), cpu_addr))
>                  return;
> @@ -790,7 +793,7 @@ static void __arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
>                  __dma_free_buffer(page, size);
>          } else if (__free_from_pool(cpu_addr, size)) {
>                  return;
> -       } else if (!IS_ENABLED(CONFIG_DMA_CMA)) {
> +       } else if (!IS_ENABLED(CONFIG_DMA_CMA) || !cma) {

Ditto.

>                  __dma_free_remap(cpu_addr, size);
>                  __dma_free_buffer(page, size);
>          } else {
> @@ -798,6 +801,7 @@ static void __arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
>                   * Non-atomic allocations cannot be freed with IRQs disabled
>                   */
>                  WARN_ON(irqs_disabled());
> +

Unrelated change.

>                  __free_from_contiguous(dev, page, cpu_addr, size);
>          }
>   }
> --
> 1.7.9.5

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--