On Fri, Aug 24, 2012 at 11:29:02AM +0300, Hiroshi Doyu wrote:

struct page **pages is necessary to align with non atomic path in __iommu_get_pages(). atomic_pool() has the intialized **pages instead of just *page.

Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com

arch/arm/mm/dma-mapping.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 601da7a..b14ee64 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -296,7 +296,7 @@ struct dma_pool { unsigned long *bitmap; unsigned long nr_pages; void *vaddr;

• struct page *page;

• struct page **pages;

}; static struct dma_pool atomic_pool = { @@ -335,12 +335,16 @@ static int __init atomic_pool_init(void) unsigned long nr_pages = pool->size >> PAGE_SHIFT; unsigned long *bitmap; struct page *page;

• struct page **pages; void *ptr; int bitmap_size = BITS_TO_LONGS(nr_pages) * sizeof(long);
• size_t size = nr_pages * sizeof(struct page *);

• bitmap = kzalloc(bitmap_size, GFP_KERNEL);

• size += bitmap_size;
• bitmap = kzalloc(size, GFP_KERNEL); if (!bitmap) goto no_bitmap;
• pages = (void *)bitmap + bitmap_size;

So you stuck a bitmap field in front of the array then? Why not just define a structure where this is clearly defined instead of doing the casting.

if (IS_ENABLED(CONFIG_CMA)) ptr = __alloc_from_contiguous(NULL, pool->size, prot, &page); @@ -348,9 +352,14 @@ static int __init atomic_pool_init(void) ptr = __alloc_remap_buffer(NULL, pool->size, GFP_KERNEL, prot, &page, NULL); if (ptr) {

• int i;
  

• for (i = 0; i < nr_pages; i++)
  

• 	pages[i] = page + i;
  

• spin_lock_init(&pool->lock); pool->vaddr = ptr;

• pool->page = page;
  

• pool->pages = pages;
  

  pool->bitmap = bitmap; pool->nr_pages = nr_pages; pr_info("DMA: preallocated %u KiB pool for atomic coherent allocations\n",

@@ -481,7 +490,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page) if (pageno < pool->nr_pages) { bitmap_set(pool->bitmap, pageno, count); ptr = pool->vaddr + PAGE_SIZE * pageno;

• *ret_page = pool->page + pageno;
  

• *ret_page = pool->pages[pageno];
  

  } else { pr_err_once("ERROR: %u KiB atomic DMA coherent pool is too small!\n" "Please increase it with coherent_pool= kernel parameter!\n",

-- 1.7.5.4

Hello,

On Friday, August 24, 2012 1:52 PM Hiroshi Doyu wrote:

Konrad Rzeszutek Wilk konrad.wilk@oracle.com wrote @ Fri, 24 Aug 2012 13:13:23 +0200:

...

On Fri, Aug 24, 2012 at 11:29:02AM +0300, Hiroshi Doyu wrote:

...

struct page **pages is necessary to align with non atomic path in __iommu_get_pages(). atomic_pool() has the intialized **pages instead of just *page.

Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com

arch/arm/mm/dma-mapping.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 601da7a..b14ee64 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -296,7 +296,7 @@ struct dma_pool { unsigned long *bitmap; unsigned long nr_pages; void *vaddr;

• struct page *page;

• struct page **pages;

};

static struct dma_pool atomic_pool = { @@ -335,12 +335,16 @@ static int __init atomic_pool_init(void) unsigned long nr_pages = pool->size >> PAGE_SHIFT; unsigned long *bitmap; struct page *page;

• struct page **pages; void *ptr; int bitmap_size = BITS_TO_LONGS(nr_pages) * sizeof(long);
• size_t size = nr_pages * sizeof(struct page *);

• bitmap = kzalloc(bitmap_size, GFP_KERNEL);

• size += bitmap_size;
• bitmap = kzalloc(size, GFP_KERNEL); if (!bitmap) goto no_bitmap;
• pages = (void *)bitmap + bitmap_size;

So you stuck a bitmap field in front of the array then? Why not just define a structure where this is clearly defined instead of doing the casting.

I just wanted to allocate only once for the members "pool->bitmap" and "pool->pages" at once. Since the size of a whole bitmap isn't known in advance, I couldn't find any fixed type for this bitmap, which pointer can be shifted without casting. IOW, they are variable length.

IMHO it is better to avoid any non-trivial things in generic arch code. Merging those 2 allocations doesn't save any significant bit of memory and might confuse someone. Better just allocate them separately.

Best regards

On Fri, Aug 24, 2012 at 11:29:03AM +0300, Hiroshi Doyu wrote:

Check the given range("start", "size") is included in "atomic_pool" or not.

Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com

arch/arm/mm/dma-mapping.c | 25 +++++++++++++++++++------ 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index b14ee64..508fde1 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -501,19 +501,32 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page) return ptr; } +static bool __in_atomic_pool(void *start, size_t size) +{

• struct dma_pool *pool = &atomic_pool;
• void *end = start + size;
• void *pool_start = pool->vaddr;
• void *pool_end = pool->vaddr + pool->size;
• if (start < pool_start || start > pool_end)

• return false;
  

• if (end > pool_end) {

• WARN(1, "freeing wrong coherent size from pool\n");
  

That does not tell what size or from what pool. Perhaps you should include some details, such as the 'size' value, the pool used, the range of the pool, etc. Something that will help _you_in the field be able to narrow down what might be wrong.

• return false;
  

• }
• return true;

+}

static int __free_from_pool(void *start, size_t size) { struct dma_pool *pool = &atomic_pool; unsigned long pageno, count; unsigned long flags;

• if (start < pool->vaddr || start > pool->vaddr + pool->size)

• return 0;
  

• if (start + size > pool->vaddr + pool->size) {

• WARN(1, "freeing wrong coherent size from pool\n");
  

• if (!__in_atomic_pool(start, size)) return 0;

• }

pageno = (start - pool->vaddr) >> PAGE_SHIFT; count = size >> PAGE_SHIFT; -- 1.7.5.4