ARM: dma-mapping: Small logical clean up

PATCH New Hiroshi Doyu 2 participants 6 messages Aug 28, 2012 – Aug 29, 2012

Subsystems (2)

Arm (32-bit) Tegra architecture support

lore

Share & tools

lore

Hiroshi Doyu

author

2012-08-28 11:43 UTC #0 lore raw

PATCH 1/4 ARM: dma-mapping: Small logical clean up

Skip unnecessary operations if order == 0. A little bit easier to read.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mm/dma-mapping.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

arch/arm/mm/dma-mapping.c +5 -4

index 70a6275..aaea5e4 100644
@@ -1032,11 +1032,12 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size, gfp_t
 		if (!pages[i])
 			goto error;
 
-		if (order)
+		if (order) {
 			split_page(pages[i], order);
-		j = 1 << order;
-		while (--j)
-			pages[i + j] = pages[i] + j;
+			j = 1 << order;
+			while (--j)
+				pages[i + j] = pages[i] + j;
+		}
 
 		__dma_clear_buffer(pages[i], PAGE_SIZE << order);
 		i += 1 << order;

Signature · 1 line

1.7.5.4

Hiroshi Doyu

author

2012-08-28 11:43 UTC #1 lore raw

PATCH 2/4 ARM: dma-mapping: Remove unsed var at arm_coherent_iommu_unmap_page

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mm/dma-mapping.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

arch/arm/mm/dma-mapping.c +0 -1

index aaea5e4..8cb9542 100644
@@ -1557,7 +1557,6 @@ static void arm_coherent_iommu_unmap_page(struct device *dev, dma_addr_t handle,
 {
 	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
 	dma_addr_t iova = handle & PAGE_MASK;
-	struct page *page = phys_to_page(iommu_iova_to_phys(mapping->domain, iova));
 	int offset = handle & ~PAGE_MASK;
 	int len = PAGE_ALIGN(size + offset);

Signature · 1 line

1.7.5.4

Hiroshi Doyu

author

2012-08-28 11:43 UTC #2 lore raw

PATCH 3/4 ARM: dma-mapping: Refrain noisy console message

In reply to Hiroshi Doyu

With many IOMMU'able devices, console gets noisy.

Tegra30 has a few dozen of IOMMU'able devices.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mm/dma-mapping.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mm/dma-mapping.c +1 -1

index 8cb9542..2621282 100644
@@ -1747,7 +1747,7 @@ int arm_iommu_attach_device(struct device *dev,
 	dev->archdata.mapping = mapping;
 	set_dma_ops(dev, &iommu_ops);
 
-	pr_info("Attached IOMMU controller to %s device.\n", dev_name(dev));
+	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
 	return 0;
 }

Signature · 1 line

1.7.5.4

Hiroshi Doyu

author

2012-08-28 11:43 UTC #3 lore raw

PATCH 4/4 ARM: dma-mapping: Skip cache maint for invalid page

In reply to Hiroshi Doyu

Skip cache maint for invalid page but warn it.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mm/dma-mapping.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

arch/arm/mm/dma-mapping.c +9 -0

index 2621282..0368702 100644
@@ -1589,6 +1589,9 @@ static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle,
 	if (!iova)
 		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
 	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
 		__dma_page_dev_to_cpu(page, offset, size, dir);

@@ -1607,6 +1610,9 @@ static void arm_iommu_sync_single_for_cpu(struct device *dev,
 	if (!iova)
 		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
 	__dma_page_dev_to_cpu(page, offset, size, dir);
 }

@@ -1621,6 +1627,9 @@ static void arm_iommu_sync_single_for_device(struct device *dev,
 	if (!iova)
 		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
 	__dma_page_cpu_to_dev(page, offset, size, dir);
 }

Signature · 1 line

1.7.5.4

Marek Szyprowski

2012-08-28 21:58 UTC #4 lore raw

PATCH 4/4 ARM: dma-mapping: Skip cache maint for invalid page

Hello,

Quoted 32 lines · Hiroshi Doyu

Skip cache maint for invalid page but warn it.

Signed-off-by Hiroshi Doyu <[email protected]>

---
  arch/arm/mm/dma-mapping.c |    9 +++++++++
  1 files changed, 9 insertions(+), 0 deletions(-)

arch/arm/mm/dma-mapping.c +6 -0

index 2621282..0368702 100644
@@ -1589,6 +1589,9 @@ static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle,
  	if (!iova)
  		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
  	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
  		__dma_page_dev_to_cpu(page, offset, size, dir);

@@ -1607,6 +1610,9 @@ static void arm_iommu_sync_single_for_cpu(struct device *dev,
  	if (!iova)
  		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
  	__dma_page_dev_to_cpu(page, offset, size, dir);
  }

arch/arm/mm/dma-mapping.c static void arm_iommu_sync_single_for_device(struct device *dev, · line 1634

Quoted 8 lines

arch/arm/mm/dma-mapping.c +3 -0

@@ -1621,6 +1627,9 @@ static void arm_iommu_sync_single_for_device(struct device *dev,
  	if (!iova)
  		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
  	__dma_page_cpu_to_dev(page, offset, size, dir);
  }

I really wonder what has happened in your system that you require such paranoid checks. All of the above functions are called on the scatter-list which has been first passed to dma_map_sg(). IMHO it makes much more sense to add a pfn_valid() check in dma_map_sg() and return error if the provided scatter list is not valid instead of these WARN_ONs.

Best regards

Signature · 2 lines

Marek Szyprowski
Samsung Poland R&D Center

Hiroshi Doyu

author

2012-08-29 04:49 UTC #5 lore raw

PATCH 4/4 ARM: dma-mapping: Skip cache maint for invalid page

Hi Marek,

Marek Szyprowski <[email protected]> wrote @ Tue, 28 Aug 2012 23:58:48 +0200:

Quoted 39 lines

Hello,

Quoted 35 lines · Hiroshi Doyu

Skip cache maint for invalid page but warn it.

Signed-off-by Hiroshi Doyu <[email protected]>

---
  arch/arm/mm/dma-mapping.c |    9 +++++++++
  1 files changed, 9 insertions(+), 0 deletions(-)

arch/arm/mm/dma-mapping.c +6 -0

index 2621282..0368702 100644
@@ -1589,6 +1589,9 @@ static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle,
  	if (!iova)
  		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
  	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
  		__dma_page_dev_to_cpu(page, offset, size, dir);

@@ -1607,6 +1610,9 @@ static void arm_iommu_sync_single_for_cpu(struct device *dev,
  	if (!iova)
  		return;
 
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
  	__dma_page_dev_to_cpu(page, offset, size, dir);
  }

@@ -1621,6 +1627,9 @@ static void arm_iommu_sync_single_for_device(struct device *dev,
  	if (!iova)
  		return;

Quoted 12 lines

arch/arm/mm/dma-mapping.c static void arm_iommu_sync_single_for_device(struct device *dev, · line 1634
Quoted 5 lines
arch/arm/mm/dma-mapping.c +3 -0
+	if (WARN_ON(!pfn_valid(page_to_pfn(page))))
+		return;
+
  	__dma_page_cpu_to_dev(page, offset, size, dir);
  }
I really wonder what has happened in your system that you require such paranoid checks. All of the above functions are called on the scatter-list which has been first passed to dma_map_sg(). IMHO it makes much more sense to add a pfn_valid() check in dma_map_sg() and return error if the provided scatter list is not valid instead of these WARN_ONs.

Actually one device driver hit Oops at unmap_page() since this function could be called directly from device drivers, and WARN_ON(dump_stack) helped to find out which device driver did something wrong. IIRC, the following functions can be called directly from device drivers?

static inline void dma_unmap_page(struct device *dev, dma_addr_t addr,

				  size_t size, enum dma_data_direction dir);
static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
					   size_t size,
					   enum dma_data_direction dir);
static inline void dma_sync_single_for_device(struct device *dev,
					      dma_addr_t addr, size_t size,
					      enum dma_data_direction dir);