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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

However everything is guarded with mutex, so we do not consider here
other threads and within single thread it indeed does not look like
misordered.

> 
> - The __ioread32_copy() writes data from MMIO into main memory,
>   and the store into main memory is guaranteed to be both before
>   the final writel() (because of the implied __iowmb()) and

Yes

>   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.

> 
> - The smp_store_release() in addition orders the write into
>   rx_data->completed after the previous memory asccesses and
>   before the writel().

smp_store_release() wasn't there before the next commit, so maybe that
was missing when interpreting Sashiko suggestions.

There is no code flow where smp_store_release() does not appear, except
when RX queue is empty from the start, but that case would exit early or
would not matter.

Tudor, please validate this more. I postpone for now pull with firmware
driver changes which were put on top of these fixes.


> 
>> If this reordering occurs, the firmware may observe the updated rear
>> pointer, assume the queue slot is available, and overwrite the SRAM
>> payload while the kernel is still actively reading from it, leading
>> to silent data corruption.
> 
> It is possible that I'm still missing the point here, but it
> very much sounds like you trusted a chatbot over trying to
> understand what is actually going on. Can you explain a
> sceneario where the barrier actually makes a difference,
> and what the corresponding barrier operation on the other
> side is?
> 
>      Arnd


Best regards,
Krzysztof