diff mbox series

[1/2,v3] parisc: Remove 64bit access on 32bit machines

Message ID 20220902075122.339753-1-linus.walleij@linaro.org (mailing list archive)
State Superseded
Headers show
Series [1/2,v3] parisc: Remove 64bit access on 32bit machines | expand

Commit Message

Linus Walleij Sept. 2, 2022, 7:51 a.m. UTC 
The parisc was using some readq/writeq accessors without special
considerations as to what will happen on 32bit CPUs if you do
this. Maybe we have been lucky that it "just worked" on 32bit
due to the compiler behaviour, or the code paths were never
executed.

Fix the two offending code sites like this:

arch/parisc/lib/iomap.c:

- Put ifdefs around the 64bit accessors and make sure
  that ioread64, ioread64be, iowrite64 and iowrite64be
  are not available on 32bit builds.

- Also fold in a bug fix where 64bit access was by
  mistake using 32bit writel() accessors rather
  than 64bit writeq().

drivers/parisc/sba_iommu.c:

- Access any 64bit registers using ioread64_lo_hi()
  or iowrite64_lo_hi().

Cc: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: John David Anglin <dave.anglin@bell.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v0->v3:
- New patch fixing the use of IO accessors
- If the parisc people have no strong preference maybe Arnd
  can merge this through the arch tree with patch 2/2.
---
 arch/parisc/lib/iomap.c    | 24 ++++++++++++++++++++++--
 drivers/parisc/sba_iommu.c | 12 ++++++++++--
 2 files changed, 32 insertions(+), 4 deletions(-)

Comments

Arnd Bergmann Sept. 2, 2022, 8:34 a.m. UTC | #1 
On Fri, Sep 2, 2022, at 9:51 AM, Linus Walleij wrote:
> The parisc was using some readq/writeq accessors without special
> considerations as to what will happen on 32bit CPUs if you do
> this. Maybe we have been lucky that it "just worked" on 32bit
> due to the compiler behaviour, or the code paths were never
> executed.
> ...

Patch looks correct to me and does address the issue.
A few minor points though:

> diff --git a/arch/parisc/lib/iomap.c b/arch/parisc/lib/iomap.c
> index 860385058085..d3d57119df64 100644
> --- a/arch/parisc/lib/iomap.c
> +++ b/arch/parisc/lib/iomap.c
> @@ -48,15 +48,19 @@ struct iomap_ops {
>  	unsigned int (*read16be)(const void __iomem *);
>  	unsigned int (*read32)(const void __iomem *);
>  	unsigned int (*read32be)(const void __iomem *);
> +#ifdef CONFIG_64BIT
>  	u64 (*read64)(const void __iomem *);
>  	u64 (*read64be)(const void __iomem *);
> +#endif
>  	void (*write8)(u8, void __iomem *);
>  	void (*write16)(u16, void __iomem *);
>  	void (*write16be)(u16, void __iomem *);
>  	void (*write32)(u32, void __iomem *);
>  	void (*write32be)(u32, void __iomem *);
> +#ifdef CONFIG_64BIT
>  	void (*write64)(u64, void __iomem *);
>  	void (*write64be)(u64, void __iomem *);
> +#endif
>  	void (*read8r)(const void __iomem *, void *, unsigned long);
>  	void (*read16r)(const void __iomem *, void *, unsigned long);
>  	void (*read32r)(const void __iomem *, void *, unsigned long);

I would not bother with the #ifdef checks in the structure definition,
but they don't hurt either, and we need the other ones anyway.
Up to you (or the maintainers).

> @@ -127,14 +128,21 @@ MODULE_PARM_DESC(sba_reserve_agpgart, "Reserve 
> half of IO pdir as AGPGART");
>  ** Superdome (in particular, REO) allows only 64-bit CSR accesses.
>  */
>  #define READ_REG32(addr)	readl(addr)
> -#define READ_REG64(addr)	readq(addr)
>  #define WRITE_REG32(val, addr)	writel((val), (addr))
> -#define WRITE_REG64(val, addr)	writeq((val), (addr))
> 
>  #ifdef CONFIG_64BIT
> +#define READ_REG64(addr)	readq(addr)
> +#define WRITE_REG64(val, addr)	writeq((val), (addr))
>  #define READ_REG(addr)		READ_REG64(addr)
>  #define WRITE_REG(value, addr)	WRITE_REG64(value, addr)
>  #else
> +/*
> + * The semantics of 64 register access on 32bit systems i undefined in the
> + * C standard, we hop the _lo_hi() macros will behave as the default compiled
> + * from C raw assignment.

typos: 'i' and 'hop'

The description is also slightly misleading, as it's not the
C standard specifically saying something about 64-bit accesses
on 32-bit targets being non-atomic, it's more that C doesn't
make a guarantee here at all, and the CPU probably can't do
double word accesses.

> +#define READ_REG64(addr)	ioread64_lo_hi(addr)
> +#define WRITE_REG64(val, addr)	iowrite64_lo_hi((val), (addr))


This change should not be needed here, as simply including the
io-64-nonatomic-lo-hi.h header gives you a working definition
of readq()/writeq(). Going through the ioread64/iowrite64 type
interfaces instead of the readq/writeq also gives you an extra
indirection that you don't really need. On Arm machines these
are the same, but they are different on parisc or x86 where
ioread multiplexes between PCI port and memory spaces.

      Arnd
diff mbox series

Patch

diff --git a/arch/parisc/lib/iomap.c b/arch/parisc/lib/iomap.c
index 860385058085..d3d57119df64 100644
--- a/arch/parisc/lib/iomap.c
+++ b/arch/parisc/lib/iomap.c
@@ -48,15 +48,19 @@  struct iomap_ops {
 	unsigned int (*read16be)(const void __iomem *);
 	unsigned int (*read32)(const void __iomem *);
 	unsigned int (*read32be)(const void __iomem *);
+#ifdef CONFIG_64BIT
 	u64 (*read64)(const void __iomem *);
 	u64 (*read64be)(const void __iomem *);
+#endif
 	void (*write8)(u8, void __iomem *);
 	void (*write16)(u16, void __iomem *);
 	void (*write16be)(u16, void __iomem *);
 	void (*write32)(u32, void __iomem *);
 	void (*write32be)(u32, void __iomem *);
+#ifdef CONFIG_64BIT
 	void (*write64)(u64, void __iomem *);
 	void (*write64be)(u64, void __iomem *);
+#endif
 	void (*read8r)(const void __iomem *, void *, unsigned long);
 	void (*read16r)(const void __iomem *, void *, unsigned long);
 	void (*read32r)(const void __iomem *, void *, unsigned long);
@@ -175,6 +179,7 @@  static unsigned int iomem_read32be(const void __iomem *addr)
 	return __raw_readl(addr);
 }
 
+#ifdef CONFIG_64BIT
 static u64 iomem_read64(const void __iomem *addr)
 {
 	return readq(addr);
@@ -184,6 +189,7 @@  static u64 iomem_read64be(const void __iomem *addr)
 {
 	return __raw_readq(addr);
 }
+#endif
 
 static void iomem_write8(u8 datum, void __iomem *addr)
 {
@@ -210,15 +216,17 @@  static void iomem_write32be(u32 datum, void __iomem *addr)
 	__raw_writel(datum, addr);
 }
 
+#ifdef CONFIG_64BIT
 static void iomem_write64(u64 datum, void __iomem *addr)
 {
-	writel(datum, addr);
+	writeq(datum, addr);
 }
 
 static void iomem_write64be(u64 datum, void __iomem *addr)
 {
-	__raw_writel(datum, addr);
+	__raw_writeq(datum, addr);
 }
+#endif
 
 static void iomem_read8r(const void __iomem *addr, void *dst, unsigned long count)
 {
@@ -274,15 +282,19 @@  static const struct iomap_ops iomem_ops = {
 	.read16be = iomem_read16be,
 	.read32 = iomem_read32,
 	.read32be = iomem_read32be,
+#ifdef CONFIG_64BIT
 	.read64 = iomem_read64,
 	.read64be = iomem_read64be,
+#endif
 	.write8 = iomem_write8,
 	.write16 = iomem_write16,
 	.write16be = iomem_write16be,
 	.write32 = iomem_write32,
 	.write32be = iomem_write32be,
+#ifdef CONFIG_64BIT
 	.write64 = iomem_write64,
 	.write64be = iomem_write64be,
+#endif
 	.read8r = iomem_read8r,
 	.read16r = iomem_read16r,
 	.read32r = iomem_read32r,
@@ -332,6 +344,7 @@  unsigned int ioread32be(const void __iomem *addr)
 	return *((u32 *)addr);
 }
 
+#ifdef CONFIG_64BIT
 u64 ioread64(const void __iomem *addr)
 {
 	if (unlikely(INDIRECT_ADDR(addr)))
@@ -345,6 +358,7 @@  u64 ioread64be(const void __iomem *addr)
 		return iomap_ops[ADDR_TO_REGION(addr)]->read64be(addr);
 	return *((u64 *)addr);
 }
+#endif
 
 u64 ioread64_lo_hi(const void __iomem *addr)
 {
@@ -411,6 +425,7 @@  void iowrite32be(u32 datum, void __iomem *addr)
 	}
 }
 
+#ifdef CONFIG_64BIT
 void iowrite64(u64 datum, void __iomem *addr)
 {
 	if (unlikely(INDIRECT_ADDR(addr))) {
@@ -428,6 +443,7 @@  void iowrite64be(u64 datum, void __iomem *addr)
 		*((u64 *)addr) = datum;
 	}
 }
+#endif
 
 void iowrite64_lo_hi(u64 val, void __iomem *addr)
 {
@@ -544,8 +560,10 @@  EXPORT_SYMBOL(ioread16);
 EXPORT_SYMBOL(ioread16be);
 EXPORT_SYMBOL(ioread32);
 EXPORT_SYMBOL(ioread32be);
+#ifdef CONFIG_64BIT
 EXPORT_SYMBOL(ioread64);
 EXPORT_SYMBOL(ioread64be);
+#endif
 EXPORT_SYMBOL(ioread64_lo_hi);
 EXPORT_SYMBOL(ioread64_hi_lo);
 EXPORT_SYMBOL(iowrite8);
@@ -553,8 +571,10 @@  EXPORT_SYMBOL(iowrite16);
 EXPORT_SYMBOL(iowrite16be);
 EXPORT_SYMBOL(iowrite32);
 EXPORT_SYMBOL(iowrite32be);
+#ifdef CONFIG_64BIT
 EXPORT_SYMBOL(iowrite64);
 EXPORT_SYMBOL(iowrite64be);
+#endif
 EXPORT_SYMBOL(iowrite64_lo_hi);
 EXPORT_SYMBOL(iowrite64_hi_lo);
 EXPORT_SYMBOL(ioread8_rep);
diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
index 374b9199878d..79091a86103a 100644
--- a/drivers/parisc/sba_iommu.c
+++ b/drivers/parisc/sba_iommu.c
@@ -28,6 +28,7 @@ 
 #include <linux/dma-map-ops.h>
 #include <linux/scatterlist.h>
 #include <linux/iommu-helper.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
 
 #include <asm/byteorder.h>
 #include <asm/io.h>
@@ -127,14 +128,21 @@  MODULE_PARM_DESC(sba_reserve_agpgart, "Reserve half of IO pdir as AGPGART");
 ** Superdome (in particular, REO) allows only 64-bit CSR accesses.
 */
 #define READ_REG32(addr)	readl(addr)
-#define READ_REG64(addr)	readq(addr)
 #define WRITE_REG32(val, addr)	writel((val), (addr))
-#define WRITE_REG64(val, addr)	writeq((val), (addr))
 
 #ifdef CONFIG_64BIT
+#define READ_REG64(addr)	readq(addr)
+#define WRITE_REG64(val, addr)	writeq((val), (addr))
 #define READ_REG(addr)		READ_REG64(addr)
 #define WRITE_REG(value, addr)	WRITE_REG64(value, addr)
 #else
+/*
+ * The semantics of 64 register access on 32bit systems i undefined in the
+ * C standard, we hop the _lo_hi() macros will behave as the default compiled
+ * from C raw assignment.
+ */
+#define READ_REG64(addr)	ioread64_lo_hi(addr)
+#define WRITE_REG64(val, addr)	iowrite64_lo_hi((val), (addr))
 #define READ_REG(addr)		READ_REG32(addr)
 #define WRITE_REG(value, addr)	WRITE_REG32(value, addr)
 #endif