[PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Joerg Roedel]

The SWIOTLB version of dma_alloc_coherent allocates all memory with
GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
failures. This patch makes the allocation strategy to use the DMA32 zone
first if this is possible. The changes are boot tested on AMD64 and
compile tested for i386 and IA64.

Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: tony.luck@intel.com
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 lib/swiotlb.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 977edbd..6ba077f 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -466,13 +466,24 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	dma_addr_t dev_addr;
 	void *ret;
 	int order = get_order(size);
+	unsigned long dma_mask;
 
-	/*
-	 * XXX fix me: the DMA API should pass us an explicit DMA mask
-	 * instead, or use ZONE_DMA32 (ia64 overloads ZONE_DMA to be a ~32
-	 * bit range instead of a 16MB one).
-	 */
-	flags |= GFP_DMA;
+	if (hwdev->dma_mask == NULL)
+		return NULL;
+
+	flags &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
+	flags |= __GFP_ZERO;
+
+	dma_mask = hwdev->coherent_dma_mask;
+	if (!dma_mask)
+		dma_mask = *(hwdev->dma_mask);
+
+	if (dma_mask <= ISA_DMA_THRESHOLD)
+		flags |= GFP_DMA;
+#ifdef CONFIG_X86_64
+	else if (dma_mask <= DMA_32BIT_MASK)
+		flags |= GFP_DMA32;
+#endif
 
 	ret = (void *)__get_free_pages(flags, order);
 	if (ret && address_needs_mapping(hwdev, virt_to_bus(ret))) {
-- 
1.5.3.7

[PATCH 0/3] fix swiotlb allocation gfp flag

[FUJITA Tomonori]

This patchset fixes a problem (in the x86 tree) that swiotlb always
use GFP_DMA so we exhaust ZONE_DMA.

This is against tip/master.

[PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

[FUJITA Tomonori]

dma_alloc_coherent in dma-mapping.h has a hack to use
x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
don't need such hack. The hack also makes it difficult for IOMMUs to
make a proper decision because the hack hides the information.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-swiotlb_64.c |   15 ++++++++++++++-
 include/asm-x86/dma-mapping.h    |    9 +--------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/pci-swiotlb_64.c b/arch/x86/kernel/pci-swiotlb_64.c
index c4ce033..f3d8d0e 100644
--- a/arch/x86/kernel/pci-swiotlb_64.c
+++ b/arch/x86/kernel/pci-swiotlb_64.c
@@ -11,6 +11,8 @@
 
 int swiotlb __read_mostly;
 
+extern struct device x86_dma_fallback_dev;
+
 static dma_addr_t
 swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
 			int direction)
@@ -18,9 +20,20 @@ swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
 	return swiotlb_map_single(hwdev, phys_to_virt(paddr), size, direction);
 }
 
+static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
+					dma_addr_t *dma_handle, gfp_t gfp)
+{
+	if (!dev) {
+		dev = &x86_dma_fallback_dev;
+		gfp |= GFP_DMA;
+	}
+
+	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
+}
+
 struct dma_mapping_ops swiotlb_dma_ops = {
 	.mapping_error = swiotlb_dma_mapping_error,
-	.alloc_coherent = swiotlb_alloc_coherent,
+	.alloc_coherent = x86_swiotlb_alloc_coherent,
 	.free_coherent = swiotlb_free_coherent,
 	.map_single = swiotlb_map_single_phys,
 	.unmap_single = swiotlb_unmap_single,
diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
index bc6c8df..3463702 100644
--- a/include/asm-x86/dma-mapping.h
+++ b/include/asm-x86/dma-mapping.h
@@ -14,7 +14,6 @@
 
 extern dma_addr_t bad_dma_address;
 extern int iommu_merge;
-extern struct device x86_dma_fallback_dev;
 extern int panic_on_overflow;
 extern int force_iommu;
 
@@ -251,14 +250,8 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
 	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
 		return memory;
 
-	if (!dev) {
-		dev = &x86_dma_fallback_dev;
-		gfp |= GFP_DMA;
-	}
-
 	if (ops->alloc_coherent)
-		return ops->alloc_coherent(dev, size,
-				dma_handle, gfp);
+		return ops->alloc_coherent(dev, size, dma_handle, gfp);
 	return NULL;
 }
 
-- 
1.5.5.GIT

[PATCH 2/3] x86: set gfp flag properly for swiotlb_alloc_coherent

[FUJITA Tomonori]

This tries to avoid GFP_DMA zone unless absolutely needed (and set gfp
flag to use GFP_DMA zone properly if necessary).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-swiotlb_64.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/pci-swiotlb_64.c b/arch/x86/kernel/pci-swiotlb_64.c
index f3d8d0e..3f4d9b4 100644
--- a/arch/x86/kernel/pci-swiotlb_64.c
+++ b/arch/x86/kernel/pci-swiotlb_64.c
@@ -23,11 +23,29 @@ swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
 static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
 					dma_addr_t *dma_handle, gfp_t gfp)
 {
+	unsigned long dma_mask;
+
+	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
+
 	if (!dev) {
 		dev = &x86_dma_fallback_dev;
 		gfp |= GFP_DMA;
 	}
 
+	if (!dev->dma_mask)
+		return NULL;
+
+	dma_mask = dev->coherent_dma_mask;
+	if (!dma_mask)
+		dma_mask = gfp & GFP_DMA ? DMA_24BIT_MASK : DMA_32BIT_MASK;
+
+	if (!(gfp & GFP_DMA)) {
+		if (dma_mask <= DMA_24BIT_MASK)
+			gfp |= GFP_DMA;
+		else if (dma_mask <= DMA_32BIT_MASK)
+			gfp |= GFP_DMA32;
+	}
+
 	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
 }
 
-- 
1.5.5.GIT

[PATCH 3/3] swiotlb: use GFP_DMA32 instead of GFP_DMA in swiotlb_alloc_coherent

[FUJITA Tomonori]

x86 sets up gfp flag propely so swiotlb_alloc_coherent doesn't need to
use GFP_DMA unconditionally. It leads to allocation failures in some
systems.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 lib/swiotlb.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 977edbd..b88d7c2 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -467,12 +467,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	void *ret;
 	int order = get_order(size);
 
-	/*
-	 * XXX fix me: the DMA API should pass us an explicit DMA mask
-	 * instead, or use ZONE_DMA32 (ia64 overloads ZONE_DMA to be a ~32
-	 * bit range instead of a 16MB one).
-	 */
-	flags |= GFP_DMA;
+	flags |= GFP_DMA32;
 
 	ret = (void *)__get_free_pages(flags, order);
 	if (ret && address_needs_mapping(hwdev, virt_to_bus(ret))) {
-- 
1.5.5.GIT

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[FUJITA Tomonori]

On Wed, 3 Sep 2008 17:03:44 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> The SWIOTLB version of dma_alloc_coherent allocates all memory with
> GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> failures. This patch makes the allocation strategy to use the DMA32 zone
> first if this is possible. The changes are boot tested on AMD64 and
> compile tested for i386 and IA64.
> 
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: tony.luck@intel.com
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  lib/swiotlb.c |   23 +++++++++++++++++------
>  1 files changed, 17 insertions(+), 6 deletions(-)

This doesn't look correct since IA64 doesn't need this flag hack.

Another problem about this patch is that swiotlb doesn't use GFP_DMA
for the fallback_dev.

I'll send patches to fix this problem differently.

Re: [PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

[FUJITA Tomonori]

On Thu,  4 Sep 2008 03:04:23 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> dma_alloc_coherent in dma-mapping.h has a hack to use
> x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
> don't need such hack. The hack also makes it difficult for IOMMUs to
> make a proper decision because the hack hides the information.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  arch/x86/kernel/pci-swiotlb_64.c |   15 ++++++++++++++-
>  include/asm-x86/dma-mapping.h    |    9 +--------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-swiotlb_64.c b/arch/x86/kernel/pci-swiotlb_64.c
> index c4ce033..f3d8d0e 100644
> --- a/arch/x86/kernel/pci-swiotlb_64.c
> +++ b/arch/x86/kernel/pci-swiotlb_64.c
> @@ -11,6 +11,8 @@
>  
>  int swiotlb __read_mostly;
>  
> +extern struct device x86_dma_fallback_dev;
> +
>  static dma_addr_t
>  swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
>  			int direction)
> @@ -18,9 +20,20 @@ swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
>  	return swiotlb_map_single(hwdev, phys_to_virt(paddr), size, direction);
>  }
>  
> +static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
> +					dma_addr_t *dma_handle, gfp_t gfp)
> +{
> +	if (!dev) {
> +		dev = &x86_dma_fallback_dev;
> +		gfp |= GFP_DMA;
> +	}
> +
> +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> +}
> +
>  struct dma_mapping_ops swiotlb_dma_ops = {
>  	.mapping_error = swiotlb_dma_mapping_error,
> -	.alloc_coherent = swiotlb_alloc_coherent,
> +	.alloc_coherent = x86_swiotlb_alloc_coherent,
>  	.free_coherent = swiotlb_free_coherent,
>  	.map_single = swiotlb_map_single_phys,
>  	.unmap_single = swiotlb_unmap_single,
> diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
> index bc6c8df..3463702 100644
> --- a/include/asm-x86/dma-mapping.h
> +++ b/include/asm-x86/dma-mapping.h
> @@ -14,7 +14,6 @@
>  
>  extern dma_addr_t bad_dma_address;
>  extern int iommu_merge;
> -extern struct device x86_dma_fallback_dev;

Oops, removing extern struct device x86_dma_fallback_dev breaks the
GART IOMMU. Here's an updated patch.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] x86: remove the NULL device hack in dma-mapping.h

dma_alloc_coherent in dma-mapping.h has a hack to use
x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
don't need such hack. The hack also makes it difficult for IOMMUs to
make a proper decision because the hack hides the information.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-swiotlb_64.c |   13 ++++++++++++-
 include/asm-x86/dma-mapping.h    |    8 +-------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/pci-swiotlb_64.c b/arch/x86/kernel/pci-swiotlb_64.c
index c4ce033..d2463de 100644
--- a/arch/x86/kernel/pci-swiotlb_64.c
+++ b/arch/x86/kernel/pci-swiotlb_64.c
@@ -18,9 +18,20 @@ swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
 	return swiotlb_map_single(hwdev, phys_to_virt(paddr), size, direction);
 }
 
+static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
+					dma_addr_t *dma_handle, gfp_t gfp)
+{
+	if (!dev) {
+		dev = &x86_dma_fallback_dev;
+		gfp |= GFP_DMA;
+	}
+
+	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
+}
+
 struct dma_mapping_ops swiotlb_dma_ops = {
 	.mapping_error = swiotlb_dma_mapping_error,
-	.alloc_coherent = swiotlb_alloc_coherent,
+	.alloc_coherent = x86_swiotlb_alloc_coherent,
 	.free_coherent = swiotlb_free_coherent,
 	.map_single = swiotlb_map_single_phys,
 	.unmap_single = swiotlb_unmap_single,
diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
index bc6c8df..e59607d 100644
--- a/include/asm-x86/dma-mapping.h
+++ b/include/asm-x86/dma-mapping.h
@@ -251,14 +251,8 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
 	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
 		return memory;
 
-	if (!dev) {
-		dev = &x86_dma_fallback_dev;
-		gfp |= GFP_DMA;
-	}
-
 	if (ops->alloc_coherent)
-		return ops->alloc_coherent(dev, size,
-				dma_handle, gfp);
+		return ops->alloc_coherent(dev, size, dma_handle, gfp);
 	return NULL;
 }
 
-- 
1.5.5.GIT

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Joerg Roedel]

On Thu, Sep 04, 2008 at 03:05:21AM +0900, FUJITA Tomonori wrote:
> On Wed, 3 Sep 2008 17:03:44 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > failures. This patch makes the allocation strategy to use the DMA32 zone
> > first if this is possible. The changes are boot tested on AMD64 and
> > compile tested for i386 and IA64.
> > 
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Cc: tony.luck@intel.com
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  lib/swiotlb.c |   23 +++++++++++++++++------
> >  1 files changed, 17 insertions(+), 6 deletions(-)
> 
> This doesn't look correct since IA64 doesn't need this flag hack.

Thats why the patch checks the DMA mask against ISA_DMA_THRESHOLD. This
is defined as the border of the ZONE_DMA (16MB on x86 and 4G on IA64).
So is correct for x86 and ia64.

> Another problem about this patch is that swiotlb doesn't use GFP_DMA
> for the fallback_dev.

Thats not true, it does. ISA_DMA_THRESHOLD is defined to be the same as
DMA_24BIT_MASK, which is the DMA mask of the fallback_dev.

Joerg

Re: [PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

[Joerg Roedel]

On Thu, Sep 04, 2008 at 03:04:23AM +0900, FUJITA Tomonori wrote:
> dma_alloc_coherent in dma-mapping.h has a hack to use
> x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
> don't need such hack. The hack also makes it difficult for IOMMUs to
> make a proper decision because the hack hides the information.

I don't think its the right way to work around shortcomings of the
generic code in the architecture specific implementations. Especially
when the generic code can be easily fixed like in this case.

> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  arch/x86/kernel/pci-swiotlb_64.c |   15 ++++++++++++++-
>  include/asm-x86/dma-mapping.h    |    9 +--------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-swiotlb_64.c b/arch/x86/kernel/pci-swiotlb_64.c
> index c4ce033..f3d8d0e 100644
> --- a/arch/x86/kernel/pci-swiotlb_64.c
> +++ b/arch/x86/kernel/pci-swiotlb_64.c
> @@ -11,6 +11,8 @@
>  
>  int swiotlb __read_mostly;
>  
> +extern struct device x86_dma_fallback_dev;
> +
>  static dma_addr_t
>  swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
>  			int direction)
> @@ -18,9 +20,20 @@ swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
>  	return swiotlb_map_single(hwdev, phys_to_virt(paddr), size, direction);
>  }
>  
> +static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
> +					dma_addr_t *dma_handle, gfp_t gfp)
> +{
> +	if (!dev) {
> +		dev = &x86_dma_fallback_dev;
> +		gfp |= GFP_DMA;
> +	}

This really should be checked in the generic x86 dma_alloc_coherent
function.

> +
> +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> +}
> +
>  struct dma_mapping_ops swiotlb_dma_ops = {
>  	.mapping_error = swiotlb_dma_mapping_error,
> -	.alloc_coherent = swiotlb_alloc_coherent,
> +	.alloc_coherent = x86_swiotlb_alloc_coherent,
>  	.free_coherent = swiotlb_free_coherent,
>  	.map_single = swiotlb_map_single_phys,
>  	.unmap_single = swiotlb_unmap_single,
> diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
> index bc6c8df..3463702 100644
> --- a/include/asm-x86/dma-mapping.h
> +++ b/include/asm-x86/dma-mapping.h
> @@ -14,7 +14,6 @@
>  
>  extern dma_addr_t bad_dma_address;
>  extern int iommu_merge;
> -extern struct device x86_dma_fallback_dev;
>  extern int panic_on_overflow;
>  extern int force_iommu;
>  
> @@ -251,14 +250,8 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
>  	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
>  		return memory;
>  
> -	if (!dev) {
> -		dev = &x86_dma_fallback_dev;
> -		gfp |= GFP_DMA;
> -	}

Why do you move this check to swiotlb implemenation? This will break
existing IOMMU implementations which don't check for a valid dev
pointer?

>  	if (ops->alloc_coherent)
> -		return ops->alloc_coherent(dev, size,
> -				dma_handle, gfp);
> +		return ops->alloc_coherent(dev, size, dma_handle, gfp);
>  	return NULL;
>  }
>  
> -- 
> 1.5.5.GIT
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/iommu

Re: [PATCH 3/3] swiotlb: use GFP_DMA32 instead of GFP_DMA in swiotlb_alloc_coherent

[Joerg Roedel]

On Thu, Sep 04, 2008 at 03:04:25AM +0900, FUJITA Tomonori wrote:
> x86 sets up gfp flag propely so swiotlb_alloc_coherent doesn't need to
> use GFP_DMA unconditionally. It leads to allocation failures in some
> systems.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  lib/swiotlb.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 977edbd..b88d7c2 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -467,12 +467,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>  	void *ret;
>  	int order = get_order(size);
>  
> -	/*
> -	 * XXX fix me: the DMA API should pass us an explicit DMA mask
> -	 * instead, or use ZONE_DMA32 (ia64 overloads ZONE_DMA to be a ~32
> -	 * bit range instead of a 16MB one).
> -	 */
> -	flags |= GFP_DMA;
> +	flags |= GFP_DMA32;

No. This is exactly the place where the swiotlb iommu implementation
should handle the dma_mask of the specific device. Unconditionally use
DMA32 here is not the best possible handling.

Joerg

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Joerg Roedel]

On Wed, Sep 03, 2008 at 09:54:15PM +0200, Joerg Roedel wrote:
> On Thu, Sep 04, 2008 at 03:05:21AM +0900, FUJITA Tomonori wrote:
> > On Wed, 3 Sep 2008 17:03:44 +0200
> > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > 
> > > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > > failures. This patch makes the allocation strategy to use the DMA32 zone
> > > first if this is possible. The changes are boot tested on AMD64 and
> > > compile tested for i386 and IA64.
> > > 
> > > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > Cc: tony.luck@intel.com
> > > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > > ---
> > >  lib/swiotlb.c |   23 +++++++++++++++++------
> > >  1 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > This doesn't look correct since IA64 doesn't need this flag hack.
> 
> Thats why the patch checks the DMA mask against ISA_DMA_THRESHOLD. This
> is defined as the border of the ZONE_DMA (16MB on x86 and 4G on IA64).
> So is correct for x86 and ia64.
> 
> > Another problem about this patch is that swiotlb doesn't use GFP_DMA
> > for the fallback_dev.
> 
> Thats not true, it does. ISA_DMA_THRESHOLD is defined to be the same as
> DMA_24BIT_MASK, which is the DMA mask of the fallback_dev.

(on x86, which defines the fallback_dev)

> 
> Joerg
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/iommu

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Andi Kleen]

Joerg Roedel <joerg.roedel@amd.com> writes:

> The SWIOTLB version of dma_alloc_coherent allocates all memory with
> GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> failures. This patch makes the allocation strategy to use the DMA32 zone
> first if this is possible. The changes are boot tested on AMD64 and
> compile tested for i386 and IA64.

The high level dma_alloc_coherent() does that anyways.

The swiotlb fallback really should only allocate from the swiotlb,
nowhere else.

-Andi

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[FUJITA Tomonori]

On Wed, 3 Sep 2008 21:54:15 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> On Thu, Sep 04, 2008 at 03:05:21AM +0900, FUJITA Tomonori wrote:
> > On Wed, 3 Sep 2008 17:03:44 +0200
> > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > 
> > > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > > failures. This patch makes the allocation strategy to use the DMA32 zone
> > > first if this is possible. The changes are boot tested on AMD64 and
> > > compile tested for i386 and IA64.
> > > 
> > > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > Cc: tony.luck@intel.com
> > > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > > ---
> > >  lib/swiotlb.c |   23 +++++++++++++++++------
> > >  1 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > This doesn't look correct since IA64 doesn't need this flag hack.
> 
> Thats why the patch checks the DMA mask against ISA_DMA_THRESHOLD. This
> is defined as the border of the ZONE_DMA (16MB on x86 and 4G on IA64).
> So is correct for x86 and ia64.

You try to bring back the tricks used in arch/x86/kernel/pci-dma.c to
lib/swiotlb.c. I believe that they should be in
arch/x86/kernel/pci-swiotlb.c.


> > Another problem about this patch is that swiotlb doesn't use GFP_DMA
> > for the fallback_dev.
> 
> Thats not true, it does. ISA_DMA_THRESHOLD is defined to be the same as
> DMA_24BIT_MASK, which is the DMA mask of the fallback_dev.

I don't think so. device x86_dma_fallback_dev is defined as (in
tip/master):

struct device x86_dma_fallback_dev = {
       .bus_id = "fallback device",
       .coherent_dma_mask = DMA_32BIT_MASK,
       .dma_mask = &x86_dma_fallback_dev.coherent_dma_mask,
};

Re: [PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

[FUJITA Tomonori]

On Wed, 3 Sep 2008 22:01:14 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> On Thu, Sep 04, 2008 at 03:04:23AM +0900, FUJITA Tomonori wrote:
> > dma_alloc_coherent in dma-mapping.h has a hack to use
> > x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
> > don't need such hack. The hack also makes it difficult for IOMMUs to
> > make a proper decision because the hack hides the information.
> 
> I don't think its the right way to work around shortcomings of the
> generic code in the architecture specific implementations. Especially
> when the generic code can be easily fixed like in this case.

Well, the generic code should not have such work around.

As I wrote in another mail, you try to bring back the tricks used in
arch/x86/kernel/pci-dma.c to lib/swiotlb.c. I try to bring back them
to arch/x86/kernel/pci-swiotlb.c.


> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  arch/x86/kernel/pci-swiotlb_64.c |   15 ++++++++++++++-
> >  include/asm-x86/dma-mapping.h    |    9 +--------
> >  2 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/pci-swiotlb_64.c b/arch/x86/kernel/pci-swiotlb_64.c
> > index c4ce033..f3d8d0e 100644
> > --- a/arch/x86/kernel/pci-swiotlb_64.c
> > +++ b/arch/x86/kernel/pci-swiotlb_64.c
> > @@ -11,6 +11,8 @@
> >  
> >  int swiotlb __read_mostly;
> >  
> > +extern struct device x86_dma_fallback_dev;
> > +
> >  static dma_addr_t
> >  swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
> >  			int direction)
> > @@ -18,9 +20,20 @@ swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
> >  	return swiotlb_map_single(hwdev, phys_to_virt(paddr), size, direction);
> >  }
> >  
> > +static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > +					dma_addr_t *dma_handle, gfp_t gfp)
> > +{
> > +	if (!dev) {
> > +		dev = &x86_dma_fallback_dev;
> > +		gfp |= GFP_DMA;
> > +	}
> 
> This really should be checked in the generic x86 dma_alloc_coherent
> function.

I don't think so. Any motherboards with the recent IOMMUs support ISA?


> > +
> > +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> > +}
> > +
> >  struct dma_mapping_ops swiotlb_dma_ops = {
> >  	.mapping_error = swiotlb_dma_mapping_error,
> > -	.alloc_coherent = swiotlb_alloc_coherent,
> > +	.alloc_coherent = x86_swiotlb_alloc_coherent,
> >  	.free_coherent = swiotlb_free_coherent,
> >  	.map_single = swiotlb_map_single_phys,
> >  	.unmap_single = swiotlb_unmap_single,
> > diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
> > index bc6c8df..3463702 100644
> > --- a/include/asm-x86/dma-mapping.h
> > +++ b/include/asm-x86/dma-mapping.h
> > @@ -14,7 +14,6 @@
> >  
> >  extern dma_addr_t bad_dma_address;
> >  extern int iommu_merge;
> > -extern struct device x86_dma_fallback_dev;
> >  extern int panic_on_overflow;
> >  extern int force_iommu;
> >  
> > @@ -251,14 +250,8 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
> >  	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
> >  		return memory;
> >  
> > -	if (!dev) {
> > -		dev = &x86_dma_fallback_dev;
> > -		gfp |= GFP_DMA;
> > -	}
> 
> Why do you move this check to swiotlb implemenation? This will break
> existing IOMMU implementations which don't check for a valid dev
> pointer?

As I wrote above, I don't think so (though I forgot to add this to
pci-nommu.c).

Re: [PATCH 3/3] swiotlb: use GFP_DMA32 instead of GFP_DMA in swiotlb_alloc_coherent

[FUJITA Tomonori]

On Wed, 3 Sep 2008 22:05:27 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> On Thu, Sep 04, 2008 at 03:04:25AM +0900, FUJITA Tomonori wrote:
> > x86 sets up gfp flag propely so swiotlb_alloc_coherent doesn't need to
> > use GFP_DMA unconditionally. It leads to allocation failures in some
> > systems.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  lib/swiotlb.c |    7 +------
> >  1 files changed, 1 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> > index 977edbd..b88d7c2 100644
> > --- a/lib/swiotlb.c
> > +++ b/lib/swiotlb.c
> > @@ -467,12 +467,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
> >  	void *ret;
> >  	int order = get_order(size);
> >  
> > -	/*
> > -	 * XXX fix me: the DMA API should pass us an explicit DMA mask
> > -	 * instead, or use ZONE_DMA32 (ia64 overloads ZONE_DMA to be a ~32
> > -	 * bit range instead of a 16MB one).
> > -	 */
> > -	flags |= GFP_DMA;
> > +	flags |= GFP_DMA32;
> 
> No. This is exactly the place where the swiotlb iommu implementation
> should handle the dma_mask of the specific device. Unconditionally use
> DMA32 here is not the best possible handling.

Not best but it works though ideally we can just remove the flags hack
completely.

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[FUJITA Tomonori]

On Thu, 04 Sep 2008 00:09:51 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> Joerg Roedel <joerg.roedel@amd.com> writes:
> 
> > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > failures. This patch makes the allocation strategy to use the DMA32 zone
> > first if this is possible. The changes are boot tested on AMD64 and
> > compile tested for i386 and IA64.
> 
> The high level dma_alloc_coherent() does that anyways.

The high level dma_alloc_coherent means the tricks in
arch/x86/pci-dma.c? If so, all the tricks has gone in tip/master.


> The swiotlb fallback really should only allocate from the swiotlb,
> nowhere else.

Agreed.

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Andi Kleen]

On Thu, Sep 04, 2008 at 01:11:47PM +0900, FUJITA Tomonori wrote:
> On Thu, 04 Sep 2008 00:09:51 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > Joerg Roedel <joerg.roedel@amd.com> writes:
> > 
> > > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > > failures. This patch makes the allocation strategy to use the DMA32 zone
> > > first if this is possible. The changes are boot tested on AMD64 and
> > > compile tested for i386 and IA64.
> > 
> > The high level dma_alloc_coherent() does that anyways.
> 
> The high level dma_alloc_coherent means the tricks in
> arch/x86/pci-dma.c? If so, all the tricks has gone in tip/master.

Seems wrong -- that is something that is needed by multiple
IOMMUs. Pretty much all which work with real memory instead
of virtual mappings.

I also have an older swiotlb rewrite in the pipeline based on the DMA
allocator. The project got a little sidelined, but I hope to eventually 
forward port it again.

-Andi

-- 
ak@linux.intel.com

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[FUJITA Tomonori]

On Thu, 4 Sep 2008 09:44:41 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> On Thu, Sep 04, 2008 at 01:11:47PM +0900, FUJITA Tomonori wrote:
> > On Thu, 04 Sep 2008 00:09:51 +0200
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > Joerg Roedel <joerg.roedel@amd.com> writes:
> > > 
> > > > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > > > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > > > failures. This patch makes the allocation strategy to use the DMA32 zone
> > > > first if this is possible. The changes are boot tested on AMD64 and
> > > > compile tested for i386 and IA64.
> > > 
> > > The high level dma_alloc_coherent() does that anyways.
> > 
> > The high level dma_alloc_coherent means the tricks in
> > arch/x86/pci-dma.c? If so, all the tricks has gone in tip/master.
> 
> Seems wrong -- that is something that is needed by multiple
> IOMMUs. Pretty much all which work with real memory instead
> of virtual mappings.

Not all the IOMMUs need the old tricks used in arch/x86/pci-dma.c.

The latest IOMMUs always need virtual mappings. They should handle
dev->dma_mask properly like POWER IOMMU does (IIRC VT-d code does the
right thing).

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Andi Kleen]

On Thu, Sep 04, 2008 at 04:58:37PM +0900, FUJITA Tomonori wrote:
> On Thu, 4 Sep 2008 09:44:41 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > On Thu, Sep 04, 2008 at 01:11:47PM +0900, FUJITA Tomonori wrote:
> > > On Thu, 04 Sep 2008 00:09:51 +0200
> > > Andi Kleen <andi@firstfloor.org> wrote:
> > > 
> > > > Joerg Roedel <joerg.roedel@amd.com> writes:
> > > > 
> > > > > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > > > > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > > > > failures. This patch makes the allocation strategy to use the DMA32 zone
> > > > > first if this is possible. The changes are boot tested on AMD64 and
> > > > > compile tested for i386 and IA64.
> > > > 
> > > > The high level dma_alloc_coherent() does that anyways.
> > > 
> > > The high level dma_alloc_coherent means the tricks in
> > > arch/x86/pci-dma.c? If so, all the tricks has gone in tip/master.
> > 
> > Seems wrong -- that is something that is needed by multiple
> > IOMMUs. Pretty much all which work with real memory instead
> > of virtual mappings.
> 
> Not all the IOMMUs need the old tricks used in arch/x86/pci-dma.c.

That is what I meant with "with real memory"

But actually even for real IOMMus it can make sense to maintain
some memory for bypass.

Also there's usually the problem that not all busses are translated,
e.g. Calgary used to do that so for e.g. dev == NULL allocations you had
to use  these fallback algorithms anyways.

Clearly copying the fallback logic into lots of low level IOMMUs instead
of having it in one place is a big step backwards.

-Andi

-- 
ak@linux.intel.com

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[FUJITA Tomonori]

On Thu, 4 Sep 2008 10:05:28 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> On Thu, Sep 04, 2008 at 04:58:37PM +0900, FUJITA Tomonori wrote:
> > On Thu, 4 Sep 2008 09:44:41 +0200
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > On Thu, Sep 04, 2008 at 01:11:47PM +0900, FUJITA Tomonori wrote:
> > > > On Thu, 04 Sep 2008 00:09:51 +0200
> > > > Andi Kleen <andi@firstfloor.org> wrote:
> > > > 
> > > > > Joerg Roedel <joerg.roedel@amd.com> writes:
> > > > > 
> > > > > > The SWIOTLB version of dma_alloc_coherent allocates all memory with
> > > > > > GFP_DMA unconditionally. This leads sometimes unnecessary to allocation
> > > > > > failures. This patch makes the allocation strategy to use the DMA32 zone
> > > > > > first if this is possible. The changes are boot tested on AMD64 and
> > > > > > compile tested for i386 and IA64.
> > > > > 
> > > > > The high level dma_alloc_coherent() does that anyways.
> > > > 
> > > > The high level dma_alloc_coherent means the tricks in
> > > > arch/x86/pci-dma.c? If so, all the tricks has gone in tip/master.
> > > 
> > > Seems wrong -- that is something that is needed by multiple
> > > IOMMUs. Pretty much all which work with real memory instead
> > > of virtual mappings.
> > 
> > Not all the IOMMUs need the old tricks used in arch/x86/pci-dma.c.
> 
> That is what I meant with "with real memory"
> 
> But actually even for real IOMMus it can make sense to maintain
> some memory for bypass.

Even if it's tree, only each IOMMU knows what's the best. That's the
point of these changes to remove the tricks in arch/x86/pci-dma.c and
let IOMMUs do what they want.


> Also there's usually the problem that not all busses are translated,
> e.g. Calgary used to do that so for e.g. dev == NULL allocations you had
> to use  these fallback algorithms anyways.

dev == NULL isn't a minor thing. It's just using the fallback
device. Playing with gfp flag is the major trick. Some (new) IOMMUs
don't need it.

Handling dev == NULL in a common place is fine by me. It's a minor
issue.


> Clearly copying the fallback logic into lots of low level IOMMUs instead
> of having it in one place is a big step backwards.

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Andi Kleen]

> Even if it's tree, only each IOMMU knows what's the best. That's the
> point of these changes to remove the tricks in arch/x86/pci-dma.c and
> let IOMMUs do what they want.

Duplicating all the code.  Sounds great.
 
> > Also there's usually the problem that not all busses are translated,
> > e.g. Calgary used to do that so for e.g. dev == NULL allocations you had
> > to use  these fallback algorithms anyways.
> 
> dev == NULL isn't a minor thing. It's just using the fallback
> device.

You still have to handle it fully.

-Andi

Re: [PATCH] swiotlb: fix dma_alloc_coherent allocation failures with swiotlb

[Joerg Roedel]

On Thu, Sep 04, 2008 at 10:31:24AM +0200, Andi Kleen wrote:
> > Even if it's tree, only each IOMMU knows what's the best. That's the
> > point of these changes to remove the tricks in arch/x86/pci-dma.c and
> > let IOMMUs do what they want.
> 
> Duplicating all the code.  Sounds great.

Even greater is that hardware IOMMUs now don't pay the penalty of this
completly unnecessary alloc-free cycle in the generic dma_alloc_coherent
function and that now finally the driver specific free_coherent function
is called. And this all to the cost of minimal code duplication. If you
look into the patches you see that the old retry-loop is now in
pci-nommu.c where it belongs to. The GART also only needs small parts of
the old logic which are now in a specific alloc_coherent function for
this driver.
 
> > > Also there's usually the problem that not all busses are translated,
> > > e.g. Calgary used to do that so for e.g. dev == NULL allocations you had
> > > to use  these fallback algorithms anyways.
> > 
> > dev == NULL isn't a minor thing. It's just using the fallback
> > device.
> 
> You still have to handle it fully.

Thats why the handling of dev == NULL is still in the dma_alloc_coherent
function and should also stay there.

Joerg

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

[Joerg Roedel]

On Thu, Sep 04, 2008 at 01:11:46PM +0900, FUJITA Tomonori wrote:
> On Wed, 3 Sep 2008 22:01:14 +0200
> Joerg Roedel <joro@8bytes.org> wrote:
> 
> > On Thu, Sep 04, 2008 at 03:04:23AM +0900, FUJITA Tomonori wrote:
> > > dma_alloc_coherent in dma-mapping.h has a hack to use
> > > x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
> > > don't need such hack. The hack also makes it difficult for IOMMUs to
> > > make a proper decision because the hack hides the information.
> > 
> > I don't think its the right way to work around shortcomings of the
> > generic code in the architecture specific implementations. Especially
> > when the generic code can be easily fixed like in this case.
> 
> Well, the generic code should not have such work around.

I don't see that as a workaround. It is the best what we can do to
handle device dma_masks with the current Linux page allocator (if we
don't have hardware dma translation).

> As I wrote in another mail, you try to bring back the tricks used in
> arch/x86/kernel/pci-dma.c to lib/swiotlb.c. I try to bring back them
> to arch/x86/kernel/pci-swiotlb.c.

As I said, I don't see that as a 'hack'. It is also what the fixme
comment in the swiotlb alloc_coherent function stated, that this
function has to handle the DMA mask of the device.

> > > +static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > > +					dma_addr_t *dma_handle, gfp_t gfp)
> > > +{
> > > +	if (!dev) {
> > > +		dev = &x86_dma_fallback_dev;
> > > +		gfp |= GFP_DMA;
> > > +	}
> > 
> > This really should be checked in the generic x86 dma_alloc_coherent
> > function.
> 
> I don't think so. Any motherboards with the recent IOMMUs support ISA?

Not that I am aware of. But as we both know there are people who do
corner case tests with the dma-api functions like passing their own
created devices or even NULL to it an look what happens :-)
We have to handle this case in _every_ IOMMU implemention. So the
generic function is the right place for this check, imho.

Joerg

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

[FUJITA Tomonori]

On Thu, 4 Sep 2008 12:00:35 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Thu, Sep 04, 2008 at 01:11:46PM +0900, FUJITA Tomonori wrote:
> > On Wed, 3 Sep 2008 22:01:14 +0200
> > Joerg Roedel <joro@8bytes.org> wrote:
> > 
> > > On Thu, Sep 04, 2008 at 03:04:23AM +0900, FUJITA Tomonori wrote:
> > > > dma_alloc_coherent in dma-mapping.h has a hack to use
> > > > x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
> > > > don't need such hack. The hack also makes it difficult for IOMMUs to
> > > > make a proper decision because the hack hides the information.
> > > 
> > > I don't think its the right way to work around shortcomings of the
> > > generic code in the architecture specific implementations. Especially
> > > when the generic code can be easily fixed like in this case.
> > 
> > Well, the generic code should not have such work around.
> 
> I don't see that as a workaround. It is the best what we can do to
> handle device dma_masks with the current Linux page allocator (if we
> don't have hardware dma translation).

It's a hack. What swiotlb should do is allocating from only swiotlb's
memory area (I agree with Andi about this). dma_mask is irrelevant for
it. The current swiotlb_alloc_coherent is a bit wrong though I don't
like to bother IA64 people.


> > > > +static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > > > +					dma_addr_t *dma_handle, gfp_t gfp)
> > > > +{
> > > > +	if (!dev) {
> > > > +		dev = &x86_dma_fallback_dev;
> > > > +		gfp |= GFP_DMA;
> > > > +	}
> > > 
> > > This really should be checked in the generic x86 dma_alloc_coherent
> > > function.
> > 
> > I don't think so. Any motherboards with the recent IOMMUs support ISA?
> 
> Not that I am aware of. But as we both know there are people who do
> corner case tests with the dma-api functions like passing their own
> created devices or even NULL to it an look what happens :-)

That's the wrong way to use DMA API. We don't need to care about
it. The only reason we handle it now is for the ancient code.


> We have to handle this case in _every_ IOMMU implemention. So the
> generic function is the right place for this check, imho.

As I said, I don't think that every IOMMU need to handle it. Well,
it's a minor issue. I don't care much so handling it in a generic code
is fine. But as I pointed out in another mail, the fallback device has
DMA_32BIT_MASK so hiding it in dma-mapping.h makes it difficult to
make pci-dma.c work as before.