[PATCH v2 0/3] CMA & device tree, another approach

List overview All Threads
Download

newer

older

[PATCH v2 00/18] Exynos SYSMMU...

[PATCH] drm: sti: remove gpio for...

Marek Szyprowski

11 Sep 2014 11 Sep '14

11:22 a.m.

Hello,

This is another approach to finish support for reserved memory regions defined in device tree. Previous attempts (http://lists.linaro.org/pipermail/linaro-mm-sig/2014-February/003738.html and https://lkml.org/lkml/2014/7/14/108) ended in merging parts of the code and documentation. Merged patches allow to reserve memory, but there is still no reserved memory drivers nor any code that actually uses reserved memory regions.

The final conclusion from the above mentioned threads is that there is no automated reserved memory initialization. All drivers that want to use reserved memory, should initialize it on their own.

This patch series provides two driver for reserved memory regions (one based on CMA and one based on dma_coherent allocator). The main improvement comparing to the previous version is removal of automated reserved memory for every device and support for named memory regions.

Those patches are for merging, rebased on top of recent linux-next tree.

Best regards Marek Szyprowski Samsung R&D Institute Poland

Changes since v1 (https://lkml.org/lkml/2014/8/26/339): - removed patches for named reserved regions - they will be discussed separately - added a check for 'no-map' property to dma coherent allocator (suggested by Laura Abbott) - removed example code for s5p-mfc driver

Changes since '[PATCH v2 RESEND 0/4] CMA & device tree, once again' version: (https://lkml.org/lkml/2014/7/14/108) - added return error value to of_reserved_mem_device_init() - added support for named memory regions (so more than one region can be defined per device) - added usage example - converted custom reserved memory code used by s5p-mfc driver to the generic reserved memory handling code

Patch summary:

Marek Szyprowski (3): drivers: of: add return value to of_reserved_mem_device_init drivers: dma-coherent: add initialization from device tree drivers: dma-contiguous: add initialization from device tree

drivers/base/dma-coherent.c | 145 ++++++++++++++++++++++++++++++++++------ drivers/base/dma-contiguous.c | 71 ++++++++++++++++++++ drivers/of/of_reserved_mem.c | 3 +- include/linux/cma.h | 3 + include/linux/of_reserved_mem.h | 9 ++- mm/cma.c | 62 ++++++++++++++--- 6 files changed, 259 insertions(+), 34 deletions(-)

-- 1.9.2

Show replies by date

Marek Szyprowski

11 Sep 11 Sep

11:22 a.m.

New subject: [PATCH v2 1/3] drivers: of: add return value to of_reserved_mem_device_init

Driver calling of_reserved_mem_device_init() might be interested if the initialization has been successful or not, so add support for returning error code.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/of/of_reserved_mem.c | 3 ++- include/linux/of_reserved_mem.h | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 59fb12e..7e7de03 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -243,7 +243,7 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node) * This function assign memory region pointed by "memory-region" device tree * property to the given device. */ -void of_reserved_mem_device_init(struct device *dev) +int of_reserved_mem_device_init(struct device *dev) { struct reserved_mem *rmem; struct device_node *np; @@ -260,6 +260,7 @@ void of_reserved_mem_device_init(struct device *dev)

rmem->ops->device_init(rmem, dev); dev_info(dev, "assigned reserved memory node %s\n", rmem->name); + return 0; }

/** diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h index 5b5efae..ad2f670 100644 --- a/include/linux/of_reserved_mem.h +++ b/include/linux/of_reserved_mem.h @@ -16,7 +16,7 @@ struct reserved_mem { };

struct reserved_mem_ops { - void (*device_init)(struct reserved_mem *rmem, + int (*device_init)(struct reserved_mem *rmem, struct device *dev); void (*device_release)(struct reserved_mem *rmem, struct device *dev); @@ -28,14 +28,17 @@ typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem); _OF_DECLARE(reservedmem, name, compat, init, reservedmem_of_init_fn)

#ifdef CONFIG_OF_RESERVED_MEM -void of_reserved_mem_device_init(struct device *dev); +int of_reserved_mem_device_init(struct device *dev); void of_reserved_mem_device_release(struct device *dev);

void fdt_init_reserved_mem(void); void fdt_reserved_mem_save_node(unsigned long node, const char *uname, phys_addr_t base, phys_addr_t size); #else -static inline void of_reserved_mem_device_init(struct device *dev) { } +static inline int of_reserved_mem_device_init(struct device *dev) +{ + return -ENOSYS; +} static inline void of_reserved_mem_device_release(struct device *pdev) { }

static inline void fdt_init_reserved_mem(void) { }

-- 1.9.2

Marek Szyprowski

26 Sep 26 Sep

6:44 a.m.

New subject: [PATCH v2 1/3] drivers: of: add return value to of_reserved_mem_device_init (fixup)

This patch adds missing return value to error paths.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/of/of_reserved_mem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 7e7de03..e975156 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -250,13 +250,13 @@ int of_reserved_mem_device_init(struct device *dev)

np = of_parse_phandle(dev->of_node, "memory-region", 0); if (!np) - return; + return -ENODEV;

rmem = __find_rmem(np); of_node_put(np);

if (!rmem || !rmem->ops || !rmem->ops->device_init) - return; + return -ENIVAL;

rmem->ops->device_init(rmem, dev); dev_info(dev, "assigned reserved memory node %s\n", rmem->name);

-- 1.9.2

Arnd Bergmann

8:13 p.m.

New subject: [PATCH v2 1/3] drivers: of: add return value to of_reserved_mem_device_init

On Thursday 11 September 2014, Marek Szyprowski wrote:

...

-void of_reserved_mem_device_init(struct device *dev) +int of_reserved_mem_device_init(struct device *dev) { struct reserved_mem *rmem; struct device_node *np; @@ -260,6 +260,7 @@ void of_reserved_mem_device_init(struct device *dev) rmem->ops->device_init(rmem, dev); dev_info(dev, "assigned reserved memory node %s\n", rmem->name);
  return 0;
}

This function has two other 'return' statements that now are missing a return value for the error case and cause undefined behavior in the caller.

Arnd

Marek Szyprowski

9 Oct 9 Oct

12:18 p.m.

New subject: [PATCH v3] drivers: of: add return value to of_reserved_mem_device_init

Driver calling of_reserved_mem_device_init() might be interested if the initialization has been successful or not, so add support for returning error code.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/of/of_reserved_mem.c | 7 ++++--- include/linux/of_reserved_mem.h | 9 ++++++--- 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 59fb12e..70780ad 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -243,23 +243,24 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node) * This function assign memory region pointed by "memory-region" device tree * property to the given device. */ -void of_reserved_mem_device_init(struct device *dev) +int of_reserved_mem_device_init(struct device *dev) { struct reserved_mem *rmem; struct device_node *np;

np = of_parse_phandle(dev->of_node, "memory-region", 0); if (!np) - return; + return -ENODEV;

rmem = __find_rmem(np); of_node_put(np);

if (!rmem || !rmem->ops || !rmem->ops->device_init) - return; + return -EINVAL;

rmem->ops->device_init(rmem, dev); dev_info(dev, "assigned reserved memory node %s\n", rmem->name); + return 0; }

#ifdef CONFIG_OF_RESERVED_MEM -void of_reserved_mem_device_init(struct device *dev); +int of_reserved_mem_device_init(struct device *dev); void of_reserved_mem_device_release(struct device *dev);

static inline void fdt_init_reserved_mem(void) { }

-- 1.9.2

Arnd Bergmann

13 Oct 13 Oct

11:19 a.m.

New subject: [PATCH v3] drivers: of: add return value to of_reserved_mem_device_init

On Thursday 09 October 2014 14:18:00 Marek Szyprowski wrote:

...

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 59fb12e..70780ad 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c

...

    if (!rmem || !rmem->ops || !rmem->ops->device_init)
          return;
          return -EINVAL;
rmem->ops->device_init(rmem, dev); dev_info(dev, "assigned reserved memory node %s\n", rmem->name);
  return 0;
}

You don't actually return the value from ->device_init() here but always return 0 on success. There are no callers of this function, so it's hard to tell if this actually makes a difference, but it contradicts your patch description.

...

diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h index 5b5efae..ad2f670 100644 --- a/include/linux/of_reserved_mem.h +++ b/include/linux/of_reserved_mem.h @@ -16,7 +16,7 @@ struct reserved_mem { }; struct reserved_mem_ops {
  void    (*device_init)(struct reserved_mem *rmem,
  int     (*device_init)(struct reserved_mem *rmem,
                         struct device *dev);
  void    (*device_release)(struct reserved_mem *rmem,
                            struct device *dev);

This part is definitely needed to avoid the new compile warnings we are getting.

Arnd

Marek Szyprowski

15 Oct 15 Oct

11:01 a.m.

New subject: [PATCH v4] drivers: of: add return value to of_reserved_mem_device_init

Driver calling of_reserved_mem_device_init() might be interested if the initialization has been successful or not, so add support for returning error code.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- This patch fixes build warining caused by commit 7bfa5ab6fa1b18f53fb94f922e107e6fbdc5e485 ("drivers: dma-coherent: add initialization from device tree"), which has been merged without this change and without fixing function return value. --- drivers/base/dma-contiguous.c | 3 ++- drivers/of/of_reserved_mem.c | 14 +++++++++----- include/linux/of_reserved_mem.h | 9 ++++++--- 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c index 473ff4892401..950fff9ce453 100644 --- a/drivers/base/dma-contiguous.c +++ b/drivers/base/dma-contiguous.c @@ -223,9 +223,10 @@ bool dma_release_from_contiguous(struct device *dev, struct page *pages, #undef pr_fmt #define pr_fmt(fmt) fmt

-static void rmem_cma_device_init(struct reserved_mem *rmem, struct device *dev) +static int rmem_cma_device_init(struct reserved_mem *rmem, struct device *dev) { dev_set_cma_area(dev, rmem->priv); + return 0; }

static void rmem_cma_device_release(struct reserved_mem *rmem, diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 59fb12e84e6b..dc566b38645f 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -243,23 +243,27 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node) * This function assign memory region pointed by "memory-region" device tree * property to the given device. */ -void of_reserved_mem_device_init(struct device *dev) +int of_reserved_mem_device_init(struct device *dev) { struct reserved_mem *rmem; struct device_node *np; + int ret;

np = of_parse_phandle(dev->of_node, "memory-region", 0); if (!np) - return; + return -ENODEV;

rmem = __find_rmem(np); of_node_put(np);

if (!rmem || !rmem->ops || !rmem->ops->device_init) - return; + return -EINVAL; + + ret = rmem->ops->device_init(rmem, dev); + if (ret == 0) + dev_info(dev, "assigned reserved memory node %s\n", rmem->name);

- rmem->ops->device_init(rmem, dev); - dev_info(dev, "assigned reserved memory node %s\n", rmem->name); + return ret; }

/** diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h index 5b5efae09135..ad2f67054372 100644 --- a/include/linux/of_reserved_mem.h +++ b/include/linux/of_reserved_mem.h @@ -16,7 +16,7 @@ struct reserved_mem { };

#ifdef CONFIG_OF_RESERVED_MEM -void of_reserved_mem_device_init(struct device *dev); +int of_reserved_mem_device_init(struct device *dev); void of_reserved_mem_device_release(struct device *dev);

static inline void fdt_init_reserved_mem(void) { }

-- 1.9.2

Arnd Bergmann

20 Oct 20 Oct

7:04 p.m.

New subject: [PATCH v4] drivers: of: add return value to of_reserved_mem_device_init

On Wednesday 15 October 2014 13:01:42 Marek Szyprowski wrote:

...

Driver calling of_reserved_mem_device_init() might be interested if the initialization has been successful or not, so add support for returning error code.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

This patch fixes build warining caused by commit 7bfa5ab6fa1b18f53fb94f922e107e6fbdc5e485 ("drivers: dma-coherent: add initialization from device tree"), which has been merged without this change and without fixing function return value.

Acked-by: Arnd Bergmann arnd@arndb.de

It also makes sense to encode the information you have below the --- line as

Fixes: 7bfa5ab6fa1b1 ("drivers: dma-coherent: add initialization from device tree")

Marek Szyprowski

11 Sep 11 Sep

11:22 a.m.

New subject: [PATCH v2 2/3] drivers: dma-coherent: add initialization from device tree

Initialization procedure of dma coherent pool has been split into two parts, so memory pool can now be initialized without assigning to particular struct device. Then initialized region can be assigned to more than one struct device. To protect from concurent allocations from different devices, a spinlock has been added to dma_coherent_mem structure. The last part of this patch adds support for handling 'shared-dma-pool' reserved-memory device tree nodes.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/base/dma-coherent.c | 145 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 126 insertions(+), 19 deletions(-)

diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c index 7d6e84a..8f88c91 100644 --- a/drivers/base/dma-coherent.c +++ b/drivers/base/dma-coherent.c @@ -14,11 +14,14 @@ struct dma_coherent_mem { int size; int flags; unsigned long *bitmap; + spinlock_t spinlock; };

-int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr, - dma_addr_t device_addr, size_t size, int flags) +static int dma_init_coherent_memory(phys_addr_t phys_addr, dma_addr_t device_addr, + size_t size, int flags, + struct dma_coherent_mem **mem) { + struct dma_coherent_mem *dma_mem = NULL; void __iomem *mem_base = NULL; int pages = size >> PAGE_SHIFT; int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long); @@ -27,27 +30,26 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr, goto out; if (!size) goto out; - if (dev->dma_mem) - goto out; - - /* FIXME: this routine just ignores DMA_MEMORY_INCLUDES_CHILDREN */

mem_base = ioremap(phys_addr, size); if (!mem_base) goto out;

- dev->dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL); - if (!dev->dma_mem) + dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL); + if (!dma_mem) goto out; - dev->dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL); - if (!dev->dma_mem->bitmap) + dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL); + if (!dma_mem->bitmap) goto free1_out;

- dev->dma_mem->virt_base = mem_base; - dev->dma_mem->device_base = device_addr; - dev->dma_mem->pfn_base = PFN_DOWN(phys_addr); - dev->dma_mem->size = pages; - dev->dma_mem->flags = flags; + dma_mem->virt_base = mem_base; + dma_mem->device_base = device_addr; + dma_mem->pfn_base = PFN_DOWN(phys_addr); + dma_mem->size = pages; + dma_mem->flags = flags; + spin_lock_init(&dma_mem->spinlock); + + *mem = dma_mem;

if (flags & DMA_MEMORY_MAP) return DMA_MEMORY_MAP; @@ -55,12 +57,51 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr, return DMA_MEMORY_IO;

free1_out: - kfree(dev->dma_mem); + kfree(dma_mem); out: if (mem_base) iounmap(mem_base); return 0; } + +static void dma_release_coherent_memory(struct dma_coherent_mem *mem) +{ + if (!mem) + return; + iounmap(mem->virt_base); + kfree(mem->bitmap); + kfree(mem); +} + +static int dma_assign_coherent_memory(struct device *dev, + struct dma_coherent_mem *mem) +{ + if (dev->dma_mem) + return -EBUSY; + + dev->dma_mem = mem; + /* FIXME: this routine just ignores DMA_MEMORY_INCLUDES_CHILDREN */ + + return 0; +} + +int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr, + dma_addr_t device_addr, size_t size, int flags) +{ + struct dma_coherent_mem *mem; + int ret; + + ret = dma_init_coherent_memory(phys_addr, device_addr, size, flags, + &mem); + if (ret == 0) + return 0; + + if (dma_assign_coherent_memory(dev, mem) == 0) + return ret; + + dma_release_coherent_memory(mem); + return 0; +} EXPORT_SYMBOL(dma_declare_coherent_memory);

void dma_release_declared_memory(struct device *dev) @@ -69,10 +110,8 @@ void dma_release_declared_memory(struct device *dev)

if (!mem) return; + dma_release_coherent_memory(mem); dev->dma_mem = NULL; - iounmap(mem->virt_base); - kfree(mem->bitmap); - kfree(mem); } EXPORT_SYMBOL(dma_release_declared_memory);

@@ -80,6 +119,7 @@ void *dma_mark_declared_memory_occupied(struct device *dev, dma_addr_t device_addr, size_t size) { struct dma_coherent_mem *mem = dev->dma_mem; + unsigned long flags; int pos, err;

size += device_addr & ~PAGE_MASK; @@ -87,8 +127,11 @@ void *dma_mark_declared_memory_occupied(struct device *dev, if (!mem) return ERR_PTR(-EINVAL);

+ spin_lock_irqsave(&mem->spinlock, flags); pos = (device_addr - mem->device_base) >> PAGE_SHIFT; err = bitmap_allocate_region(mem->bitmap, pos, get_order(size)); + spin_unlock_irqrestore(&mem->spinlock, flags); + if (err != 0) return ERR_PTR(err); return mem->virt_base + (pos << PAGE_SHIFT); @@ -115,6 +158,7 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size, { struct dma_coherent_mem *mem; int order = get_order(size); + unsigned long flags; int pageno;

if (!dev) @@ -124,6 +168,7 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size, return 0;

*ret = NULL; + spin_lock_irqsave(&mem->spinlock, flags);

if (unlikely(size > (mem->size << PAGE_SHIFT))) goto err; @@ -138,10 +183,12 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size, *dma_handle = mem->device_base + (pageno << PAGE_SHIFT); *ret = mem->virt_base + (pageno << PAGE_SHIFT); memset(*ret, 0, size); + spin_unlock_irqrestore(&mem->spinlock, flags);

return 1;

err: + spin_unlock_irqrestore(&mem->spinlock, flags); /* * In the case where the allocation can not be satisfied from the * per-device area, try to fall back to generic memory if the @@ -171,8 +218,11 @@ int dma_release_from_coherent(struct device *dev, int order, void *vaddr) if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT))) { int page = (vaddr - mem->virt_base) >> PAGE_SHIFT; + unsigned long flags;

+ spin_lock_irqsave(&mem->spinlock, flags); bitmap_release_region(mem->bitmap, page, order); + spin_unlock_irqrestore(&mem->spinlock, flags); return 1; } return 0; @@ -218,3 +268,60 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma, return 0; } EXPORT_SYMBOL(dma_mmap_from_coherent); + +/* + * Support for reserved memory regions defined in device tree + */ +#ifdef CONFIG_OF_RESERVED_MEM +#include <linux/of.h> +#include <linux/of_fdt.h> +#include <linux/of_reserved_mem.h> + +static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) +{ + struct dma_coherent_mem *mem = rmem->priv; + if (!mem && + dma_init_coherent_memory(rmem->base, rmem->base, rmem->size, + DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE, + &mem) != DMA_MEMORY_MAP) { + pr_info("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n", + &rmem->base, (unsigned long)rmem->size / SZ_1M); + return -ENODEV; + } + rmem->priv = mem; + dma_assign_coherent_memory(dev, mem); + return 0; +} + +static void rmem_dma_device_release(struct reserved_mem *rmem, + struct device *dev) +{ + dev->dma_mem = NULL; +} + +static const struct reserved_mem_ops rmem_dma_ops = { + .device_init = rmem_dma_device_init, + .device_release = rmem_dma_device_release, +}; + +static int __init rmem_dma_setup(struct reserved_mem *rmem) +{ + unsigned long node = rmem->fdt_node; + + if (of_get_flat_dt_prop(node, "reusable", NULL)) + return -EINVAL; + +#ifdef CONFIG_ARM + if (!of_get_flat_dt_prop(node, "no-map", NULL)) { + pr_info("Reserved memory: regions without no-map are not yet supported\n"); + return -EINVAL; + } +#endif + + rmem->ops = &rmem_dma_ops; + pr_info("Reserved memory: created DMA memory pool at %pa, size %ld MiB\n", + &rmem->base, (unsigned long)rmem->size / SZ_1M); + return 0; +} +RESERVEDMEM_OF_DECLARE(dma, "shared-dma-pool", rmem_dma_setup); +#endif

-- 1.9.2

Andrew Morton

24 Sep 24 Sep

10:26 p.m.

New subject: [PATCH v2 2/3] drivers: dma-coherent: add initialization from device tree

On Thu, 11 Sep 2014 13:22:40 +0200 Marek Szyprowski m.szyprowski@samsung.com wrote:

...

Initialization procedure of dma coherent pool has been split into two parts, so memory pool can now be initialized without assigning to particular struct device. Then initialized region can be assigned to more than one struct device. To protect from concurent allocations from different devices, a spinlock has been added to dma_coherent_mem structure. The last part of this patch adds support for handling 'shared-dma-pool' reserved-memory device tree nodes.

--- a/drivers/base/dma-coherent.c +++ b/drivers/base/dma-coherent.c @@ -14,11 +14,14 @@ struct dma_coherent_mem { int size; int flags; unsigned long *bitmap;

spinlock_t spinlock;

A bit of documentation would be nice: explain what the lock protects, that it is irq-safe, etc.

...

}; -int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
		dma_addr_t device_addr, size_t size, int flags)
+static int dma_init_coherent_memory(phys_addr_t phys_addr, dma_addr_t device_addr,
	     size_t size, int flags,
	     struct dma_coherent_mem **mem)
{

struct dma_coherent_mem *dma_mem = NULL;

The only reason to initialise this is so we can kfree() it without checking. In which case we don't need label free1_out?

--- a/drivers/base/dma-coherent.c~drivers-dma-coherent-add-initialization-from-device-tree-fix +++ a/drivers/base/dma-coherent.c @@ -40,7 +40,7 @@ static int dma_init_coherent_memory(phys goto out; dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL); if (!dma_mem->bitmap) - goto free1_out; + goto out;

dma_mem->virt_base = mem_base; dma_mem->device_base = device_addr; @@ -56,9 +56,8 @@ static int dma_init_coherent_memory(phys

return DMA_MEMORY_IO;

- free1_out: +out: kfree(dma_mem); - out: if (mem_base) iounmap(mem_base); return 0; _

Andrew Morton

10:28 p.m.

New subject: [PATCH v2 2/3] drivers: dma-coherent: add initialization from device tree

On Thu, 11 Sep 2014 13:22:40 +0200 Marek Szyprowski m.szyprowski@samsung.com wrote:

...

Initialization procedure of dma coherent pool has been split into two parts, so memory pool can now be initialized without assigning to particular struct device. Then initialized region can be assigned to more than one struct device. To protect from concurent allocations from different devices, a spinlock has been added to dma_coherent_mem structure. The last part of this patch adds support for handling 'shared-dma-pool' reserved-memory device tree nodes.

...

+static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) +{
struct dma_coherent_mem *mem = rmem->priv;

if (!mem &&
   dma_init_coherent_memory(rmem->base, rmem->base, rmem->size,
		     DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE,
		     &mem) != DMA_MEMORY_MAP) {
pr_info("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n",
	&rmem->base, (unsigned long)rmem->size / SZ_1M);

pr_info() seems inappropriate here. It's an error, so pr_err()?

...

return -ENODEV;
}

rmem->priv = mem;

dma_assign_coherent_memory(dev, mem);

return 0;
+}

...

+static int __init rmem_dma_setup(struct reserved_mem *rmem) +{
unsigned long node = rmem->fdt_node;

if (of_get_flat_dt_prop(node, "reusable", NULL))
return -EINVAL;
+#ifdef CONFIG_ARM
if (!of_get_flat_dt_prop(node, "no-map", NULL)) {
pr_info("Reserved memory: regions without no-map are not yet supported\n");

Same here.

...

return -EINVAL;
}
+#endif
rmem->ops = &rmem_dma_ops;

pr_info("Reserved memory: created DMA memory pool at %pa, size %ld MiB\n",
&rmem->base, (unsigned long)rmem->size / SZ_1M);

This *is* an appropriate use of pr_info().

...

return 0;

+} +RESERVEDMEM_OF_DECLARE(dma, "shared-dma-pool", rmem_dma_setup); +#endif

Marek Szyprowski

11 Sep 11 Sep

11:22 a.m.

New subject: [PATCH v2 3/3] drivers: dma-contiguous: add initialization from device tree

Add a function to create CMA region from previously reserved memory and add support for handling 'shared-dma-pool' reserved-memory device tree nodes.

Based on previous code provided by Josh Cartwright joshc@codeaurora.org

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/base/dma-contiguous.c | 71 +++++++++++++++++++++++++++++++++++++++++++ include/linux/cma.h | 3 ++ mm/cma.c | 62 ++++++++++++++++++++++++++++++------- 3 files changed, 125 insertions(+), 11 deletions(-)

diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c index 6606abd..eefb81b8 100644 --- a/drivers/base/dma-contiguous.c +++ b/drivers/base/dma-contiguous.c @@ -211,3 +211,74 @@ bool dma_release_from_contiguous(struct device *dev, struct page *pages, { return cma_release(dev_get_cma_area(dev), pages, count); } + +/* + * Support for reserved memory regions defined in device tree + */ +#ifdef CONFIG_OF_RESERVED_MEM +#include <linux/of.h> +#include <linux/of_fdt.h> +#include <linux/of_reserved_mem.h> + +#undef pr_fmt +#define pr_fmt(fmt) fmt + +static int rmem_cma_device_init(struct reserved_mem *rmem, struct device *dev) +{ + struct cma *cma = rmem->priv; + if (!cma) + return -ENODEV; + + dev_set_cma_area(dev, cma); + return 0; +} + +static void rmem_cma_device_release(struct reserved_mem *rmem, + struct device *dev) +{ + dev_set_cma_area(dev, NULL); +} + +static const struct reserved_mem_ops rmem_cma_ops = { + .device_init = rmem_cma_device_init, + .device_release = rmem_cma_device_release, +}; + +static int __init rmem_cma_setup(struct reserved_mem *rmem) +{ + phys_addr_t align = PAGE_SIZE << max(MAX_ORDER - 1, pageblock_order); + phys_addr_t mask = align - 1; + unsigned long node = rmem->fdt_node; + struct cma *cma; + int err; + + if (!of_get_flat_dt_prop(node, "reusable", NULL) || + of_get_flat_dt_prop(node, "no-map", NULL)) + return -EINVAL; + + if ((rmem->base & mask) || (rmem->size & mask)) { + pr_err("Reserved memory: incorrect alignment of CMA region\n"); + return -EINVAL; + } + + err = cma_init_reserved_mem(rmem->base, rmem->size, 0, &cma); + if (err) { + pr_err("Reserved memory: unable to setup CMA region\n"); + return err; + } + /* Architecture specific contiguous memory fixup. */ + dma_contiguous_early_fixup(rmem->base, rmem->size); + + if (of_get_flat_dt_prop(node, "linux,cma-default", NULL)) + dma_contiguous_set_default(cma); + + rmem->ops = &rmem_cma_ops; + rmem->priv = cma; + + pr_info("Reserved memory: created CMA memory pool at %pa, size %ld MiB\n", + &rmem->base, (unsigned long)rmem->size / SZ_1M); + + return 0; +} +RESERVEDMEM_OF_DECLARE(cma, "shared-dma-pool", rmem_cma_setup); +#endif diff --git a/include/linux/cma.h b/include/linux/cma.h index 371b930..0430ed0 100644 --- a/include/linux/cma.h +++ b/include/linux/cma.h @@ -22,6 +22,9 @@ extern int __init cma_declare_contiguous(phys_addr_t size, phys_addr_t base, phys_addr_t limit, phys_addr_t alignment, unsigned int order_per_bit, bool fixed, struct cma **res_cma); +extern int cma_init_reserved_mem(phys_addr_t size, + phys_addr_t base, int order_per_bit, + struct cma **res_cma); extern struct page *cma_alloc(struct cma *cma, int count, unsigned int align); extern bool cma_release(struct cma *cma, struct page *pages, int count); #endif diff --git a/mm/cma.c b/mm/cma.c index 474c644..014e145 100644 --- a/mm/cma.c +++ b/mm/cma.c @@ -141,6 +141,54 @@ static int __init cma_init_reserved_areas(void) core_initcall(cma_init_reserved_areas);

/** + * cma_init_reserved_mem() - create custom contiguous area from reserved memory + * @base: Base address of the reserved area + * @size: Size of the reserved area (in bytes), + * @order_per_bit: Order of pages represented by one bit on bitmap. + * @res_cma: Pointer to store the created cma region. + * + * This function creates custom contiguous area from already reserved memory. + */ +int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size, + int order_per_bit, struct cma **res_cma) +{ + struct cma *cma; + phys_addr_t alignment; + + /* Sanity checks */ + if (cma_area_count == ARRAY_SIZE(cma_areas)) { + pr_err("Not enough slots for CMA reserved regions!\n"); + return -ENOSPC; + } + + if (!size || !memblock_is_region_reserved(base, size)) + return -EINVAL; + + /* ensure minimal alignment requied by mm core */ + alignment = PAGE_SIZE << max(MAX_ORDER - 1, pageblock_order); + + /* alignment should be aligned with order_per_bit */ + if (!IS_ALIGNED(alignment >> PAGE_SHIFT, 1 << order_per_bit)) + return -EINVAL; + + if (ALIGN(base, alignment) != base || ALIGN(size, alignment) != size) + return -EINVAL; + + /* + * Each reserved area must be initialised later, when more kernel + * subsystems (like slab allocator) are available. + */ + cma = &cma_areas[cma_area_count]; + cma->base_pfn = PFN_DOWN(base); + cma->count = size >> PAGE_SHIFT; + cma->order_per_bit = order_per_bit; + *res_cma = cma; + cma_area_count++; + + return 0; +} + +/** * cma_declare_contiguous() - reserve custom contiguous area * @base: Base address of the reserved area optional, use 0 for any * @size: Size of the reserved area (in bytes), @@ -163,7 +211,6 @@ int __init cma_declare_contiguous(phys_addr_t base, phys_addr_t alignment, unsigned int order_per_bit, bool fixed, struct cma **res_cma) { - struct cma *cma; phys_addr_t memblock_end = memblock_end_of_DRAM(); phys_addr_t highmem_start = __pa(high_memory); int ret = 0; @@ -235,16 +282,9 @@ int __init cma_declare_contiguous(phys_addr_t base, } }

- /* - * Each reserved area must be initialised later, when more kernel - * subsystems (like slab allocator) are available. - */ - cma = &cma_areas[cma_area_count]; - cma->base_pfn = PFN_DOWN(base); - cma->count = size >> PAGE_SHIFT; - cma->order_per_bit = order_per_bit; - *res_cma = cma; - cma_area_count++; + ret = cma_init_reserved_mem(base, size, order_per_bit, res_cma); + if (ret) + goto err;

pr_info("Reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M, (unsigned long)base);

-- 1.9.2

Marek Szyprowski

23 Sep 23 Sep

8:05 a.m.

New subject: [PATCH v2 0/3] CMA & device tree, another approach

Hi Andrew,

On 2014-09-11 13:22, Marek Szyprowski wrote:

...

Hello,

This is another approach to finish support for reserved memory regions defined in device tree. Previous attempts (http://lists.linaro.org/pipermail/linaro-mm-sig/2014-February/003738.html and https://lkml.org/lkml/2014/7/14/108) ended in merging parts of the code and documentation. Merged patches allow to reserve memory, but there is still no reserved memory drivers nor any code that actually uses reserved memory regions.

The final conclusion from the above mentioned threads is that there is no automated reserved memory initialization. All drivers that want to use reserved memory, should initialize it on their own.

This patch series provides two driver for reserved memory regions (one based on CMA and one based on dma_coherent allocator). The main improvement comparing to the previous version is removal of automated reserved memory for every device and support for named memory regions.

Those patches are for merging, rebased on top of recent linux-next tree.

Andrew: could you take those patches to your "next" branch together with other CMA-related changes that are already there?

...

Best regards Marek Szyprowski Samsung R&D Institute Poland

Changes since v1 (https://lkml.org/lkml/2014/8/26/339):

removed patches for named reserved regions - they will be discussed separately

added a check for 'no-map' property to dma coherent allocator (suggested by Laura Abbott)

removed example code for s5p-mfc driver

Changes since '[PATCH v2 RESEND 0/4] CMA & device tree, once again' version: (https://lkml.org/lkml/2014/7/14/108)

added return error value to of_reserved_mem_device_init()

added support for named memory regions (so more than one region can be defined per device)

added usage example - converted custom reserved memory code used by s5p-mfc driver to the generic reserved memory handling code

Patch summary:

Marek Szyprowski (3): drivers: of: add return value to of_reserved_mem_device_init drivers: dma-coherent: add initialization from device tree drivers: dma-contiguous: add initialization from device tree

drivers/base/dma-coherent.c | 145 ++++++++++++++++++++++++++++++++++------ drivers/base/dma-contiguous.c | 71 ++++++++++++++++++++ drivers/of/of_reserved_mem.c | 3 +- include/linux/cma.h | 3 + include/linux/of_reserved_mem.h | 9 ++- mm/cma.c | 62 ++++++++++++++--- 6 files changed, 259 insertions(+), 34 deletions(-)

Best regards

-- Marek Szyprowski, PhD Samsung R&D Institute Poland

3718

days inactive

3757

days old

linaro-mm-sig@lists.linaro.org

12 comments

participants

tags (0)

participants (3)

Andrew Morton
Arnd Bergmann
Marek Szyprowski