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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Oct 2022 18:23:39 +0900
From:   Akira Yokosawa <akiyks@...il.com>
To:     Arnd Bergmann <arnd@...db.de>, Parav Pandit <parav@...dia.com>
Cc:     Bagas Sanjaya <bagasdotme@...il.com>,
        Alan Stern <stern@...land.harvard.edu>, parri.andrea@...il.com,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>, boqun.feng@...il.com,
        Nicholas Piggin <npiggin@...il.com>, dhowells@...hat.com,
        j.alglave@....ac.uk, luc.maranget@...ia.fr,
        "Paul E. McKenney" <paulmck@...nel.org>, dlustig@...dia.com,
        Joel Fernandes <joel@...lfernandes.org>,
        Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
        Linux-Arch <linux-arch@...r.kernel.org>,
        linux-doc@...r.kernel.org, Akira Yokosawa <akiyks@...il.com>
Subject: Re: [PATCH v4] locking/memory-barriers.txt: Improve documentation for
 writel() example

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