[PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non DMA_64BIT_MASK devices

[PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non DMA_64BIT_MASK devices

[FUJITA Tomonori]

Before the dma ops unification, IA64 always uses GFP_DMA for
dma_alloc_coherent like:

#define dma_alloc_coherent(dev, size, handle, gfp)	\
	platform_dma_alloc_coherent(dev, size, handle, (gfp) | GFP_DMA)

This GFP_DMA enforcement doesn't make sense for IOMMUs since they can
do address translation to give addresses that devices can access
to. The IOMMU drivers ignore the zone flag. However, this is still
necessary for swiotlb since it can't do address translation.

We don't always need to use GFP_DMA for swiotlb. We need GFP_DMA for
devices incapable of 64bit DMA.

This patch is sorta updated version of:

http://marc.info/?l=linux-kernel&m\x122638215612705&w=2

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Yasunori Goto <y-goto@jp.fujitsu.com>
---
 arch/ia64/kernel/pci-swiotlb.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
index 717ad4f..573f02c 100644
--- a/arch/ia64/kernel/pci-swiotlb.c
+++ b/arch/ia64/kernel/pci-swiotlb.c
@@ -13,8 +13,16 @@
 int swiotlb __read_mostly;
 EXPORT_SYMBOL(swiotlb);
 
+static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
+					 dma_addr_t *dma_handle, gfp_t gfp)
+{
+	if (dev->coherent_dma_mask != DMA_64BIT_MASK)
+		gfp |= GFP_DMA;
+	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
+}
+
 struct dma_map_ops swiotlb_dma_ops = {
-	.alloc_coherent = swiotlb_alloc_coherent,
+	.alloc_coherent = ia64_swiotlb_alloc_coherent,
 	.free_coherent = swiotlb_free_coherent,
 	.map_page = swiotlb_map_page,
 	.unmap_page = swiotlb_unmap_page,
-- 
1.6.0.6

[PATCH -tip 2/3] IA64: fix VT-d dma_mapping_error

[FUJITA Tomonori]

dma_mapping_error is used to see if dma_map_single and dma_map_page
succeed. IA64 VT-d dma_mapping_error always says that dma_map_single
is successful even though it could fail. Note that X86 VT-d works
properly in this regard.

This patch fixes IA64 VT-d dma_mapping_error by adding VT-d's own
dma_mapping_error() that works for both X86_64 and IA64. VT-d uses
zero as an error dma address so VT-d's dma_mapping_error returns 1 if
a passed dma address is zero (as x86's VT-d dma_mapping_error does
now).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: David Woodhouse <David.Woodhouse@intel.com>
---
 arch/ia64/kernel/pci-dma.c |    6 ------
 drivers/pci/intel-iommu.c  |    6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
index a647f11..e4cb443 100644
--- a/arch/ia64/kernel/pci-dma.c
+++ b/arch/ia64/kernel/pci-dma.c
@@ -99,11 +99,6 @@ int iommu_dma_supported(struct device *dev, u64 mask)
 }
 EXPORT_SYMBOL(iommu_dma_supported);
 
-static int vtd_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
-{
-	return 0;
-}
-
 void __init pci_iommu_alloc(void)
 {
 	dma_ops = &intel_dma_ops;
@@ -113,7 +108,6 @@ void __init pci_iommu_alloc(void)
 	dma_ops->sync_single_for_device = machvec_dma_sync_single;
 	dma_ops->sync_sg_for_device = machvec_dma_sync_sg;
 	dma_ops->dma_supported = iommu_dma_supported;
-	dma_ops->mapping_error = vtd_dma_mapping_error;
 
 	/*
 	 * The order of these functions is important for
diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index c933980..59de563 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2581,6 +2581,11 @@ int intel_map_sg(struct device *hwdev, struct scatterlist *sglist, int nelems,
 	return nelems;
 }
 
+static int intel_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	return !dma_addr;
+}
+
 struct dma_map_ops intel_dma_ops = {
 	.alloc_coherent = intel_alloc_coherent,
 	.free_coherent = intel_free_coherent,
@@ -2588,6 +2593,7 @@ struct dma_map_ops intel_dma_ops = {
 	.unmap_sg = intel_unmap_sg,
 	.map_page = intel_map_page,
 	.unmap_page = intel_unmap_page,
+	.mapping_error = intel_mapping_error,
 };
 
 static inline int iommu_domain_cache_init(void)
-- 
1.6.0.6

[PATCH -tip 3/3] intel-iommu: make dma mapping functions static

[FUJITA Tomonori]

The dma ops unification enables X86 and IA64 to share intel_dma_ops so
we can make dma mapping functions static. This also remove unused
intel_map_single().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/pci/intel-iommu.c   |   29 +++++++++++------------------
 include/linux/intel-iommu.h |    9 ---------
 2 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 59de563..628f8b7 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2283,13 +2283,6 @@ static dma_addr_t intel_map_page(struct device *dev, struct page *page,
 				  dir, to_pci_dev(dev)->dma_mask);
 }
 
-dma_addr_t intel_map_single(struct device *hwdev, phys_addr_t paddr,
-			    size_t size, int dir)
-{
-	return __intel_map_single(hwdev, paddr, size, dir,
-				  to_pci_dev(hwdev)->dma_mask);
-}
-
 static void flush_unmaps(void)
 {
 	int i, j;
@@ -2397,14 +2390,14 @@ static void intel_unmap_page(struct device *dev, dma_addr_t dev_addr,
 	}
 }
 
-void intel_unmap_single(struct device *dev, dma_addr_t dev_addr, size_t size,
-			int dir)
+static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr, size_t size,
+			       int dir)
 {
 	intel_unmap_page(dev, dev_addr, size, dir, NULL);
 }
 
-void *intel_alloc_coherent(struct device *hwdev, size_t size,
-			   dma_addr_t *dma_handle, gfp_t flags)
+static void *intel_alloc_coherent(struct device *hwdev, size_t size,
+				  dma_addr_t *dma_handle, gfp_t flags)
 {
 	void *vaddr;
 	int order;
@@ -2427,8 +2420,8 @@ void *intel_alloc_coherent(struct device *hwdev, size_t size,
 	return NULL;
 }
 
-void intel_free_coherent(struct device *hwdev, size_t size, void *vaddr,
-			 dma_addr_t dma_handle)
+static void intel_free_coherent(struct device *hwdev, size_t size, void *vaddr,
+				dma_addr_t dma_handle)
 {
 	int order;
 
@@ -2441,9 +2434,9 @@ void intel_free_coherent(struct device *hwdev, size_t size, void *vaddr,
 
 #define SG_ENT_VIRT_ADDRESS(sg)	(sg_virt((sg)))
 
-void intel_unmap_sg(struct device *hwdev, struct scatterlist *sglist,
-		    int nelems, enum dma_data_direction dir,
-		    struct dma_attrs *attrs)
+static void intel_unmap_sg(struct device *hwdev, struct scatterlist *sglist,
+			   int nelems, enum dma_data_direction dir,
+			   struct dma_attrs *attrs)
 {
 	int i;
 	struct pci_dev *pdev = to_pci_dev(hwdev);
@@ -2500,8 +2493,8 @@ static int intel_nontranslate_map_sg(struct device *hddev,
 	return nelems;
 }
 
-int intel_map_sg(struct device *hwdev, struct scatterlist *sglist, int nelems,
-		 enum dma_data_direction dir, struct dma_attrs *attrs)
+static int intel_map_sg(struct device *hwdev, struct scatterlist *sglist, int nelems,
+			enum dma_data_direction dir, struct dma_attrs *attrs)
 {
 	void *addr;
 	int i;
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index a254db1..43412ae 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -330,13 +330,4 @@ extern int qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
 
 extern void qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu);
 
-extern void *intel_alloc_coherent(struct device *, size_t, dma_addr_t *, gfp_t);
-extern void intel_free_coherent(struct device *, size_t, void *, dma_addr_t);
-extern dma_addr_t intel_map_single(struct device *, phys_addr_t, size_t, int);
-extern void intel_unmap_single(struct device *, dma_addr_t, size_t, int);
-extern int intel_map_sg(struct device *, struct scatterlist *, int,
-			enum dma_data_direction, struct dma_attrs *);
-extern void intel_unmap_sg(struct device *, struct scatterlist *, int,
-			   enum dma_data_direction, struct dma_attrs *);
-
 #endif
-- 
1.6.0.6

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non DMA_64BIT_MASK devices

[Yasunori Goto]

> Before the dma ops unification, IA64 always uses GFP_DMA for
> dma_alloc_coherent like:
> 
> #define dma_alloc_coherent(dev, size, handle, gfp)	\
> 	platform_dma_alloc_coherent(dev, size, handle, (gfp) | GFP_DMA)
> 
> This GFP_DMA enforcement doesn't make sense for IOMMUs since they can
> do address translation to give addresses that devices can access
> to. The IOMMU drivers ignore the zone flag. However, this is still
> necessary for swiotlb since it can't do address translation.
> 
> We don't always need to use GFP_DMA for swiotlb. We need GFP_DMA for
> devices incapable of 64bit DMA.
> 
> This patch is sorta updated version of:
> 
> http://marc.info/?l=linux-kernel&m\x122638215612705&w=2
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Yasunori Goto <y-goto@jp.fujitsu.com>
> ---
>  arch/ia64/kernel/pci-swiotlb.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> index 717ad4f..573f02c 100644
> --- a/arch/ia64/kernel/pci-swiotlb.c
> +++ b/arch/ia64/kernel/pci-swiotlb.c
> @@ -13,8 +13,16 @@
>  int swiotlb __read_mostly;
>  EXPORT_SYMBOL(swiotlb);
>  
> +static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
> +					 dma_addr_t *dma_handle, gfp_t gfp)
> +{
> +	if (dev->coherent_dma_mask != DMA_64BIT_MASK)
> +		gfp |= GFP_DMA;
> +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> +}
> +
>  struct dma_map_ops swiotlb_dma_ops = {
> -	.alloc_coherent = swiotlb_alloc_coherent,
> +	.alloc_coherent = ia64_swiotlb_alloc_coherent,
>  	.free_coherent = swiotlb_free_coherent,
>  	.map_page = swiotlb_map_page,
>  	.unmap_page = swiotlb_unmap_page,
> -- 
> 1.6.0.6


Though I may be misunderstanding something, this initialization seems to be
called only when CONFIG_IA64_DIG_VTD is on.

CONFIG_IA64_DIG_VTD
  platform_dma_init() -> pci_iommu_alloc() -> pci_swiotlb_init()

CONFIG_IA64_DIG/CONFIG_IA64_GENERIC
  platform_dma_init() -> swiotlb_dma_init()

Is it intended? 
Do you wish GFP_DMA should be always ON when generic config?


Bye.

-- 
Yasunori Goto 

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non

[FUJITA Tomonori]

On Thu, 29 Jan 2009 20:41:07 +0900
Yasunori Goto <y-goto@jp.fujitsu.com> wrote:

> > Before the dma ops unification, IA64 always uses GFP_DMA for
> > dma_alloc_coherent like:
> > 
> > #define dma_alloc_coherent(dev, size, handle, gfp)	\
> > 	platform_dma_alloc_coherent(dev, size, handle, (gfp) | GFP_DMA)
> > 
> > This GFP_DMA enforcement doesn't make sense for IOMMUs since they can
> > do address translation to give addresses that devices can access
> > to. The IOMMU drivers ignore the zone flag. However, this is still
> > necessary for swiotlb since it can't do address translation.
> > 
> > We don't always need to use GFP_DMA for swiotlb. We need GFP_DMA for
> > devices incapable of 64bit DMA.
> > 
> > This patch is sorta updated version of:
> > 
> > http://marc.info/?l=linux-kernel&m\x122638215612705&w=2
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Yasunori Goto <y-goto@jp.fujitsu.com>
> > ---
> >  arch/ia64/kernel/pci-swiotlb.c |   10 +++++++++-
> >  1 files changed, 9 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> > index 717ad4f..573f02c 100644
> > --- a/arch/ia64/kernel/pci-swiotlb.c
> > +++ b/arch/ia64/kernel/pci-swiotlb.c
> > @@ -13,8 +13,16 @@
> >  int swiotlb __read_mostly;
> >  EXPORT_SYMBOL(swiotlb);
> >  
> > +static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > +					 dma_addr_t *dma_handle, gfp_t gfp)
> > +{
> > +	if (dev->coherent_dma_mask != DMA_64BIT_MASK)
> > +		gfp |= GFP_DMA;
> > +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> > +}
> > +
> >  struct dma_map_ops swiotlb_dma_ops = {
> > -	.alloc_coherent = swiotlb_alloc_coherent,
> > +	.alloc_coherent = ia64_swiotlb_alloc_coherent,
> >  	.free_coherent = swiotlb_free_coherent,
> >  	.map_page = swiotlb_map_page,
> >  	.unmap_page = swiotlb_unmap_page,
> > -- 
> > 1.6.0.6
> 
> 
> Though I may be misunderstanding something, this initialization seems to be
> called only when CONFIG_IA64_DIG_VTD is on.

Hmm, what's 'this initialization'?

This patch always sets swiotlb_dma_ops.alloc_coherent to
ia64_swiotlb_alloc_coherent.


> CONFIG_IA64_DIG_VTD
>   platform_dma_init() -> pci_iommu_alloc() -> pci_swiotlb_init()
> 
> CONFIG_IA64_DIG/CONFIG_IA64_GENERIC
>   platform_dma_init() -> swiotlb_dma_init()
> 
> Is it intended? 
> Do you wish GFP_DMA should be always ON when generic config?

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non

[Ingo Molnar]

Applied them to tip:core/iommu:

 d7ab5c4: intel-iommu: make dma mapping functions static
 dfb805e: IA64: fix VT-d dma_mapping_error
 97d9800: IA64: fix swiotlb alloc_coherent for non DMA_64BIT_MASK devices

thanks!

	Ingo

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non DMA_64BIT_MASK devices

[Yasunori Goto]

> On Thu, 29 Jan 2009 20:41:07 +0900
> Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> 
> > > Before the dma ops unification, IA64 always uses GFP_DMA for
> > > dma_alloc_coherent like:
> > > 
> > > #define dma_alloc_coherent(dev, size, handle, gfp)	\
> > > 	platform_dma_alloc_coherent(dev, size, handle, (gfp) | GFP_DMA)
> > > 
> > > This GFP_DMA enforcement doesn't make sense for IOMMUs since they can
> > > do address translation to give addresses that devices can access
> > > to. The IOMMU drivers ignore the zone flag. However, this is still
> > > necessary for swiotlb since it can't do address translation.
> > > 
> > > We don't always need to use GFP_DMA for swiotlb. We need GFP_DMA for
> > > devices incapable of 64bit DMA.
> > > 
> > > This patch is sorta updated version of:
> > > 
> > > http://marc.info/?l=linux-kernel&m\x122638215612705&w=2
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > Cc: Yasunori Goto <y-goto@jp.fujitsu.com>
> > > ---
> > >  arch/ia64/kernel/pci-swiotlb.c |   10 +++++++++-
> > >  1 files changed, 9 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> > > index 717ad4f..573f02c 100644
> > > --- a/arch/ia64/kernel/pci-swiotlb.c
> > > +++ b/arch/ia64/kernel/pci-swiotlb.c
> > > @@ -13,8 +13,16 @@
> > >  int swiotlb __read_mostly;
> > >  EXPORT_SYMBOL(swiotlb);
> > >  
> > > +static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > > +					 dma_addr_t *dma_handle, gfp_t gfp)
> > > +{
> > > +	if (dev->coherent_dma_mask != DMA_64BIT_MASK)
> > > +		gfp |= GFP_DMA;
> > > +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> > > +}
> > > +
> > >  struct dma_map_ops swiotlb_dma_ops = {
> > > -	.alloc_coherent = swiotlb_alloc_coherent,
> > > +	.alloc_coherent = ia64_swiotlb_alloc_coherent,
> > >  	.free_coherent = swiotlb_free_coherent,
> > >  	.map_page = swiotlb_map_page,
> > >  	.unmap_page = swiotlb_unmap_page,
> > > -- 
> > > 1.6.0.6
> > 
> > 
> > Though I may be misunderstanding something, this initialization seems to be
> > called only when CONFIG_IA64_DIG_VTD is on.
> 
> Hmm, what's 'this initialization'?
> 
> This patch always sets swiotlb_dma_ops.alloc_coherent to
> ia64_swiotlb_alloc_coherent.

I tested your patch, but, ia64_swiotlb_alloc_coherent() was not called with
CONFIG_GENERIC.

swiotlb_dma_ops is set by pci_swiotlb_init().
However, it seems to be not called when CONFIG_IA64_DIG/Generic configuration.


> > > This patch is sorta updated version of:
> > > 
> > > http://marc.info/?l=linux-kernel&m\x122638215612705&w=2
> > > 

This old version is effective even CONFIG_GENERIC/CONFIG_IA64_DIG.

Bye.

> 
> 
> > CONFIG_IA64_DIG_VTD
> >   platform_dma_init() -> pci_iommu_alloc() -> pci_swiotlb_init()
> > 
> > CONFIG_IA64_DIG/CONFIG_IA64_GENERIC
> >   platform_dma_init() -> swiotlb_dma_init()
> > 
> > Is it intended? 
> > Do you wish GFP_DMA should be always ON when generic config?


-- 
Yasunori Goto 

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non

[FUJITA Tomonori]

On Fri, 30 Jan 2009 07:54:07 +0900
Yasunori Goto <y-goto@jp.fujitsu.com> wrote:

> > On Thu, 29 Jan 2009 20:41:07 +0900
> > Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> > 
> > > > Before the dma ops unification, IA64 always uses GFP_DMA for
> > > > dma_alloc_coherent like:
> > > > 
> > > > #define dma_alloc_coherent(dev, size, handle, gfp)	\
> > > > 	platform_dma_alloc_coherent(dev, size, handle, (gfp) | GFP_DMA)
> > > > 
> > > > This GFP_DMA enforcement doesn't make sense for IOMMUs since they can
> > > > do address translation to give addresses that devices can access
> > > > to. The IOMMU drivers ignore the zone flag. However, this is still
> > > > necessary for swiotlb since it can't do address translation.
> > > > 
> > > > We don't always need to use GFP_DMA for swiotlb. We need GFP_DMA for
> > > > devices incapable of 64bit DMA.
> > > > 
> > > > This patch is sorta updated version of:
> > > > 
> > > > http://marc.info/?l=linux-kernel&m\x122638215612705&w=2
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > Cc: Yasunori Goto <y-goto@jp.fujitsu.com>
> > > > ---
> > > >  arch/ia64/kernel/pci-swiotlb.c |   10 +++++++++-
> > > >  1 files changed, 9 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> > > > index 717ad4f..573f02c 100644
> > > > --- a/arch/ia64/kernel/pci-swiotlb.c
> > > > +++ b/arch/ia64/kernel/pci-swiotlb.c
> > > > @@ -13,8 +13,16 @@
> > > >  int swiotlb __read_mostly;
> > > >  EXPORT_SYMBOL(swiotlb);
> > > >  
> > > > +static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > > > +					 dma_addr_t *dma_handle, gfp_t gfp)
> > > > +{
> > > > +	if (dev->coherent_dma_mask != DMA_64BIT_MASK)
> > > > +		gfp |= GFP_DMA;
> > > > +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> > > > +}
> > > > +
> > > >  struct dma_map_ops swiotlb_dma_ops = {
> > > > -	.alloc_coherent = swiotlb_alloc_coherent,
> > > > +	.alloc_coherent = ia64_swiotlb_alloc_coherent,
> > > >  	.free_coherent = swiotlb_free_coherent,
> > > >  	.map_page = swiotlb_map_page,
> > > >  	.unmap_page = swiotlb_unmap_page,
> > > > -- 
> > > > 1.6.0.6
> > > 
> > > 
> > > Though I may be misunderstanding something, this initialization seems to be
> > > called only when CONFIG_IA64_DIG_VTD is on.
> > 
> > Hmm, what's 'this initialization'?
> > 
> > This patch always sets swiotlb_dma_ops.alloc_coherent to
> > ia64_swiotlb_alloc_coherent.
> 
> I tested your patch, but, ia64_swiotlb_alloc_coherent() was not called with
> CONFIG_GENERIC.

You use the generic kernel on a DIG box, right? If so, yeah, there is
something wrong.


> swiotlb_dma_ops is set by pci_swiotlb_init().
> However, it seems to be not called when CONFIG_IA64_DIG/Generic configuration.

You are talking about setting dma_ops to swiotlb_dma_ops?

I think that swiotlb_dma_init() also sets dma_ops to swiotlb_dma_ops.

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non DMA_64BIT_MASK devices

[Yasunori Goto]

> On Fri, 30 Jan 2009 07:54:07 +0900
> Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> 
> > > On Thu, 29 Jan 2009 20:41:07 +0900
> > > Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> > > 
> > > > > Before the dma ops unification, IA64 always uses GFP_DMA for
> > > > > dma_alloc_coherent like:
> > > > > 
> > > > > #define dma_alloc_coherent(dev, size, handle, gfp)	\
> > > > > 	platform_dma_alloc_coherent(dev, size, handle, (gfp) | GFP_DMA)
> > > > > 
> > > > > This GFP_DMA enforcement doesn't make sense for IOMMUs since they can
> > > > > do address translation to give addresses that devices can access
> > > > > to. The IOMMU drivers ignore the zone flag. However, this is still
> > > > > necessary for swiotlb since it can't do address translation.
> > > > > 
> > > > > We don't always need to use GFP_DMA for swiotlb. We need GFP_DMA for
> > > > > devices incapable of 64bit DMA.
> > > > > 
> > > > > This patch is sorta updated version of:
> > > > > 
> > > > > http://marc.info/?l=linux-kernel&m\x122638215612705&w=2
> > > > > 
> > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > > Cc: Yasunori Goto <y-goto@jp.fujitsu.com>
> > > > > ---
> > > > >  arch/ia64/kernel/pci-swiotlb.c |   10 +++++++++-
> > > > >  1 files changed, 9 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> > > > > index 717ad4f..573f02c 100644
> > > > > --- a/arch/ia64/kernel/pci-swiotlb.c
> > > > > +++ b/arch/ia64/kernel/pci-swiotlb.c
> > > > > @@ -13,8 +13,16 @@
> > > > >  int swiotlb __read_mostly;
> > > > >  EXPORT_SYMBOL(swiotlb);
> > > > >  
> > > > > +static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > > > > +					 dma_addr_t *dma_handle, gfp_t gfp)
> > > > > +{
> > > > > +	if (dev->coherent_dma_mask != DMA_64BIT_MASK)
> > > > > +		gfp |= GFP_DMA;
> > > > > +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> > > > > +}
> > > > > +
> > > > >  struct dma_map_ops swiotlb_dma_ops = {
> > > > > -	.alloc_coherent = swiotlb_alloc_coherent,
> > > > > +	.alloc_coherent = ia64_swiotlb_alloc_coherent,
> > > > >  	.free_coherent = swiotlb_free_coherent,
> > > > >  	.map_page = swiotlb_map_page,
> > > > >  	.unmap_page = swiotlb_unmap_page,
> > > > > -- 
> > > > > 1.6.0.6
> > > > 
> > > > 
> > > > Though I may be misunderstanding something, this initialization seems to be
> > > > called only when CONFIG_IA64_DIG_VTD is on.
> > > 
> > > Hmm, what's 'this initialization'?
> > > 
> > > This patch always sets swiotlb_dma_ops.alloc_coherent to
> > > ia64_swiotlb_alloc_coherent.
> > 
> > I tested your patch, but, ia64_swiotlb_alloc_coherent() was not called with
> > CONFIG_GENERIC.
> 
> You use the generic kernel on a DIG box, right? If so, yeah, there is
> something wrong.
> 
> 
> > swiotlb_dma_ops is set by pci_swiotlb_init().
> > However, it seems to be not called when CONFIG_IA64_DIG/Generic configuration.
> 
> You are talking about setting dma_ops to swiotlb_dma_ops?
> 
> I think that swiotlb_dma_init() also sets dma_ops to swiotlb_dma_ops.

Certainly.
I overlooked the 7th patch of your unifying dma_ops patch set.
ia64_swiotlb_alloc_coherent was called when I tested again.
I'm sorry.

However, GFP_DMA is still always on in ia64_swiotlb_alloc_coherent()
regardless of the value of dev->coherent_dma_mask.

After following patch applied, GFP_DMA is set correctly by your patch.
Am I still overlooking other your patch?

Bye.



---
 arch/ia64/include/asm/dma-mapping.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: dma_fix_test/arch/ia64/include/asm/dma-mapping.h
=================================--- dma_fix_test.orig/arch/ia64/include/asm/dma-mapping.h       2009-01-30 19:36:33.000000000 +0900
+++ dma_fix_test/arch/ia64/include/asm/dma-mapping.h    2009-01-30 20:20:06.000000000 +0900
@@ -71,7 +71,7 @@ static inline void *dma_alloc_coherent(s
                                       dma_addr_t *daddr, gfp_t gfp)
 {
        struct dma_mapping_ops *ops = platform_dma_get_ops(dev);
-       return ops->alloc_coherent(dev, size, daddr, gfp | GFP_DMA);
+       return ops->alloc_coherent(dev, size, daddr, gfp);
 }

 static inline void dma_free_coherent(struct device *dev, size_t size,


-- 
Yasunori Goto 

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non

[FUJITA Tomonori]

On Fri, 30 Jan 2009 20:24:13 +0900
Yasunori Goto <y-goto@jp.fujitsu.com> wrote:

> > > swiotlb_dma_ops is set by pci_swiotlb_init().
> > > However, it seems to be not called when CONFIG_IA64_DIG/Generic configuration.
> > 
> > You are talking about setting dma_ops to swiotlb_dma_ops?
> > 
> > I think that swiotlb_dma_init() also sets dma_ops to swiotlb_dma_ops.
> 
> Certainly.
> I overlooked the 7th patch of your unifying dma_ops patch set.
> ia64_swiotlb_alloc_coherent was called when I tested again.
> I'm sorry.
> 
> However, GFP_DMA is still always on in ia64_swiotlb_alloc_coherent()
> regardless of the value of dev->coherent_dma_mask.
> 
> After following patch applied, GFP_DMA is set correctly by your patch.
> Am I still overlooking other your patch?

No, you are not. We need the following change. Somehow I thought that
I already made this change.

Can you resend this to Ingo with the proper description and
Signed-off-by (you can add my Acked-by if you like)?

Thanks,


> Bye.
> 
> 
> 
> ---
>  arch/ia64/include/asm/dma-mapping.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: dma_fix_test/arch/ia64/include/asm/dma-mapping.h
> =================================> --- dma_fix_test.orig/arch/ia64/include/asm/dma-mapping.h       2009-01-30 19:36:33.000000000 +0900
> +++ dma_fix_test/arch/ia64/include/asm/dma-mapping.h    2009-01-30 20:20:06.000000000 +0900
> @@ -71,7 +71,7 @@ static inline void *dma_alloc_coherent(s
>                                        dma_addr_t *daddr, gfp_t gfp)
>  {
>         struct dma_mapping_ops *ops = platform_dma_get_ops(dev);
> -       return ops->alloc_coherent(dev, size, daddr, gfp | GFP_DMA);
> +       return ops->alloc_coherent(dev, size, daddr, gfp);
>  }
> 
>  static inline void dma_free_coherent(struct device *dev, size_t size,
> 
> 
> -- 
> Yasunori Goto 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -tip 1/3] IA64: fix swiotlb alloc_coherent for non DMA_64BIT_MASK devices

[Yasunori Goto]

> On Fri, 30 Jan 2009 20:24:13 +0900
> Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> 
> > > > swiotlb_dma_ops is set by pci_swiotlb_init().
> > > > However, it seems to be not called when CONFIG_IA64_DIG/Generic configuration.
> > > 
> > > You are talking about setting dma_ops to swiotlb_dma_ops?
> > > 
> > > I think that swiotlb_dma_init() also sets dma_ops to swiotlb_dma_ops.
> > 
> > Certainly.
> > I overlooked the 7th patch of your unifying dma_ops patch set.
> > ia64_swiotlb_alloc_coherent was called when I tested again.
> > I'm sorry.
> > 
> > However, GFP_DMA is still always on in ia64_swiotlb_alloc_coherent()
> > regardless of the value of dev->coherent_dma_mask.
> > 
> > After following patch applied, GFP_DMA is set correctly by your patch.
> > Am I still overlooking other your patch?
> 
> No, you are not. We need the following change. Somehow I thought that
> I already made this change.
> 
> Can you resend this to Ingo with the proper description and
> Signed-off-by (you can add my Acked-by if you like)?

Sure!
Ingo-san. Could you apply the following patch?

Bye.

-----

Because dma_alloc_coherent() always required DMA zone even if DMA is
NOT necessary, FUJITA Tomonori posted a patch to fix it.

http://marc.info/?l=linux-ia64&m\x123314730923356&w=2

However, this fix needs one more patch to fix completely.
I tested and confirmed dma_alloc_coherent() returns 
correct zone after applied following patch.


Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/ia64/include/asm/dma-mapping.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: dma_fix_test/arch/ia64/include/asm/dma-mapping.h
=================================--- dma_fix_test.orig/arch/ia64/include/asm/dma-mapping.h	2009-01-30 19:36:33.000000000 +0900
+++ dma_fix_test/arch/ia64/include/asm/dma-mapping.h	2009-01-30 20:20:06.000000000 +0900
@@ -71,7 +71,7 @@ static inline void *dma_alloc_coherent(s
 				       dma_addr_t *daddr, gfp_t gfp)
 {
 	struct dma_mapping_ops *ops = platform_dma_get_ops(dev);
-	return ops->alloc_coherent(dev, size, daddr, gfp | GFP_DMA);
+	return ops->alloc_coherent(dev, size, daddr, gfp);
 }
 
 static inline void dma_free_coherent(struct device *dev, size_t size,


-- 
Yasunori Goto