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

["Arnd Bergmann" <arnd@arndb.de>]

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