Re: [PATCH 12/21] dma-iommu: factor atomic pool allocations into helpers

[Christoph Hellwig]

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;
 }