Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1226758 > unrolled thread

[PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64

Back to article view | Back to linux.kernel

Contents

  [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 Andre Przywara <andre.przywara@arm.com> - 2015-09-17 10:40 +0200
    Re: [PATCH] iommu-common: only compile lib/iommu_common.c for  Sparc64 David Miller <davem@davemloft.net> - 2015-09-17 20:40 +0200
      Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 Andre Przywara <andre.przywara@arm.com> - 2015-09-18 11:10 +0200
        [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc() Andre Przywara <andre.przywara@arm.com> - 2015-09-18 11:10 +0200
        Re: [PATCH] iommu-common: only compile lib/iommu_common.c for  Sparc64 David Miller <davem@davemloft.net> - 2015-09-18 19:10 +0200
          Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64 Andre Przywara <andre.przywara@arm.com> - 2015-09-18 19:30 +0200
            [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types Andre Przywara <andre.przywara@arm.com> - 2015-09-21 19:20 +0200
              Re: [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types Andre Przywara <andre.przywara@arm.com> - 2015-10-05 12:00 +0200

#1226758 — [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64

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

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

[toc] | [next] | [standalone]

#1227310 — Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64

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

[toc] | [prev] | [next] | [standalone]

#1227693

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

[toc] | [prev] | [next] | [standalone]

#1227696 — [PATCH] iommu-common: fix return type of iommu_tbl_range_alloc()

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

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

[toc] | [prev] | [next] | [standalone]

#1228189 — Re: [PATCH] iommu-common: only compile lib/iommu_common.c for Sparc64

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

[toc] | [prev] | [next] | [standalone]

#1228197

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

[toc] | [prev] | [next] | [standalone]

#1229590 — [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types

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

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

[toc] | [prev] | [next] | [standalone]

#1239400 — Re: [PATCH] sparc(64)/iommu: fixup iommu_tbl_range_alloc() types

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 */
> 
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web