| Message ID | 1406079308-5232-4-git-send-email-lauraa@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 23, 2014 at 02:35:06AM +0100, Laura Abbott wrote: > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -298,37 +298,19 @@ static void * > __dma_alloc_remap(struct page *page, size_t size, gfp_t gfp, pgprot_t prot, > const void *caller) > { > - struct vm_struct *area; > - unsigned long addr; > - > /* > * DMA allocation can be mapped to user space, so lets > * set VM_USERMAP flags too. > */ > - area = get_vm_area_caller(size, VM_ARM_DMA_CONSISTENT | VM_USERMAP, > - caller); > - if (!area) > - return NULL; > - addr = (unsigned long)area->addr; > - area->phys_addr = __pfn_to_phys(page_to_pfn(page)); > - > - if (ioremap_page_range(addr, addr + size, area->phys_addr, prot)) { > - vunmap((void *)addr); > - return NULL; > - } > - return (void *)addr; > + return dma_common_contiguous_remap(page, size, > + VM_ARM_DMA_CONSISTENT | VM_USERMAP, > + prot, caller); I think we still need at least a comment in the commit log since the arm code is moving from ioremap_page_range() to map_vm_area(). There is a slight performance penalty with the addition of a kmalloc() on this path. Or even better (IMO), see below. > --- a/drivers/base/dma-mapping.c > +++ b/drivers/base/dma-mapping.c [...] > +void *dma_common_contiguous_remap(struct page *page, size_t size, > + unsigned long vm_flags, > + pgprot_t prot, const void *caller) > +{ > + int i; > + struct page **pages; > + void *ptr; > + > + pages = kmalloc(sizeof(struct page *) << get_order(size), GFP_KERNEL); > + if (!pages) > + return NULL; > + > + for (i = 0; i < (size >> PAGE_SHIFT); i++) > + pages[i] = page + i; > + > + ptr = dma_common_pages_remap(pages, size, vm_flags, prot, caller); > + > + kfree(pages); > + > + return ptr; > +} You could avoid the dma_common_page_remap() here (and kmalloc) and simply use ioremap_page_range(). We know that dma_common_contiguous_remap() is only called with contiguous physical range, so ioremap_page_range() is suitable. It also makes it a non-functional change for arch/arm.
On Wed, Jul 23, 2014 at 02:35:06AM +0100, Laura Abbott wrote: > +void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) > +{ > + struct vm_struct *area = find_vm_area(cpu_addr); > + > + if (!area || (area->flags & vm_flags) != vm_flags) { > + WARN(1, "trying to free invalid coherent area: %p\n", cpu_addr); > + return; > + } > + > + unmap_kernel_range((unsigned long)cpu_addr, size); > + vunmap(cpu_addr); > +} One more thing - is unmap_kernel_range() needed here? vunmap() ends up calling vunmap_page_range(), same as unmap_kernel_range(). I think one difference is that in the vunmap case, TLB flushing is done lazily.
On 7/23/2014 3:45 AM, Catalin Marinas wrote: > On Wed, Jul 23, 2014 at 02:35:06AM +0100, Laura Abbott wrote: >> --- a/arch/arm/mm/dma-mapping.c >> +++ b/arch/arm/mm/dma-mapping.c >> @@ -298,37 +298,19 @@ static void * >> __dma_alloc_remap(struct page *page, size_t size, gfp_t gfp, pgprot_t prot, >> const void *caller) >> { >> - struct vm_struct *area; >> - unsigned long addr; >> - >> /* >> * DMA allocation can be mapped to user space, so lets >> * set VM_USERMAP flags too. >> */ >> - area = get_vm_area_caller(size, VM_ARM_DMA_CONSISTENT | VM_USERMAP, >> - caller); >> - if (!area) >> - return NULL; >> - addr = (unsigned long)area->addr; >> - area->phys_addr = __pfn_to_phys(page_to_pfn(page)); >> - >> - if (ioremap_page_range(addr, addr + size, area->phys_addr, prot)) { >> - vunmap((void *)addr); >> - return NULL; >> - } >> - return (void *)addr; >> + return dma_common_contiguous_remap(page, size, >> + VM_ARM_DMA_CONSISTENT | VM_USERMAP, >> + prot, caller); > > I think we still need at least a comment in the commit log since the arm > code is moving from ioremap_page_range() to map_vm_area(). There is a > slight performance penalty with the addition of a kmalloc() on this > path. > > Or even better (IMO), see below. > >> --- a/drivers/base/dma-mapping.c >> +++ b/drivers/base/dma-mapping.c > [...] >> +void *dma_common_contiguous_remap(struct page *page, size_t size, >> + unsigned long vm_flags, >> + pgprot_t prot, const void *caller) >> +{ >> + int i; >> + struct page **pages; >> + void *ptr; >> + >> + pages = kmalloc(sizeof(struct page *) << get_order(size), GFP_KERNEL); >> + if (!pages) >> + return NULL; >> + >> + for (i = 0; i < (size >> PAGE_SHIFT); i++) >> + pages[i] = page + i; >> + >> + ptr = dma_common_pages_remap(pages, size, vm_flags, prot, caller); >> + >> + kfree(pages); >> + >> + return ptr; >> +} > > You could avoid the dma_common_page_remap() here (and kmalloc) and > simply use ioremap_page_range(). We know that > dma_common_contiguous_remap() is only called with contiguous physical > range, so ioremap_page_range() is suitable. It also makes it a > non-functional change for arch/arm. > My original thought with using map_vm_area vs. ioremap_page_range was that ioremap_page_range is really intended for mapping io devices and the like into the kernel virtual address space. map_vm_area is designed to handle pages of kernel managed memory. Perhaps it's too nit-picky a distinction though. Thanks, Laura
On Wed, Jul 23, 2014 at 10:56:11PM +0100, Laura Abbott wrote: > On 7/23/2014 3:45 AM, Catalin Marinas wrote: > > On Wed, Jul 23, 2014 at 02:35:06AM +0100, Laura Abbott wrote: > >> --- a/drivers/base/dma-mapping.c > >> +++ b/drivers/base/dma-mapping.c > > [...] > >> +void *dma_common_contiguous_remap(struct page *page, size_t size, > >> + unsigned long vm_flags, > >> + pgprot_t prot, const void *caller) > >> +{ > >> + int i; > >> + struct page **pages; > >> + void *ptr; > >> + > >> + pages = kmalloc(sizeof(struct page *) << get_order(size), GFP_KERNEL); > >> + if (!pages) > >> + return NULL; > >> + > >> + for (i = 0; i < (size >> PAGE_SHIFT); i++) > >> + pages[i] = page + i; > >> + > >> + ptr = dma_common_pages_remap(pages, size, vm_flags, prot, caller); > >> + > >> + kfree(pages); > >> + > >> + return ptr; > >> +} > > > > You could avoid the dma_common_page_remap() here (and kmalloc) and > > simply use ioremap_page_range(). We know that > > dma_common_contiguous_remap() is only called with contiguous physical > > range, so ioremap_page_range() is suitable. It also makes it a > > non-functional change for arch/arm. > > My original thought with using map_vm_area vs. ioremap_page_range was > that ioremap_page_range is really intended for mapping io devices and > the like into the kernel virtual address space. map_vm_area is designed > to handle pages of kernel managed memory. Perhaps it's too nit-picky > a distinction though. I think you are right. We had a discussion in the past about using ioremap on valid RAM addresses and decided not to allow this. This would be similar with the ioremap_page_range() here. From my perspective, you can leave the code as is (wouldn't be any functional change for arm64 since it was using vmap() already). But please add a comment in the commit log about this change.
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 3116880..8c4c755 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -298,37 +298,19 @@ static void * __dma_alloc_remap(struct page *page, size_t size, gfp_t gfp, pgprot_t prot, const void *caller) { - struct vm_struct *area; - unsigned long addr; - /* * DMA allocation can be mapped to user space, so lets * set VM_USERMAP flags too. */ - area = get_vm_area_caller(size, VM_ARM_DMA_CONSISTENT | VM_USERMAP, - caller); - if (!area) - return NULL; - addr = (unsigned long)area->addr; - area->phys_addr = __pfn_to_phys(page_to_pfn(page)); - - if (ioremap_page_range(addr, addr + size, area->phys_addr, prot)) { - vunmap((void *)addr); - return NULL; - } - return (void *)addr; + return dma_common_contiguous_remap(page, size, + VM_ARM_DMA_CONSISTENT | VM_USERMAP, + prot, caller); } static void __dma_free_remap(void *cpu_addr, size_t size) { - unsigned int flags = VM_ARM_DMA_CONSISTENT | VM_USERMAP; - struct vm_struct *area = find_vm_area(cpu_addr); - if (!area || (area->flags & flags) != flags) { - WARN(1, "trying to free invalid coherent area: %p\n", cpu_addr); - return; - } - unmap_kernel_range((unsigned long)cpu_addr, size); - vunmap(cpu_addr); + dma_common_free_remap(cpu_addr, size, + VM_ARM_DMA_CONSISTENT | VM_USERMAP); } #define DEFAULT_DMA_COHERENT_POOL_SIZE SZ_256K @@ -1262,29 +1244,8 @@ static void * __iommu_alloc_remap(struct page **pages, size_t size, gfp_t gfp, pgprot_t prot, const void *caller) { - unsigned int i, nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; - struct vm_struct *area; - unsigned long p; - - area = get_vm_area_caller(size, VM_ARM_DMA_CONSISTENT | VM_USERMAP, - caller); - if (!area) - return NULL; - - area->pages = pages; - area->nr_pages = nr_pages; - p = (unsigned long)area->addr; - - for (i = 0; i < nr_pages; i++) { - phys_addr_t phys = __pfn_to_phys(page_to_pfn(pages[i])); - if (ioremap_page_range(p, p + PAGE_SIZE, phys, prot)) - goto err; - p += PAGE_SIZE; - } - return area->addr; -err: - unmap_kernel_range((unsigned long)area->addr, size); - vunmap(area->addr); + return dma_common_pages_remap(pages, size, + VM_ARM_DMA_CONSISTENT | VM_USERMAP, prot, caller); return NULL; } @@ -1492,8 +1453,8 @@ void arm_iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr, } if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs)) { - unmap_kernel_range((unsigned long)cpu_addr, size); - vunmap(cpu_addr); + dma_common_free_remap(cpu_addr, size, + VM_ARM_DMA_CONSISTENT | VM_USERMAP); } __iommu_remove_mapping(dev, handle, size); diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c index 6cd08e1..ddaad8d 100644 --- a/drivers/base/dma-mapping.c +++ b/drivers/base/dma-mapping.c @@ -10,6 +10,8 @@ #include <linux/dma-mapping.h> #include <linux/export.h> #include <linux/gfp.h> +#include <linux/slab.h> +#include <linux/vmalloc.h> #include <asm-generic/dma-coherent.h> /* @@ -267,3 +269,68 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, return ret; } EXPORT_SYMBOL(dma_common_mmap); + +/* + * remaps an allocated contiguous region into another vm_area. + * Cannot be used in non-sleeping contexts + */ + +void *dma_common_contiguous_remap(struct page *page, size_t size, + unsigned long vm_flags, + pgprot_t prot, const void *caller) +{ + int i; + struct page **pages; + void *ptr; + + pages = kmalloc(sizeof(struct page *) << get_order(size), GFP_KERNEL); + if (!pages) + return NULL; + + for (i = 0; i < (size >> PAGE_SHIFT); i++) + pages[i] = page + i; + + ptr = dma_common_pages_remap(pages, size, vm_flags, prot, caller); + + kfree(pages); + + return ptr; +} + +/* + * remaps an array of PAGE_SIZE pages into another vm_area + * Cannot be used in non-sleeping contexts + */ +void *dma_common_pages_remap(struct page **pages, size_t size, + unsigned long vm_flags, pgprot_t prot, + const void *caller) +{ + struct vm_struct *area; + + area = get_vm_area_caller(size, vm_flags, caller); + if (!area) + return NULL; + + if (map_vm_area(area, prot, pages)) { + vunmap(area->addr); + return NULL; + } + + return area->addr; +} + +/* + * unmaps a range previously mapped by dma_common_*_remap + */ +void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) +{ + struct vm_struct *area = find_vm_area(cpu_addr); + + if (!area || (area->flags & vm_flags) != vm_flags) { + WARN(1, "trying to free invalid coherent area: %p\n", cpu_addr); + return; + } + + unmap_kernel_range((unsigned long)cpu_addr, size); + vunmap(cpu_addr); +} diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h index de8bf89..a9fd248 100644 --- a/include/asm-generic/dma-mapping-common.h +++ b/include/asm-generic/dma-mapping-common.h @@ -179,6 +179,15 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, extern int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size); +void *dma_common_contiguous_remap(struct page *page, size_t size, + unsigned long vm_flags, + pgprot_t prot, const void *caller); + +void *dma_common_pages_remap(struct page **pages, size_t size, + unsigned long vm_flags, pgprot_t prot, + const void *caller); +void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags); + /** * dma_mmap_attrs - map a coherent DMA allocation into user space * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
For architectures without coherent DMA, memory for DMA may need to be remapped with coherent attributes. Factor out the the remapping code from arm and put it in a common location to reduced code duplication. Signed-off-by: Laura Abbott <lauraa@codeaurora.org> --- arch/arm/mm/dma-mapping.c | 57 +++++---------------------- drivers/base/dma-mapping.c | 67 ++++++++++++++++++++++++++++++++ include/asm-generic/dma-mapping-common.h | 9 +++++ 3 files changed, 85 insertions(+), 48 deletions(-)