Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754525AbaG2XjP (ORCPT ); Tue, 29 Jul 2014 19:39:15 -0400 Received: from mail-pd0-f178.google.com ([209.85.192.178]:40577 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753918AbaG2XjN (ORCPT ); Tue, 29 Jul 2014 19:39:13 -0400 From: Grant Likely Subject: Re: [PATCH v2 RESEND 3/4] drivers: dma-coherent: add initialization from device tree To: Marek Szyprowski , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Cc: Marek Szyprowski , linaro-mm-sig@lists.linaro.org, devicetree@vger.kernel.org, Arnd Bergmann , Michal Nazarewicz , Tomasz Figa , Sascha Hauer , Laura Abbott , Nishanth Peethambaran , Marc , Josh Cartwright , Catalin Marinas , Will Deacon , Paul Mackerras , Jon Medhurst , Joonsoo Kim , "Aneesh Kumar K.V." , Andrew Morton In-Reply-To: <1405326487-15346-4-git-send-email-m.szyprowski@samsung.com> References: <1405326487-15346-1-git-send-email-m.szyprowski@samsung.com> <1405326487-15346-4-git-send-email-m.szyprowski@samsung.com> Date: Tue, 29 Jul 2014 15:54:32 -0600 Message-Id: <20140729215432.4B73FC40738@trevor.secretlab.ca> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 14 Jul 2014 10:28:06 +0200, Marek Szyprowski 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. > > Signed-off-by: Marek Szyprowski I think this looks okay. It isn't in my area of expertise though. Comments below. > --- > drivers/base/dma-coherent.c | 137 ++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 118 insertions(+), 19 deletions(-) > > diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c > index 7d6e84a51424..7185a4f247e1 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) This is a bit odd. Why wouldn't you return the dma_mem pointer directly instead of passing in a **mem argument? > { > + 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,52 @@ 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 > +#include > +#include > + > +static void rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) > +{ > + struct dma_coherent_mem *mem = rmem->priv; Will the reserved_mem->priv pointer ever point to some other kind of structure? How do we know that the pointer here is always a dma_coherent_mem struct (if there are other uses of priv, what is the guarantee against another user assigning something to it?) Is it the reserved_mem_ops below that provide the guarantee? If it is a risk, then the alternative would be to put an explicit dma_coherent_mem pointer into the reserved_mem structure. > + 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; > + } > + rmem->priv = mem; > + dma_assign_coherent_memory(dev, mem); > +} > + > +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; > + > + 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/