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

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

[Linus Walleij]

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(-)

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

[PATCH 2/2 v3] parisc: Use the generic IO helpers

[Linus Walleij]

This enables the parisc to use <asm-generic/io.h> to fill in the
missing (undefined) [read|write]sq I/O accessor functions.

This is needed if parisc[64] ever wants to uses CONFIG_REGMAP_MMIO
which has been patches to use accelerated _noinc accessors
such as readsq/writesq that parisc64, while being a 64bit platform,
as of now not yet provide.

This comes with the requirement that everything the architecture
already provides needs to be defined, rather than just being,
say, static inline functions.

Bite the bullet and just provide the definitions and make it work.
Compile-tested on parisc32 and parisc64. Drop some of the __raw
functions that now get implemented in <asm-generic/io.h>.

Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/linux-arm-kernel/62fcc351.hAyADB%2FY8JTxz+kh%25lkp@intel.com/
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: Mark Brown <broonie@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 v2->v3:
- Prepend a patch removing all 64bit accessor use before the
  conversion.
- Drop the idea to create 64bit accesories on 32bit systems.
ChangeLog v1->v2:
- Missed a dangling onelineer adding pci_iounmap in my tree.
- Just drop all the __raw_read[b|w|l|q] and __raw_write[b|w|l|q]
  as these are identical to what <asm-generic/io.h> provides.
- Just drop the local read[b|w|l|q] and write[b|w|l|q] as well
  as the generic versions probably do a better job: this adds
  the __io_ar()/__io_bw() barriers as they should probably be
  there, we have just been lucky.
- Drop a comment that parisc does not select GENERIC_IOMAP.
- In arch/parisc/lib/iomap.c the .read64 and .read64be operations
  were defined unconditionally and drivers/parisc/sba_iommu.c
  also expect readq/writeq to be available so add a special
  parameter to <asm-generic/io.h> to make sure we get these
  also on 32bit parisc.
- The changes to <asm-generic/io.h> probably makes this patch
  a candidate for the arch tree rather than parisc.
---
 arch/parisc/include/asm/io.h | 132 ++++++++++++-----------------------
 1 file changed, 43 insertions(+), 89 deletions(-)

diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h
index 42ffb60a6ea9..37580ad8d5bd 100644
--- a/arch/parisc/include/asm/io.h
+++ b/arch/parisc/include/asm/io.h
@@ -128,98 +128,16 @@ static inline void gsc_writeq(unsigned long long val, unsigned long addr)
 void __iomem *ioremap(unsigned long offset, unsigned long size);
 #define ioremap_wc			ioremap
 #define ioremap_uc			ioremap
+#define pci_iounmap			pci_iounmap
 
 extern void iounmap(const volatile void __iomem *addr);
 
-static inline unsigned char __raw_readb(const volatile void __iomem *addr)
-{
-	return (*(volatile unsigned char __force *) (addr));
-}
-static inline unsigned short __raw_readw(const volatile void __iomem *addr)
-{
-	return *(volatile unsigned short __force *) addr;
-}
-static inline unsigned int __raw_readl(const volatile void __iomem *addr)
-{
-	return *(volatile unsigned int __force *) addr;
-}
-static inline unsigned long long __raw_readq(const volatile void __iomem *addr)
-{
-	return *(volatile unsigned long long __force *) addr;
-}
-
-static inline void __raw_writeb(unsigned char b, volatile void __iomem *addr)
-{
-	*(volatile unsigned char __force *) addr = b;
-}
-static inline void __raw_writew(unsigned short b, volatile void __iomem *addr)
-{
-	*(volatile unsigned short __force *) addr = b;
-}
-static inline void __raw_writel(unsigned int b, volatile void __iomem *addr)
-{
-	*(volatile unsigned int __force *) addr = b;
-}
-static inline void __raw_writeq(unsigned long long b, volatile void __iomem *addr)
-{
-	*(volatile unsigned long long __force *) addr = b;
-}
-
-static inline unsigned char readb(const volatile void __iomem *addr)
-{
-	return __raw_readb(addr);
-}
-static inline unsigned short readw(const volatile void __iomem *addr)
-{
-	return le16_to_cpu((__le16 __force) __raw_readw(addr));
-}
-static inline unsigned int readl(const volatile void __iomem *addr)
-{
-	return le32_to_cpu((__le32 __force) __raw_readl(addr));
-}
-static inline unsigned long long readq(const volatile void __iomem *addr)
-{
-	return le64_to_cpu((__le64 __force) __raw_readq(addr));
-}
-
-static inline void writeb(unsigned char b, volatile void __iomem *addr)
-{
-	__raw_writeb(b, addr);
-}
-static inline void writew(unsigned short w, volatile void __iomem *addr)
-{
-	__raw_writew((__u16 __force) cpu_to_le16(w), addr);
-}
-static inline void writel(unsigned int l, volatile void __iomem *addr)
-{
-	__raw_writel((__u32 __force) cpu_to_le32(l), addr);
-}
-static inline void writeq(unsigned long long q, volatile void __iomem *addr)
-{
-	__raw_writeq((__u64 __force) cpu_to_le64(q), addr);
-}
-
-#define	readb	readb
-#define	readw	readw
-#define	readl	readl
-#define readq	readq
-#define writeb	writeb
-#define writew	writew
-#define writel	writel
-#define writeq	writeq
-
-#define readb_relaxed(addr)	readb(addr)
-#define readw_relaxed(addr)	readw(addr)
-#define readl_relaxed(addr)	readl(addr)
-#define readq_relaxed(addr)	readq(addr)
-#define writeb_relaxed(b, addr)	writeb(b, addr)
-#define writew_relaxed(w, addr)	writew(w, addr)
-#define writel_relaxed(l, addr)	writel(l, addr)
-#define writeq_relaxed(q, addr)	writeq(q, addr)
-
 void memset_io(volatile void __iomem *addr, unsigned char val, int count);
 void memcpy_fromio(void *dst, const volatile void __iomem *src, int count);
 void memcpy_toio(volatile void __iomem *dst, const void *src, int count);
+#define memset_io memset_io
+#define memcpy_fromio memcpy_fromio
+#define memcpy_toio memcpy_toio
 
 /* Port-space IO */
 
@@ -241,10 +159,15 @@ extern void eisa_out32(unsigned int data, unsigned short port);
 extern unsigned char inb(int addr);
 extern unsigned short inw(int addr);
 extern unsigned int inl(int addr);
-
 extern void outb(unsigned char b, int addr);
 extern void outw(unsigned short b, int addr);
 extern void outl(unsigned int b, int addr);
+#define inb inb
+#define inw inw
+#define inl inl
+#define outb outb
+#define outw outw
+#define outl outl
 #elif defined(CONFIG_EISA)
 #define inb eisa_in8
 #define inw eisa_in16
@@ -270,7 +193,9 @@ static inline int inl(unsigned long addr)
 	BUG();
 	return -1;
 }
-
+#define inb inb
+#define inw inw
+#define inl inl
 #define outb(x, y)	({(void)(x); (void)(y); BUG(); 0;})
 #define outw(x, y)	({(void)(x); (void)(y); BUG(); 0;})
 #define outl(x, y)	({(void)(x); (void)(y); BUG(); 0;})
@@ -285,7 +210,12 @@ extern void insl (unsigned long port, void *dst, unsigned long count);
 extern void outsb (unsigned long port, const void *src, unsigned long count);
 extern void outsw (unsigned long port, const void *src, unsigned long count);
 extern void outsl (unsigned long port, const void *src, unsigned long count);
-
+#define insb insb
+#define insw insw
+#define insl insl
+#define outsb outsb
+#define outsw outsw
+#define outsl outsl
 
 /* IO Port space is :      BBiiii   where BB is HBA number. */
 #define IO_SPACE_LIMIT 0x00ffffff
@@ -307,6 +237,28 @@ extern void iowrite64(u64 val, void __iomem *addr);
 extern void iowrite64be(u64 val, void __iomem *addr);
 
 #include <asm-generic/iomap.h>
+/*
+ * These get provided from <asm-generic/iomap.h> since parisc does not
+ * select GENERIC_IOMAP.
+ */
+#define ioport_map ioport_map
+#define ioport_unmap ioport_unmap
+#define ioread8 ioread8
+#define ioread16 ioread16
+#define ioread32 ioread32
+#define ioread16be ioread16be
+#define ioread32be ioread32be
+#define iowrite8 iowrite8
+#define iowrite16 iowrite16
+#define iowrite32 iowrite32
+#define iowrite16be iowrite16be
+#define iowrite32be iowrite32be
+#define ioread8_rep ioread8_rep
+#define ioread16_rep ioread16_rep
+#define ioread32_rep ioread32_rep
+#define iowrite8_rep iowrite8_rep
+#define iowrite16_rep iowrite16_rep
+#define iowrite32_rep iowrite32_rep
 
 /*
  * Convert a physical pointer to a virtual kernel pointer for /dev/mem
@@ -316,4 +268,6 @@ extern void iowrite64be(u64 val, void __iomem *addr);
 
 extern int devmem_is_allowed(unsigned long pfn);
 
+#include <asm-generic/io.h>
+
 #endif
-- 
2.37.2

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

[Arnd Bergmann]

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

Re: [PATCH 2/2 v3] parisc: Use the generic IO helpers

[Arnd Bergmann]

On Fri, Sep 2, 2022, at 9:51 AM, Linus Walleij wrote:
> This enables the parisc to use <asm-generic/io.h> to fill in the
> missing (undefined) [read|write]sq I/O accessor functions.
>
> This is needed if parisc[64] ever wants to uses CONFIG_REGMAP_MMIO
> which has been patches to use accelerated _noinc accessors
> such as readsq/writesq that parisc64, while being a 64bit platform,
> as of now not yet provide.
>
> This comes with the requirement that everything the architecture
> already provides needs to be defined, rather than just being,
> say, static inline functions.
>
> Bite the bullet and just provide the definitions and make it work.
> Compile-tested on parisc32 and parisc64. Drop some of the __raw
> functions that now get implemented in <asm-generic/io.h>.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Link: 
> https://lore.kernel.org/linux-arm-kernel/62fcc351.hAyADB%2FY8JTxz+kh%25lkp@intel.com/
> 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: Mark Brown <broonie@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>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

>  #include <asm-generic/iomap.h>
> +/*
> + * These get provided from <asm-generic/iomap.h> since parisc does not
> + * select GENERIC_IOMAP.
> + */
> +#define ioport_map ioport_map
> +#define ioport_unmap ioport_unmap
> +#define ioread8 ioread8

[nitpicking] 's/since/though/' or similar:

As discussed, the asm-generic/iomap.h declarations are meant to
be used with GENERIC_IOMAP, parisc just uses them anyway despite
having its own abstraction.

     Arnd