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

[Vineet Gupta <vgupta@kernel.org>]

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