Re: [PATCH v5 4/7] firmware: samsung: acpm: Add memory barrier before advancing RX pointer

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

On Fri, May 29, 2026, at 09:47, Krzysztof Kozlowski wrote:
> On 28/05/2026 19:44, Arnd Bergmann wrote:
>> On Tue, May 5, 2026, at 15:13, Tudor Ambarus wrote:
>>> Sashiko identified a silent data corruption in [1].
>>>
>>> In acpm_get_rx(), the driver reads the response payload from SRAM using
>>> __ioread32_copy() and subsequently updates the hardware RX rear pointer
>>> via writel().
>>>
>>> On weakly ordered architectures like ARM64, writel() provides a write
>>> memory barrier (wmb()), which strictly orders prior writes against
>>> subsequent writes. However, it does not order prior reads against
>>> subsequent writes. Consequently, the CPU is permitted to reorder the
>>> writel() store to become globally visible before the payload reads
>>> have completed.
>> 
>> I am very confused by this after seeing it in the Exynos fixes pull
>> request. How would anything get reordered here? What I see is that
>> 
>> - The SRAM is device memory, so any access to it is architecturally
>>   ordered against other accesses to the same device. Even on
>>   architectures that don't guarantee this, Linux I/O accessors
>>   do.
>
> Well, __ioread32_copy does not guarantee that, I think. That's the
> relaxed version.

__ioread32_copy() certainly does not guarantee the ordering within
the block, and I think you are right that we don't properly document
the ordering between a __raw_readl() and following writel(), but
as far as I can tell all implementations do provide strict
ordering here because either the MMIO load/store instructions are
architecturally ordered (x86, arm64, ...) or there are sufficient
barriers in the writel() to serialize the __raw_readl() as well
(mips, alpha, ...).

[side note: there is a difference between __raw_readl() and
readl_relaxed() here. The _relaxed MMIO operations are required
to to be serialized with each other but not against memory
accesses, while the __raw_ version used here provides neither
guarantee]

>>   after the read (because of the data dependency).
>
> I don't see the data dependency regarding the write. We read 'rx_front'
> and 'i' in the loop. The 'i' is used for subsequent read (addr = base +
> mlen*i) and that's dependency, but that 'addr' is not used in any
> further writes.

What I meant is that the store into the target memory buffer
(xfer->rxd or rx_data->cmd) depends on the data being read from
MMIO first. The writel() guarantees that this buffer is visible
to all DMA masters in the system and that can only happen when
the __raw_readl() has provided the data first.

       Arnd