Re: [PATCH 12/21] dma-iommu: factor atomic pool allocations into helpers
From: Christoph Hellwig
Date: Fri Apr 19 2019 - 15:00:10 EST
On Thu, Apr 18, 2019 at 07:15:00PM +0100, Robin Murphy wrote:
> Still, I've worked in the vm_map_pages() stuff pending in MM and given them
> the same treatment to finish the picture. Both x86_64_defconfig and
> i386_defconfig do indeed compile and link fine as I expected, so I really
> would like to understand the concern around #ifdefs better.
This looks generally fine to me. One thing I'd like to do is to
generally make use of the fact that __iommu_dma_get_pages returns NULL
for the force contigous case as that cleans up a few things. Also
for the !DMA_REMAP case we need to try the page allocator when
dma_alloc_from_contiguous does not return a page. What do you thing
of the following incremental diff? If that is fine with you I can
fold that in and add back in the remaining patches from my series
not obsoleted by your patches and resend.
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1bc8d1de1a1d..50b44e220de3 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -894,7 +894,7 @@ static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
static void __iommu_dma_free(struct device *dev, void *cpu_addr, size_t size)
{
- struct page *page, **pages;
+ struct page *page = NULL;
int count = size >> PAGE_SHIFT;
/* Non-coherent atomic allocation? Easy */
@@ -902,24 +902,26 @@ static void __iommu_dma_free(struct device *dev, void *cpu_addr, size_t size)
dma_free_from_pool(cpu_addr, size))
return;
- /* Lowmem means a coherent atomic or CMA allocation */
- if (!IS_ENABLED(CONFIG_DMA_REMAP) || !is_vmalloc_addr(cpu_addr)) {
- page = virt_to_page(cpu_addr);
- if (!dma_release_from_contiguous(dev, page, count))
- __free_pages(page, get_order(size));
- return;
- }
- /*
- * If it's remapped, then it's either non-coherent or highmem CMA, or
- * an iommu_dma_alloc_remap() construction.
- */
- page = vmalloc_to_page(cpu_addr);
- if (!dma_release_from_contiguous(dev, page, count)) {
- pages = __iommu_dma_get_pages(cpu_addr);
+ if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && is_vmalloc_addr(cpu_addr)) {
+ /*
+ * If it the address is remapped, then it's either non-coherent
+ * or highmem CMA, or an iommu_dma_alloc_remap() construction.
+ */
+ struct page **pages = __iommu_dma_get_pages(cpu_addr);
+
if (pages)
__iommu_dma_free_pages(pages, count);
+ else
+ page = vmalloc_to_page(cpu_addr);
+
+ dma_common_free_remap(cpu_addr, size, VM_USERMAP);
+ } else {
+ /* Lowmem means a coherent atomic or CMA allocation */
+ page = virt_to_page(cpu_addr);
}
- dma_common_free_remap(cpu_addr, size, VM_USERMAP);
+
+ if (page && !dma_release_from_contiguous(dev, page, count))
+ __free_pages(page, get_order(size));
}
static void *iommu_dma_alloc(struct device *dev, size_t size,
@@ -935,25 +937,26 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
gfp |= __GFP_ZERO;
+ if (IS_ENABLED(CONFIG_DMA_REMAP) && gfpflags_allow_blocking(gfp) &&
+ !(attrs & DMA_ATTR_FORCE_CONTIGUOUS))
+ return iommu_dma_alloc_remap(dev, size, handle, gfp, attrs);
+
if (!gfpflags_allow_blocking(gfp)) {
- if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && !coherent)
+ if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && !coherent) {
cpu_addr = dma_alloc_from_pool(alloc_size, &page, gfp);
- else
- page = alloc_pages(gfp, page_order);
- } else if (!IS_ENABLED(CONFIG_DMA_REMAP) ||
- (attrs & DMA_ATTR_FORCE_CONTIGUOUS)) {
+ if (!cpu_addr)
+ return NULL;
+ goto do_iommu_map;
+ }
+ } else {
page = dma_alloc_from_contiguous(dev, count, page_order,
gfp & __GFP_NOWARN);
- } else {
- return iommu_dma_alloc_remap(dev, size, handle, gfp, attrs);
}
-
+ if (!page)
+ page = alloc_pages(gfp, page_order);
if (!page)
return NULL;
- if (cpu_addr)
- goto do_iommu_map;
-
if (IS_ENABLED(CONFIG_DMA_REMAP) && (!coherent || PageHighMem(page))) {
pgprot_t prot = arch_dma_mmap_pgprot(dev, PAGE_KERNEL, attrs);
@@ -1007,16 +1010,14 @@ static int iommu_dma_mmap(struct device *dev, struct vm_area_struct *vma,
if (off >= nr_pages || vma_pages(vma) > nr_pages - off)
return -ENXIO;
- if (!is_vmalloc_addr(cpu_addr)) {
- pfn = page_to_pfn(virt_to_page(cpu_addr));
- } else if (!IS_ENABLED(CONFIG_DMA_REMAP) ||
- (attrs & DMA_ATTR_FORCE_CONTIGUOUS)) {
+ if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr)) {
+ struct page **pages = __iommu_dma_get_pages(cpu_addr);
+
+ if (pages)
+ return vm_map_pages(vma, pages, nr_pages);
pfn = vmalloc_to_pfn(cpu_addr);
} else {
- struct page **pages = __iommu_dma_get_pages(cpu_addr);
- if (!pages)
- return -ENXIO;
- return vm_map_pages(vma, pages, nr_pages);
+ pfn = page_to_pfn(virt_to_page(cpu_addr));
}
return remap_pfn_range(vma, vma->vm_start, pfn + off,
@@ -1028,26 +1029,25 @@ static int iommu_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
unsigned long attrs)
{
- struct page *page = NULL, **pages = NULL;
- int ret = -ENXIO;
+ struct page *page;
+ int ret;
- if (!is_vmalloc_addr(cpu_addr))
- page = virt_to_page(cpu_addr);
- else if (!IS_ENABLED(CONFIG_DMA_REMAP) ||
- (attrs & DMA_ATTR_FORCE_CONTIGUOUS))
+ if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr)) {
+ struct page **pages = __iommu_dma_get_pages(cpu_addr);
+
+ if (pages)
+ return sg_alloc_table_from_pages(sgt,
+ __iommu_dma_get_pages(cpu_addr),
+ PAGE_ALIGN(size) >> PAGE_SHIFT, 0, size,
+ GFP_KERNEL);
page = vmalloc_to_page(cpu_addr);
- else
- pages = __iommu_dma_get_pages(cpu_addr);
-
- if (page) {
- ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
- if (!ret)
- sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
- } else if (pages) {
- ret = sg_alloc_table_from_pages(sgt, pages,
- PAGE_ALIGN(size) >> PAGE_SHIFT,
- 0, size, GFP_KERNEL);
+ } else {
+ page = virt_to_page(cpu_addr);
}
+
+ ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
+ if (!ret)
+ sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
return ret;
}