linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64
@ 2015-09-17  8:40 Andre Przywara
  2015-09-17 18:38 ` David Miller
  0 siblings, 1 reply; 11+ messages in thread
From: Andre Przywara @ 2015-09-17  8:40 UTC (permalink / raw)
  To: David S. Miller; +Cc: Joerg Roedel, sparclinux, linux-kernel, linux-arm-kernel

Though the file lib/iommu_common.c resides in a common directory, the
code in there is actually only used by Sparc(64).
The recent change of DMA_ERROR_CODE in the ARM tree now generates a
compiler warning when compiled with LPAE support (where dma_addr_t is
u64, but unsigned long is still u32):
*********
In file included from /src/linux/include/linux/dma-mapping.h:86:0,
                 from /src/linux/lib/iommu-common.c:11:
/src/linux/lib/iommu-common.c: In function 'iommu_tbl_range_alloc':
/src/linux/arch/arm/include/asm/dma-mapping.h:16:24: warning: large
integer implicitly truncated to unsigned type [-Woverflow]
 #define DMA_ERROR_CODE (~(dma_addr_t)0x0)
                        ^
/src/linux/lib/iommu-common.c:127:10: note: in expansion of macro
'DMA_ERROR_CODE'
   return DMA_ERROR_CODE;
          ^
*********

It seems the types used in this file are not really correct, but a
fix isn't trivial.
So for the time being restrict this code to be compiled only when we
actually need it.
Compile tested on Sparc, Sparc64, PPC64, ARM, ARM64, x86.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/sparc/Kconfig | 3 +++
 lib/Makefile       | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 56442d2..1cedb7e 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -93,6 +93,9 @@ config IOMMU_HELPER
 	bool
 	default y if SPARC64
 
+config IOMMU_TBL_HELPER
+	def_bool y if IOMMU_HELPER
+
 config STACKTRACE_SUPPORT
 	bool
 	default y if SPARC64
diff --git a/lib/Makefile b/lib/Makefile
index 13a7c6a..82944a1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -113,7 +113,8 @@ obj-$(CONFIG_AUDIT_GENERIC) += audit.o
 obj-$(CONFIG_AUDIT_COMPAT_GENERIC) += compat_audit.o
 
 obj-$(CONFIG_SWIOTLB) += swiotlb.o
-obj-$(CONFIG_IOMMU_HELPER) += iommu-helper.o iommu-common.o
+obj-$(CONFIG_IOMMU_HELPER) += iommu-helper.o
+obj-$(CONFIG_IOMMU_TBL_HELPER) += iommu-common.o
 obj-$(CONFIG_FAULT_INJECTION) += fault-inject.o
 obj-$(CONFIG_NOTIFIER_ERROR_INJECTION) += notifier-error-inject.o
 obj-$(CONFIG_CPU_NOTIFIER_ERROR_INJECT) += cpu-notifier-error-inject.o
-- 
2.5.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64
  2015-09-17  8:40 [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 Andre Przywara
@ 2015-09-17 18:38 ` David Miller
  2015-09-18  9:03   ` Andre Przywara
  0 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2015-09-17 18:38 UTC (permalink / raw)
  To: andre.przywara; +Cc: joro, sparclinux, linux-kernel, linux-arm-kernel

From: Andre Przywara <andre.przywara@arm.com>
Date: Thu, 17 Sep 2015 09:40:27 +0100

> It seems the types used in this file are not really correct, but a
> fix isn't trivial.  So for the time being restrict this code to be
> compiled only when we actually need it.  Compile tested on Sparc,
> Sparc64, PPC64, ARM, ARM64, x86.

The whole reason this code gets compiled unconditionally is to
ensure it's portability so that other platforms can use it at
some point.

So I would ask that, instead of sweeping it under the rug, you
help work on fixing the problem you've uncovered.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64
  2015-09-17 18:38 ` David Miller
@ 2015-09-18  9:03   ` Andre Przywara
  2015-09-18  9:09     ` [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc() Andre Przywara
  2015-09-18 17:09     ` [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 David Miller
  0 siblings, 2 replies; 11+ messages in thread
From: Andre Przywara @ 2015-09-18  9:03 UTC (permalink / raw)
  To: David Miller; +Cc: joro, sparclinux, linux-kernel, linux-arm-kernel

Hi David,

On 17/09/15 19:38, David Miller wrote:
> From: Andre Przywara <andre.przywara@arm.com>
> Date: Thu, 17 Sep 2015 09:40:27 +0100
> 
>> It seems the types used in this file are not really correct, but a
>> fix isn't trivial.  So for the time being restrict this code to be
>> compiled only when we actually need it.  Compile tested on Sparc,
>> Sparc64, PPC64, ARM, ARM64, x86.
> 
> The whole reason this code gets compiled unconditionally is to
> ensure it's portability so that other platforms can use it at
> some point.
> 
> So I would ask that, instead of sweeping it under the rug, you
> help work on fixing the problem you've uncovered.

OK, I understand that. To be honest I tried that: changing the return
type of iommu_tbl_range_alloc() to dma_addr_t and taking it from there.
This resulted in quite some changes in the callers in arch/sparc, which
scared me a bit because I don't have a clue about the Sparc IOMMU and I
cannot test it seriously.
Also I deemed it not appropriate still for the 4.3 release, which was
what my patch was aiming at (because it is a 4.3 merging window regression).

So I have a simpler version of the proper fix mentioned above, which
simply changes the return type of iommu_tbl_range_alloc() and pushes
this warning away, though this still isn't right and just papers over
the issue, I think. (Sending that out in a minute in a separate mail for
reference).

So what is your hunch on that? Shall we go with a dual approach: Fixing
the compiler warning with either the simple return type change or the
Kconfig approach now for 4.3 and revert that after the actual culprit
gets fixed later?

Or do you know of a simpler way to fixing it properly which could make
it still into 4.3?

Cheers,
Andre.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc()
  2015-09-18  9:03   ` Andre Przywara
@ 2015-09-18  9:09     ` Andre Przywara
  2015-10-21 19:12       ` Geert Uytterhoeven
  2015-09-18 17:09     ` [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 David Miller
  1 sibling, 1 reply; 11+ messages in thread
From: Andre Przywara @ 2015-09-18  9:09 UTC (permalink / raw)
  To: David S. Miller; +Cc: Joerg Roedel, sparclinux, linux-kernel, linux-arm-kernel

Though iommu_tbl_range_alloc() is only used by Sparc code, the
function itself lives in lib/iommu-common.c and is thus included in
other architecture's code as well.
When compiled on a 32-bit architecture using 64-bit DMA addresses
(ARM with LPAE), there is a compiler warning about a type mismatch
between dma_addr_t and the return type of this function:

In file included from /src/linux/include/linux/dma-mapping.h:86:0,
                 from /src/linux/lib/iommu-common.c:11:
/src/linux/lib/iommu-common.c: In function 'iommu_tbl_range_alloc':
/src/linux/arch/arm/include/asm/dma-mapping.h:16:24: warning: large integer implicitly truncated to unsigned type [-Woverflow]
 #define DMA_ERROR_CODE (~(dma_addr_t)0x0)
                        ^
/src/linux/lib/iommu-common.c:127:10: note: in expansion of macro
'DMA_ERROR_CODE'
   return DMA_ERROR_CODE;

If I am not mistaken, dma_addr_t on both Sparc variants is always
32-bit, so we don't need necessarily to adjust the callers, just the
function itself.
This still isn't right (since now the types in the caller mismatch,
though in a different way the compiler does not complain about), but
works as a fix for the 4.3 release. I couldn't spot any warnings on
the architectures I managed to compile.

Compile tested on Sparc, Sparc64, PowerPC64, ARM, ARM64, x86.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/linux/iommu-common.h | 12 ++++++------
 lib/iommu-common.c           | 15 ++++++++-------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/linux/iommu-common.h b/include/linux/iommu-common.h
index bbced83..fdff585 100644
--- a/include/linux/iommu-common.h
+++ b/include/linux/iommu-common.h
@@ -37,12 +37,12 @@ extern void iommu_tbl_pool_init(struct iommu_map_table *iommu,
 				bool large_pool, u32 npools,
 				bool skip_span_boundary_check);
 
-extern unsigned long iommu_tbl_range_alloc(struct device *dev,
-					   struct iommu_map_table *iommu,
-					   unsigned long npages,
-					   unsigned long *handle,
-					   unsigned long mask,
-					   unsigned int align_order);
+extern dma_addr_t iommu_tbl_range_alloc(struct device *dev,
+					struct iommu_map_table *iommu,
+					unsigned long npages,
+					unsigned long *handle,
+					unsigned long mask,
+					unsigned int align_order);
 
 extern void iommu_tbl_range_free(struct iommu_map_table *iommu,
 				 u64 dma_addr, unsigned long npages,
diff --git a/lib/iommu-common.c b/lib/iommu-common.c
index ff19f66..6f0324e 100644
--- a/lib/iommu-common.c
+++ b/lib/iommu-common.c
@@ -99,15 +99,16 @@ void iommu_tbl_pool_init(struct iommu_map_table *iommu,
 }
 EXPORT_SYMBOL(iommu_tbl_pool_init);
 
-unsigned long iommu_tbl_range_alloc(struct device *dev,
-				struct iommu_map_table *iommu,
-				unsigned long npages,
-				unsigned long *handle,
-				unsigned long mask,
-				unsigned int align_order)
+dma_addr_t iommu_tbl_range_alloc(struct device *dev,
+				 struct iommu_map_table *iommu,
+				 unsigned long npages,
+				 unsigned long *handle,
+				 unsigned long mask,
+				 unsigned int align_order)
 {
 	unsigned int pool_hash = __this_cpu_read(iommu_hash_common);
-	unsigned long n, end, start, limit, boundary_size;
+	dma_addr_t n;
+	unsigned long end, start, limit, boundary_size;
 	struct iommu_pool *pool;
 	int pass = 0;
 	unsigned int pool_nr;
-- 
2.5.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64
  2015-09-18  9:03   ` Andre Przywara
  2015-09-18  9:09     ` [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc() Andre Przywara
@ 2015-09-18 17:09     ` David Miller
  2015-09-18 17:28       ` Andre Przywara
  1 sibling, 1 reply; 11+ messages in thread
From: David Miller @ 2015-09-18 17:09 UTC (permalink / raw)
  To: andre.przywara; +Cc: joro, sparclinux, linux-kernel, linux-arm-kernel

From: Andre Przywara <andre.przywara@arm.com>
Date: Fri, 18 Sep 2015 10:03:40 +0100

> Or do you know of a simpler way to fixing it properly which could make
> it still into 4.3?

All I worry about is that if you take it out of the build, doing
the proper fix will suddenly become very low priority and be
forgotten about completely.

Whereas if it actually blocks the build for someone, it's a high
priority issue to address.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64
  2015-09-18 17:09     ` [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 David Miller
@ 2015-09-18 17:28       ` Andre Przywara
  2015-09-21 17:17         ` [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types Andre Przywara
  0 siblings, 1 reply; 11+ messages in thread
From: Andre Przywara @ 2015-09-18 17:28 UTC (permalink / raw)
  To: David Miller; +Cc: joro, sparclinux, linux-kernel, linux-arm-kernel

Hi David,

On 18/09/15 18:09, David Miller wrote:
> From: Andre Przywara <andre.przywara@arm.com>
> Date: Fri, 18 Sep 2015 10:03:40 +0100
> 
>> Or do you know of a simpler way to fixing it properly which could make
>> it still into 4.3?
> 
> All I worry about is that if you take it out of the build, doing
> the proper fix will suddenly become very low priority and be
> forgotten about completely.
> 
> Whereas if it actually blocks the build for someone, it's a high
> priority issue to address.

I see. What about if I provide the two patches at the same time? An easy
fix for 4.3 and the proper fix for 4.4? You can then even choose to take
the proper fix for 4.3 if it's reasonably low risk.
I will try sending something on Monday.

Cheers,
Andre.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types
  2015-09-18 17:28       ` Andre Przywara
@ 2015-09-21 17:17         ` Andre Przywara
  2015-10-05  9:54           ` Andre Przywara
  0 siblings, 1 reply; 11+ messages in thread
From: Andre Przywara @ 2015-09-21 17:17 UTC (permalink / raw)
  To: David S. Miller; +Cc: Joerg Roedel, sparclinux, linux-kernel, linux-arm-kernel

With DMA_ERROR_CODE now being dma_addr_t in most architectures, it
turned out that iommu_tbl_range_alloc (defined in lib/iommu-common.c)
is actually using a wrong return type.
This was easily fixed in a previous patch, but now the types in the
callers do not match anymore.
This patch fixes the obvious mismatches to allow sane comparisons with
the error return value.
Compile-tested on sparc, sparc64, x86, ARM, arm64.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi David,

as promised my first naive try on fixing the callers of
iommu_tbl_range_alloc() as well. This goes on top of the return type
fix I sent on Friday.
Please let me know if that makes sense or whether I am looking in the
totally wrong direction.

Cheers,
Andre.
 arch/sparc/kernel/iommu.c     | 5 +++--
 arch/sparc/kernel/ldc.c       | 5 +++--
 arch/sparc/kernel/pci_sun4v.c | 7 ++++---
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/sparc/kernel/iommu.c b/arch/sparc/kernel/iommu.c
index 5320689..7d04a87 100644
--- a/arch/sparc/kernel/iommu.c
+++ b/arch/sparc/kernel/iommu.c
@@ -157,7 +157,7 @@ static inline iopte_t *alloc_npages(struct device *dev,
 				    struct iommu *iommu,
 				    unsigned long npages)
 {
-	unsigned long entry;
+	dma_addr_t entry;
 
 	entry = iommu_tbl_range_alloc(dev, &iommu->tbl, npages, NULL,
 				      (unsigned long)(-1), 0);
@@ -476,7 +476,8 @@ static int dma_4u_map_sg(struct device *dev, struct scatterlist *sglist,
 				  IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
 	base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
 	for_each_sg(sglist, s, nelems, i) {
-		unsigned long paddr, npages, entry, out_entry = 0, slen;
+		unsigned long paddr, npages, slen;
+		dma_addr_t entry, out_entry = 0;
 		iopte_t *base;
 
 		slen = s->length;
diff --git a/arch/sparc/kernel/ldc.c b/arch/sparc/kernel/ldc.c
index 1ae5eb1..41e79cb 100644
--- a/arch/sparc/kernel/ldc.c
+++ b/arch/sparc/kernel/ldc.c
@@ -10,6 +10,7 @@
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/string.h>
+#include <linux/dma-mapping.h>
 #include <linux/scatterlist.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
@@ -1949,11 +1950,11 @@ static u64 make_cookie(u64 index, u64 pgsz_code, u64 page_offset)
 static struct ldc_mtable_entry *alloc_npages(struct ldc_iommu *iommu,
 					     unsigned long npages)
 {
-	long entry;
+	dma_addr_t entry;
 
 	entry = iommu_tbl_range_alloc(NULL, &iommu->iommu_map_table,
 				      npages, NULL, (unsigned long)-1, 0);
-	if (unlikely(entry < 0))
+	if (unlikely(entry == DMA_ERROR_CODE))
 		return NULL;
 
 	return iommu->page_table + entry;
diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
index d2fe57d..f86902f 100644
--- a/arch/sparc/kernel/pci_sun4v.c
+++ b/arch/sparc/kernel/pci_sun4v.c
@@ -136,7 +136,7 @@ static void *dma_4v_alloc_coherent(struct device *dev, size_t size,
 	struct iommu *iommu;
 	struct page *page;
 	void *ret;
-	long entry;
+	dma_addr_t entry;
 	int nid;
 
 	size = IO_PAGE_ALIGN(size);
@@ -242,7 +242,7 @@ static dma_addr_t dma_4v_map_page(struct device *dev, struct page *page,
 	unsigned long i, base_paddr;
 	u32 bus_addr, ret;
 	unsigned long prot;
-	long entry;
+	dma_addr_t entry;
 
 	iommu = dev->archdata.iommu;
 
@@ -361,7 +361,8 @@ static int dma_4v_map_sg(struct device *dev, struct scatterlist *sglist,
 				  IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
 	base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
 	for_each_sg(sglist, s, nelems, i) {
-		unsigned long paddr, npages, entry, out_entry = 0, slen;
+		unsigned long paddr, npages, slen;
+		dma_addr_t entry, out_entry = 0;
 
 		slen = s->length;
 		/* Sanity check */
-- 
2.5.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types
  2015-09-21 17:17         ` [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types Andre Przywara
@ 2015-10-05  9:54           ` Andre Przywara
  2015-11-04 16:38             ` David Miller
  0 siblings, 1 reply; 11+ messages in thread
From: Andre Przywara @ 2015-10-05  9:54 UTC (permalink / raw)
  To: David S. Miller; +Cc: Joerg Roedel, sparclinux, linux-kernel, linux-arm-kernel

Hi David,

On 21/09/15 18:17, Andre Przywara wrote:
> With DMA_ERROR_CODE now being dma_addr_t in most architectures, it
> turned out that iommu_tbl_range_alloc (defined in lib/iommu-common.c)
> is actually using a wrong return type.
> This was easily fixed in a previous patch, but now the types in the
> callers do not match anymore.
> This patch fixes the obvious mismatches to allow sane comparisons with
> the error return value.
> Compile-tested on sparc, sparc64, x86, ARM, arm64.

Is there any news on that issue? Are you willing to take either of my
patches to fix the compile warnings I see on ARM with LPAE enabled?

Cheers,
Andre.

> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi David,
> 
> as promised my first naive try on fixing the callers of
> iommu_tbl_range_alloc() as well. This goes on top of the return type
> fix I sent on Friday.
> Please let me know if that makes sense or whether I am looking in the
> totally wrong direction.
> 
> Cheers,
> Andre.
>  arch/sparc/kernel/iommu.c     | 5 +++--
>  arch/sparc/kernel/ldc.c       | 5 +++--
>  arch/sparc/kernel/pci_sun4v.c | 7 ++++---
>  3 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/sparc/kernel/iommu.c b/arch/sparc/kernel/iommu.c
> index 5320689..7d04a87 100644
> --- a/arch/sparc/kernel/iommu.c
> +++ b/arch/sparc/kernel/iommu.c
> @@ -157,7 +157,7 @@ static inline iopte_t *alloc_npages(struct device *dev,
>  				    struct iommu *iommu,
>  				    unsigned long npages)
>  {
> -	unsigned long entry;
> +	dma_addr_t entry;
>  
>  	entry = iommu_tbl_range_alloc(dev, &iommu->tbl, npages, NULL,
>  				      (unsigned long)(-1), 0);
> @@ -476,7 +476,8 @@ static int dma_4u_map_sg(struct device *dev, struct scatterlist *sglist,
>  				  IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
>  	base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
>  	for_each_sg(sglist, s, nelems, i) {
> -		unsigned long paddr, npages, entry, out_entry = 0, slen;
> +		unsigned long paddr, npages, slen;
> +		dma_addr_t entry, out_entry = 0;
>  		iopte_t *base;
>  
>  		slen = s->length;
> diff --git a/arch/sparc/kernel/ldc.c b/arch/sparc/kernel/ldc.c
> index 1ae5eb1..41e79cb 100644
> --- a/arch/sparc/kernel/ldc.c
> +++ b/arch/sparc/kernel/ldc.c
> @@ -10,6 +10,7 @@
>  #include <linux/delay.h>
>  #include <linux/errno.h>
>  #include <linux/string.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/scatterlist.h>
>  #include <linux/interrupt.h>
>  #include <linux/list.h>
> @@ -1949,11 +1950,11 @@ static u64 make_cookie(u64 index, u64 pgsz_code, u64 page_offset)
>  static struct ldc_mtable_entry *alloc_npages(struct ldc_iommu *iommu,
>  					     unsigned long npages)
>  {
> -	long entry;
> +	dma_addr_t entry;
>  
>  	entry = iommu_tbl_range_alloc(NULL, &iommu->iommu_map_table,
>  				      npages, NULL, (unsigned long)-1, 0);
> -	if (unlikely(entry < 0))
> +	if (unlikely(entry == DMA_ERROR_CODE))
>  		return NULL;
>  
>  	return iommu->page_table + entry;
> diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
> index d2fe57d..f86902f 100644
> --- a/arch/sparc/kernel/pci_sun4v.c
> +++ b/arch/sparc/kernel/pci_sun4v.c
> @@ -136,7 +136,7 @@ static void *dma_4v_alloc_coherent(struct device *dev, size_t size,
>  	struct iommu *iommu;
>  	struct page *page;
>  	void *ret;
> -	long entry;
> +	dma_addr_t entry;
>  	int nid;
>  
>  	size = IO_PAGE_ALIGN(size);
> @@ -242,7 +242,7 @@ static dma_addr_t dma_4v_map_page(struct device *dev, struct page *page,
>  	unsigned long i, base_paddr;
>  	u32 bus_addr, ret;
>  	unsigned long prot;
> -	long entry;
> +	dma_addr_t entry;
>  
>  	iommu = dev->archdata.iommu;
>  
> @@ -361,7 +361,8 @@ static int dma_4v_map_sg(struct device *dev, struct scatterlist *sglist,
>  				  IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
>  	base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
>  	for_each_sg(sglist, s, nelems, i) {
> -		unsigned long paddr, npages, entry, out_entry = 0, slen;
> +		unsigned long paddr, npages, slen;
> +		dma_addr_t entry, out_entry = 0;
>  
>  		slen = s->length;
>  		/* Sanity check */
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc()
  2015-09-18  9:09     ` [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc() Andre Przywara
@ 2015-10-21 19:12       ` Geert Uytterhoeven
  2015-10-21 19:56         ` Sowmini Varadhan
  0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2015-10-21 19:12 UTC (permalink / raw)
  To: Andre Przywara, Joerg Roedel
  Cc: David S. Miller, sparclinux, linux-kernel, linux-arm-kernel,
	Parisc List, iommu

Hi Andre, Jörg,

On Fri, Sep 18, 2015 at 11:09 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> Though iommu_tbl_range_alloc() is only used by Sparc code, the
> function itself lives in lib/iommu-common.c and is thus included in
> other architecture's code as well.
> When compiled on a 32-bit architecture using 64-bit DMA addresses
> (ARM with LPAE), there is a compiler warning about a type mismatch
> between dma_addr_t and the return type of this function:
>
> In file included from /src/linux/include/linux/dma-mapping.h:86:0,
>                  from /src/linux/lib/iommu-common.c:11:
> /src/linux/lib/iommu-common.c: In function 'iommu_tbl_range_alloc':
> /src/linux/arch/arm/include/asm/dma-mapping.h:16:24: warning: large integer implicitly truncated to unsigned type [-Woverflow]
>  #define DMA_ERROR_CODE (~(dma_addr_t)0x0)
>                         ^
> /src/linux/lib/iommu-common.c:127:10: note: in expansion of macro
> 'DMA_ERROR_CODE'
>    return DMA_ERROR_CODE;

While I welcome a fix for this annoying warning on arm...

> --- a/lib/iommu-common.c
> +++ b/lib/iommu-common.c
> @@ -99,15 +99,16 @@ void iommu_tbl_pool_init(struct iommu_map_table *iommu,
>  }
>  EXPORT_SYMBOL(iommu_tbl_pool_init);
>
> -unsigned long iommu_tbl_range_alloc(struct device *dev,
> -                               struct iommu_map_table *iommu,
> -                               unsigned long npages,
> -                               unsigned long *handle,
> -                               unsigned long mask,
> -                               unsigned int align_order)
> +dma_addr_t iommu_tbl_range_alloc(struct device *dev,
> +                                struct iommu_map_table *iommu,
> +                                unsigned long npages,
> +                                unsigned long *handle,
> +                                unsigned long mask,
> +                                unsigned int align_order)
>  {
>         unsigned int pool_hash = __this_cpu_read(iommu_hash_common);
> -       unsigned long n, end, start, limit, boundary_size;
> +       dma_addr_t n;
> +       unsigned long end, start, limit, boundary_size;

... this doesn't look like the right fix.

Apparently 64-bit parisc doesn't set ARCH_DMA_ADDR_T_64BIT, hence assigning
(64-bit) "unsigned long" to (32-bit) dma_addr_t will truncate the address.

Does this function really need to return DMA_ERROR_CODE in case of
failure?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc()
  2015-10-21 19:12       ` Geert Uytterhoeven
@ 2015-10-21 19:56         ` Sowmini Varadhan
  0 siblings, 0 replies; 11+ messages in thread
From: Sowmini Varadhan @ 2015-10-21 19:56 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Andre Przywara, Joerg Roedel, David S. Miller, sparclinux,
	linux-kernel, linux-arm-kernel, Parisc List, iommu

On (10/21/15 21:12), Geert Uytterhoeven wrote:
> 
> Does this function really need to return DMA_ERROR_CODE in case of
> failure?

fwiw, this came up very early in code review:

  http://marc.info/?l=linux-sparc&m=142613044704833&w=2

--Sowmini

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types
  2015-10-05  9:54           ` Andre Przywara
@ 2015-11-04 16:38             ` David Miller
  0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2015-11-04 16:38 UTC (permalink / raw)
  To: andre.przywara; +Cc: joro, sparclinux, linux-kernel, linux-arm-kernel

From: Andre Przywara <andre.przywara@arm.com>
Date: Mon, 5 Oct 2015 10:54:05 +0100

> On 21/09/15 18:17, Andre Przywara wrote:
>> With DMA_ERROR_CODE now being dma_addr_t in most architectures, it
>> turned out that iommu_tbl_range_alloc (defined in lib/iommu-common.c)
>> is actually using a wrong return type.
>> This was easily fixed in a previous patch, but now the types in the
>> callers do not match anymore.
>> This patch fixes the obvious mismatches to allow sane comparisons with
>> the error return value.
>> Compile-tested on sparc, sparc64, x86, ARM, arm64.
> 
> Is there any news on that issue? Are you willing to take either of my
> patches to fix the compile warnings I see on ARM with LPAE enabled?

Sorry for the late response, I was travelling/conferencing and didn't
have a lot of time for issues like this.

I'm going to fix iommu_tbl_range_alloc()'s return value semantics.

The thing it is returning is a page table index, not a dma_addr_t
address.  So it is appropriate to create a special error indication
value for this, independant of DMA_ERROR_CODE.

I got this wrong during my initial review of these changes.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-11-04 16:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-17  8:40 [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 Andre Przywara
2015-09-17 18:38 ` David Miller
2015-09-18  9:03   ` Andre Przywara
2015-09-18  9:09     ` [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc() Andre Przywara
2015-10-21 19:12       ` Geert Uytterhoeven
2015-10-21 19:56         ` Sowmini Varadhan
2015-09-18 17:09     ` [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 David Miller
2015-09-18 17:28       ` Andre Przywara
2015-09-21 17:17         ` [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types Andre Przywara
2015-10-05  9:54           ` Andre Przywara
2015-11-04 16:38             ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).