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

[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

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 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 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 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.

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

[Robert Hancock]

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:
>>> +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?

Probably some LPC devices on those boards..