Re: [PATCH v4] locking/memory-barriers.txt: Improve documentation for writel() example

[Akira Yokosawa]

On Tue, 18 Oct 2022 09:49:34 +0200, Arnd Bergmann wrote:
> On Tue, Oct 18, 2022, at 9:40 AM, Akira Yokosawa wrote:
>> On Tue, 18 Oct 2022 08:44:09 +0200, Arnd Bergmann wrote:
>>>
>>> Anything weaker than a full "wmb()" probably makes the driver calling
>>> the writel() non-portable, so that is both vague and incorrect.
>>
>> Do you mean there is a writel() implementation somewhere in the kernel
>> which doesn't guarantee an implicit wmb() before MMIO write?
> 
> There are lots of those, but that's not what I meant. E.g. on x86,
> writel() does not imply a full wmb() but still guarantees serialization
> between DMA and the register access.
> 
>> Or do you mean my version is confusing because it can imply a weaker
>> write barrier is sufficient before writel_relaxed()?
> 
> That's what I meant, yes. On a lot of architectures, it is sufficient
> to have something weaker than wmb() before writel_relaxed(), especially
> on anything that defines writel_relaxed() to be the same as writel(),
> any barrier would technically work. On arm32, using __iowmb() would be
> sufficient, and this can be less than a full wmb() but again it's
> obviously not portable. These details should not be needed in the
> documentation.
Thanks for the clarification.

I think I was confused by the current wording.
I might be wrong, but I guess Parav's motivation of this change was
to prevent this kind of confusion from the first place.

Parav, may I suggest a reworked changelog? :

    The cited commit describes that when using writel(), explcit wmb()
    is not needed. However, wmb() can be an expensive barrier depending
    on platforms. Arch-specific writel() can use a platform-specific
    weaker barrier needed for the guarantee mentioned in section "KERNEL
    I/O BARRIER EFFECTS".

    Current wording of:
        Note that, when using writel(), a prior wmb() is not needed
        to guarantee that the cache coherent memory writes have completed
        before writing to the MMIO region.

    is confusing because it can be interpreted that writel() always has
    a barrier equivalent to the heavy-weight wmb(), which is not the case.

    Hence stop mentioning wmb() and just call "a prior barrier" in the
    notice.

    commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")

Am I still missing something?

        Thanks, Akira

> 
>       Arnd