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

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

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