[PATCH 0/2] ARC io-accessors updates for 7.4

[PATCH 0/2] ARC io-accessors updates for 7.4

[Vineet Gupta]

Hi,

A couple of patches that I plan to send for v7.4.
One is an old issue and other a cleanup out of it.
CC'ing Arnd to ensure 2/2 is in the right direction.


Thx,
-Vineet:


Vineet Gupta (2):
  ARC: io: work around GCC PR127718 in __raw_write{w,l}()
  ARC: io: use asm-generic MMIO accessors

 arch/arc/include/asm/io.h | 134 ++++++--------------------------------
 1 file changed, 19 insertions(+), 115 deletions(-)

-- 
2.53.0

[PATCH 1/2] ARC: io: work around GCC PR127718 in __raw_write{w,l}()

[Vineet Gupta]

The __raw_write{w,l}() accessors use inline asm with an "m" operand for
the store destination. GCC's ARC backend lets "m" match any legitimate
address, including the register-indexed [b,c] forms that only loads
support: stores only have [reg], [reg,s9] and [limm] addressing. With
newer GCC (late-combine in GCC 15, combine in GCC 16), a writew() with a
scaled, non-constant offset gets

	stw.as r2,[r0,r1]

which the assembler rejects:

  Error: inappropriate arguments for opcode 'stw'

With GCC 16 this breaks even hsdk_defconfig (drivers/net/pcs/
pcs-xpcs-plat.c) and axs101_defconfig. allyesconfig additionally trips
in drivers/pinctrl/renesas/pinctrl-rza2.c and drivers/video/fbdev/
s3c-fb.c.

Work around it by passing the address in a register and using a fixed
[%1] address in the asm template, so the compiler can't pick an
addressing mode that stores don't support. This is the minimal fix.
The next patch drops the inline asm altogether.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=127718
Cc: stable@vger.kernel.org
Signed-off-by: Vineet Gupta <vgupta@kernel.org>
---
 arch/arc/include/asm/io.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
index 00171a212b3c..b4a3393d1bdc 100644
--- a/arch/arc/include/asm/io.h
+++ b/arch/arc/include/asm/io.h
@@ -133,10 +133,14 @@ static inline void __raw_writeb(u8 b, volatile void __iomem *addr)
 #define __raw_writew __raw_writew
 static inline void __raw_writew(u16 s, volatile void __iomem *addr)
 {
+	/*
+	 * Not an "m" operand: GCC can pick a register-indexed address for it,
+	 * which ST doesn't support (GCC PR target/127718).
+	 */
 	__asm__ __volatile__(
-	"	stw%U1 %0, %1	\n"
+	"	stw %0, [%1]	\n"
 	:
-	: "r" (s), "m" (*(volatile u16 __force *)addr)
+	: "r" (s), "r" (addr)
 	: "memory");
 
 }
@@ -144,10 +148,11 @@ static inline void __raw_writew(u16 s, volatile void __iomem *addr)
 #define __raw_writel __raw_writel
 static inline void __raw_writel(u32 w, volatile void __iomem *addr)
 {
+	/* See __raw_writew() above for why this doesn't use an "m" operand. */
 	__asm__ __volatile__(
-	"	st%U1 %0, %1	\n"
+	"	st %0, [%1]	\n"
 	:
-	: "r" (w), "m" (*(volatile u32 __force *)addr)
+	: "r" (w), "r" (addr)
 	: "memory");
 
 }
-- 
2.53.0

[PATCH 2/2] ARC: io: use asm-generic MMIO accessors

[Vineet Gupta]

ARC MMIO accessors use inline asm, which isn't really needed. It was
introduced with the original port back in 2013.

As the previous patch showed, inline asm can be error-prone. Letting the
compiler emit the accesses also allows 16-bit encodings, scaled-index
loads and folding constant offsets (s9) into the access.

Without the asm, ARC's __raw_{read,write}{b,w,l}(), *_relaxed(),
{read,write}{b,w,l}() and io{read,write}{16,32}be() are the same as the
asm-generic/io.h versions, so drop them and use the generic ones.

Two things stay ARC-specific:

- The barriers, now hooked into the generic accessors through
  __io_ar()/__io_bw(). On ARCv2 they remain dmb (rmb()/wmb()).
  On ARCompact they were empty, relying on the asm "memory" clobber
  as a compiler barrier, so make them barrier() explicitly.
  The generic defaults would be mb(), i.e. the heavyweight SYNC.
  The *_relaxed() accessors only need ordering against other MMIO
  accesses, which volatile guarantees.

- The {read,write}s{b,w,l}() string helpers, which, unlike the generic
  ones, cope with an unaligned memory buffer. They are defined before
  asm-generic/io.h is included, as it calls them (e.g. ioread8_rep()),
  so they do the volatile access directly rather than via __raw_*().

As a side effect, readX()/writeX() gain MMIO access tracing
(CONFIG_TRACE_MMIO_ACCESS).

vmlinux .text with GCC 16.2 (hsdk_defconfig): -5412 bytes.

Signed-off-by: Vineet Gupta <vgupta@kernel.org>
Assisted-by: Claude:claude-opus-5
---
 arch/arc/include/asm/io.h | 139 ++++++--------------------------------
 1 file changed, 19 insertions(+), 120 deletions(-)

diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
index b4a3393d1bdc..de929136c1a3 100644
--- a/arch/arc/include/asm/io.h
+++ b/arch/arc/include/asm/io.h
@@ -11,15 +11,26 @@
 #include <asm/page.h>
 #include <linux/unaligned.h>
 
-#ifdef CONFIG_ISA_ARCV2
 #include <asm/barrier.h>
+
+#ifdef CONFIG_ISA_ARCV2
 #define __iormb()		rmb()
 #define __iowmb()		wmb()
 #else
-#define __iormb()		do { } while (0)
-#define __iowmb()		do { } while (0)
+/*
+ * The accessors are plain volatile accesses, which the compiler only orders
+ * against other volatile accesses. Make readX()/writeX() compiler barriers so
+ * that e.g. DMA buffer writes are not moved past the MMIO write that starts
+ * the DMA. The asm-generic default, mb(), would be the heavyweight SYNC.
+ */
+#define __iormb()		barrier()
+#define __iowmb()		barrier()
 #endif
 
+/* Barriers for the asm-generic readX()/writeX() */
+#define __io_ar(v)		__iormb()
+#define __io_bw()		__iowmb()
+
 extern void __iomem *ioremap(phys_addr_t paddr, unsigned long size);
 #define ioremap ioremap
 #define ioremap_prot ioremap_prot
@@ -33,57 +44,6 @@ static inline void ioport_unmap(void __iomem *addr)
 {
 }
 
-/*
- * io{read,write}{16,32}be() macros
- */
-#define ioread16be(p)		({ u16 __v = be16_to_cpu((__force __be16)__raw_readw(p)); __iormb(); __v; })
-#define ioread32be(p)		({ u32 __v = be32_to_cpu((__force __be32)__raw_readl(p)); __iormb(); __v; })
-
-#define iowrite16be(v,p)	({ __iowmb(); __raw_writew((__force u16)cpu_to_be16(v), p); })
-#define iowrite32be(v,p)	({ __iowmb(); __raw_writel((__force u32)cpu_to_be32(v), p); })
-
-#define __raw_readb __raw_readb
-static inline u8 __raw_readb(const volatile void __iomem *addr)
-{
-	u8 b;
-
-	__asm__ __volatile__(
-	"	ldb%U1 %0, %1	\n"
-	: "=r" (b)
-	: "m" (*(volatile u8 __force *)addr)
-	: "memory");
-
-	return b;
-}
-
-#define __raw_readw __raw_readw
-static inline u16 __raw_readw(const volatile void __iomem *addr)
-{
-	u16 s;
-
-	__asm__ __volatile__(
-	"	ldw%U1 %0, %1	\n"
-	: "=r" (s)
-	: "m" (*(volatile u16 __force *)addr)
-	: "memory");
-
-	return s;
-}
-
-#define __raw_readl __raw_readl
-static inline u32 __raw_readl(const volatile void __iomem *addr)
-{
-	u32 w;
-
-	__asm__ __volatile__(
-	"	ld%U1 %0, %1	\n"
-	: "=r" (w)
-	: "m" (*(volatile u32 __force *)addr)
-	: "memory");
-
-	return w;
-}
-
 /*
  * {read,write}s{b,w,l}() repeatedly access the same IO address in
  * native endianness in 8-, 16-, 32-bit chunks {into,from} memory,
@@ -102,12 +62,12 @@ static inline void __raw_reads##f(const volatile void __iomem *addr,	\
 	/* Some ARC CPU's don't support unaligned accesses */		\
 	if (is_aligned) {						\
 		do {							\
-			u##t x = __raw_read##f(addr);			\
+			u##t x = *(const volatile u##t __force *)addr;	\
 			*buf++ = x;					\
 		} while (--count);					\
 	} else {							\
 		do {							\
-			u##t x = __raw_read##f(addr);			\
+			u##t x = *(const volatile u##t __force *)addr;	\
 			put_unaligned(x, buf++);			\
 		} while (--count);					\
 	}								\
@@ -120,43 +80,6 @@ __raw_readsx(16, w)
 #define __raw_readsl __raw_readsl
 __raw_readsx(32, l)
 
-#define __raw_writeb __raw_writeb
-static inline void __raw_writeb(u8 b, volatile void __iomem *addr)
-{
-	__asm__ __volatile__(
-	"	stb%U1 %0, %1	\n"
-	:
-	: "r" (b), "m" (*(volatile u8 __force *)addr)
-	: "memory");
-}
-
-#define __raw_writew __raw_writew
-static inline void __raw_writew(u16 s, volatile void __iomem *addr)
-{
-	/*
-	 * Not an "m" operand: GCC can pick a register-indexed address for it,
-	 * which ST doesn't support (GCC PR target/127718).
-	 */
-	__asm__ __volatile__(
-	"	stw %0, [%1]	\n"
-	:
-	: "r" (s), "r" (addr)
-	: "memory");
-
-}
-
-#define __raw_writel __raw_writel
-static inline void __raw_writel(u32 w, volatile void __iomem *addr)
-{
-	/* See __raw_writew() above for why this doesn't use an "m" operand. */
-	__asm__ __volatile__(
-	"	st %0, [%1]	\n"
-	:
-	: "r" (w), "r" (addr)
-	: "memory");
-
-}
-
 #define __raw_writesx(t,f)						\
 static inline void __raw_writes##f(volatile void __iomem *addr, 	\
 				   const void *ptr, unsigned int count)	\
@@ -170,11 +93,12 @@ static inline void __raw_writes##f(volatile void __iomem *addr, 	\
 	/* Some ARC CPU's don't support unaligned accesses */		\
 	if (is_aligned) {						\
 		do {							\
-			__raw_write##f(*buf++, addr);			\
+			*(volatile u##t __force *)addr = *buf++;	\
 		} while (--count);					\
 	} else {							\
 		do {							\
-			__raw_write##f(get_unaligned(buf++), addr);	\
+			u##t x = get_unaligned(buf++);			\
+			*(volatile u##t __force *)addr = x;		\
 		} while (--count);					\
 	}								\
 }
@@ -198,39 +122,14 @@ __raw_writesx(32, l)
  *
  * http://lkml.kernel.org/r/20150622133656.GG1583@arm.com
  */
-#define readb(c)		({ u8  __v = readb_relaxed(c); __iormb(); __v; })
-#define readw(c)		({ u16 __v = readw_relaxed(c); __iormb(); __v; })
-#define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(); __v; })
 #define readsb(p,d,l)		({ __raw_readsb(p,d,l); __iormb(); })
 #define readsw(p,d,l)		({ __raw_readsw(p,d,l); __iormb(); })
 #define readsl(p,d,l)		({ __raw_readsl(p,d,l); __iormb(); })
 
-#define writeb(v,c)		({ __iowmb(); writeb_relaxed(v,c); })
-#define writew(v,c)		({ __iowmb(); writew_relaxed(v,c); })
-#define writel(v,c)		({ __iowmb(); writel_relaxed(v,c); })
 #define writesb(p,d,l)		({ __iowmb(); __raw_writesb(p,d,l); })
 #define writesw(p,d,l)		({ __iowmb(); __raw_writesw(p,d,l); })
 #define writesl(p,d,l)		({ __iowmb(); __raw_writesl(p,d,l); })
 
-/*
- * Relaxed API for drivers which can handle barrier ordering themselves
- *
- * Also these are defined to perform little endian accesses.
- * To provide the typical device register semantics of fixed endian,
- * swap the byte order for Big Endian
- *
- * http://lkml.kernel.org/r/201603100845.30602.arnd@arndb.de
- */
-#define readb_relaxed(c)	__raw_readb(c)
-#define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \
-					__raw_readw(c)); __r; })
-#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
-					__raw_readl(c)); __r; })
-
-#define writeb_relaxed(v,c)	__raw_writeb(v,c)
-#define writew_relaxed(v,c)	__raw_writew((__force u16) cpu_to_le16(v),c)
-#define writel_relaxed(v,c)	__raw_writel((__force u32) cpu_to_le32(v),c)
-
 #include <asm-generic/io.h>
 
 #endif /* _ASM_ARC_IO_H */
-- 
2.53.0

Re: [PATCH 2/2] ARC: io: use asm-generic MMIO accessors

[Arnd Bergmann]

On Wed, Nov 4, 2026, at 05:35, Vineet Gupta wrote:
> ARC MMIO accessors use inline asm, which isn't really needed. It was
> introduced with the original port back in 2013.
>
> As the previous patch showed, inline asm can be error-prone. Letting the
> compiler emit the accesses also allows 16-bit encodings, scaled-index
> loads and folding constant offsets (s9) into the access.
>
> Without the asm, ARC's __raw_{read,write}{b,w,l}(), *_relaxed(),
> {read,write}{b,w,l}() and io{read,write}{16,32}be() are the same as the
> asm-generic/io.h versions, so drop them and use the generic ones.

Hi Vineet,

In my experience, the generic helpers are also fragile, they just lead
to different problems when the compiler optimizes them in ways it
should not.

I would recommend keeping the __raw helpers with the workaround from
the first patch, possibly both variants depending on the compiler
version.

Using the generic higher level interfaces is a very welcome change
though, I hope we can eventually get to the point where every
architecture just needs to provide the __raw_*() ones.

> Two things stay ARC-specific:
>
> - The barriers, now hooked into the generic accessors through
>   __io_ar()/__io_bw(). On ARCv2 they remain dmb (rmb()/wmb()).
>   On ARCompact they were empty, relying on the asm "memory" clobber
>   as a compiler barrier, so make them barrier() explicitly.
>   The generic defaults would be mb(), i.e. the heavyweight SYNC.
>   The *_relaxed() accessors only need ordering against other MMIO
>   accesses, which volatile guarantees.

Sounds good.

> - The {read,write}s{b,w,l}() string helpers, which, unlike the generic
>   ones, cope with an unaligned memory buffer. They are defined before
>   asm-generic/io.h is included, as it calls them (e.g. ioread8_rep()),
>   so they do the volatile access directly rather than via __raw_*().

I don't think we want a custom version here. Do you know the reason this
tries to handle unaligned buffers? If there are drivers that require it,
we can change the generic implementation to be more like the arc version,
otherwise this likely just causes larger object code for all users that
don't ever need unaligned access.

I see that the sparc version does the same thing, other ones without
unaligned access (in particular armv5 and riscv) assume that the
memory buffer is aligned. On alpha and parisc, there is an alignment
check for insl()/outsl() but not readsl()/writesl(). I suspect this
is not intentional.

If they do run into this problem, they will fix it up with a slow
fault handler on the unaligned memory access.

      Arnd

Re: [PATCH 2/2] ARC: io: use asm-generic MMIO accessors

[Vineet Gupta]

On 11/3/26 23:40, Arnd Bergmann wrote:
> On Wed, Nov 4, 2026, at 05:35, Vineet Gupta wrote:
>> ARC MMIO accessors use inline asm, which isn't really needed. It was
>> introduced with the original port back in 2013.
>>
>> As the previous patch showed, inline asm can be error-prone. Letting the
>> compiler emit the accesses also allows 16-bit encodings, scaled-index
>> loads and folding constant offsets (s9) into the access.
>>
>> Without the asm, ARC's __raw_{read,write}{b,w,l}(), *_relaxed(),
>> {read,write}{b,w,l}() and io{read,write}{16,32}be() are the same as the
>> asm-generic/io.h versions, so drop them and use the generic ones.
> Hi Vineet,
>
> In my experience, the generic helpers are also fragile, they just lead
> to different problems when the compiler optimizes them in ways it
> should not.
>
> I would recommend keeping the __raw helpers with the workaround from
> the first patch, possibly both variants depending on the compiler
> version.

OK.

> Using the generic higher level interfaces is a very welcome change
> though, I hope we can eventually get to the point where every
> architecture just needs to provide the __raw_*() ones.

Yes that would be ideal but what we have today is not bad either. I was 
trying to see if its possible to remove boilerplate.

>> Two things stay ARC-specific:
>>
>> - The barriers, now hooked into the generic accessors through
>>    __io_ar()/__io_bw(). On ARCv2 they remain dmb (rmb()/wmb()).
>>    On ARCompact they were empty, relying on the asm "memory" clobber
>>    as a compiler barrier, so make them barrier() explicitly.
>>    The generic defaults would be mb(), i.e. the heavyweight SYNC.
>>    The *_relaxed() accessors only need ordering against other MMIO
>>    accesses, which volatile guarantees.
> Sounds good.
>
>> - The {read,write}s{b,w,l}() string helpers, which, unlike the generic
>>    ones, cope with an unaligned memory buffer. They are defined before
>>    asm-generic/io.h is included, as it calls them (e.g. ioread8_rep()),
>>    so they do the volatile access directly rather than via __raw_*().
> I don't think we want a custom version here. Do you know the reason this
> tries to handle unaligned buffers? If there are drivers that require it,
> we can change the generic implementation to be more like the arc version,
> otherwise this likely just causes larger object code for all users that
> don't ever need unaligned access.

Not specifically but one of my old notes mentions a driver calling 
readsl as follows:

static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master,
       u8 *bytes, int nbytes)
{
   readsl(master->regs + RX_TX_DATA_PORT, bytes, nbytes / 4);
   if (nbytes & 3) {
    u32 tmp;

    readsl(master->regs + RX_TX_DATA_PORT, &tmp, 1);
    memcpy(bytes + (nbytes & ~3), &tmp, nbytes & 3);
   }
}


And with readsl at the time being as following was broken due to the 
recast to int.

static inline void readsl(const volatile void __iomem *addr, void *buffer,
			  unsigned int count)
{
	if (count) {
		u32 *buf = buffer;

		do {
			u32 x = __raw_readl(addr);
			*buf++ = x;
		} while (--count);
	}
}


Thx,
-Vineet

> I see that the sparc version does the same thing, other ones without
> unaligned access (in particular armv5 and riscv) assume that the
> memory buffer is aligned. On alpha and parisc, there is an alignment
> check for insl()/outsl() but not readsl()/writesl(). I suspect this
> is not intentional.
>
> If they do run into this problem, they will fix it up with a slow
> fault handler on the unaligned memory access.
>
>        Arnd

Re: [PATCH 2/2] ARC: io: use asm-generic MMIO accessors

[Arnd Bergmann]

On Thu, Nov 5, 2026, at 03:57, Vineet Gupta wrote:
> On 11/3/26 23:40, Arnd Bergmann wrote:
>> On Wed, Nov 4, 2026, at 05:35, Vineet Gupta wrote:
>>> Two things stay ARC-specific:
>>>
>>> - The barriers, now hooked into the generic accessors through
>>>    __io_ar()/__io_bw(). On ARCv2 they remain dmb (rmb()/wmb()).
>>>    On ARCompact they were empty, relying on the asm "memory" clobber
>>>    as a compiler barrier, so make them barrier() explicitly.
>>>    The generic defaults would be mb(), i.e. the heavyweight SYNC.
>>>    The *_relaxed() accessors only need ordering against other MMIO
>>>    accesses, which volatile guarantees.
>> Sounds good.
>>
>>> - The {read,write}s{b,w,l}() string helpers, which, unlike the generic
>>>    ones, cope with an unaligned memory buffer. They are defined before
>>>    asm-generic/io.h is included, as it calls them (e.g. ioread8_rep()),
>>>    so they do the volatile access directly rather than via __raw_*().
>> I don't think we want a custom version here. Do you know the reason this
>> tries to handle unaligned buffers? If there are drivers that require it,
>> we can change the generic implementation to be more like the arc version,
>> otherwise this likely just causes larger object code for all users that
>> don't ever need unaligned access.
>
> Not specifically but one of my old notes mentions a driver calling 
> readsl as follows:
>
> static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master,
>        u8 *bytes, int nbytes)
> {
>    readsl(master->regs + RX_TX_DATA_PORT, bytes, nbytes / 4);
>    if (nbytes & 3) {
>     u32 tmp;
>
>     readsl(master->regs + RX_TX_DATA_PORT, &tmp, 1);
>     memcpy(bytes + (nbytes & ~3), &tmp, nbytes & 3);
>    }
> }

Indeed, that one looks like a legitimate use of readsl() that
is likely to be affected. Can you send a patch to fix the
asm-generic implementation and use that then?

We have also debated previously whether there should be
barriers before and after readsl()/writesl() and have so
far decided not have barriers against DMA here.

> And with readsl at the time being as following was broken due to the 
> recast to int.
>
> static inline void readsl(const volatile void __iomem *addr, void *buffer,
> 			  unsigned int count)
> {
> 	if (count) {
> 		u32 *buf = buffer;
>
> 		do {
> 			u32 x = __raw_readl(addr);
> 			*buf++ = x;
> 		} while (--count);
> 	}
> }

I think we want something like

       if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) || !is_aligned) {
 		do {
 			u32 x = __raw_readl(addr);
 			put_unaligned(buf++, x);
 		} while (--count);
       } else {
 		do {
 			u32 x = __raw_readl(addr);
 			*buf++ = x;
 		} while (--count);
       }

I.e. use the put_unaligned() unconditionally on architectures
where this is fast and treat the 'is_aligned' check as an
optimization for architectures where aligned buffers can be
accessed faster. (dereferencing an unaligned word without
put_unaligned() on architectures where this is fast tends
to work as well, but is technically still undefined behavior
and may cause the compiler to misbehave).

The coding style for include/asm-generic is to spell out each
copy (2/4/8 byte) rather than use macros to generate them.

      Arnd