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 20:33:08 +0000
From:   Parav Pandit <parav@...dia.com>
To:     "paulmck@...nel.org" <paulmck@...nel.org>,
        Will Deacon <will@...nel.org>
CC:     Arnd Bergmann <arnd@...db.de>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        Alan Stern <stern@...land.harvard.edu>,
        "parri.andrea@...il.com" <parri.andrea@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "boqun.feng@...il.com" <boqun.feng@...il.com>,
        Nicholas Piggin <npiggin@...il.com>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "j.alglave@....ac.uk" <j.alglave@....ac.uk>,
        "luc.maranget@...ia.fr" <luc.maranget@...ia.fr>,
        Akira Yokosawa <akiyks@...il.com>,
        Dan Lustig <dlustig@...dia.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Jonathan Corbet <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux-Arch <linux-arch@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: RE: [PATCH v4] locking/memory-barriers.txt: Improve documentation for
 writel() example

Hi Paul, Will,

> From: Paul E. McKenney <paulmck@...nel.org>
> Sent: Tuesday, October 18, 2022 1:49 PM
> 
> On Tue, Oct 18, 2022 at 11:05:55AM +0100, Will Deacon wrote:
> > On Mon, Oct 17, 2022 at 10:55:00PM +0200, Arnd Bergmann wrote:
> > > On Mon, Oct 10, 2022, at 12:13 PM, Parav Pandit wrote:
> > > > The cited commit describes that when using writel(), explcit wmb()
> > > > is not needed. wmb() is an expensive barrier. writel() uses the
> > > > needed platform specific barrier instead of expensive wmb().
> > > >
> > > > Hence update the example to be more accurate that matches the
> > > > current implementation.
> > > >
> > > > commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA
> vs.
> > > > MMIO ordering example")
> > > >
> > > > Signed-off-by: Parav Pandit <parav@...dia.com>
> > >
> > > I have no objections, though I still don't see a real need to change
> > > the wording here.
> >
> > FWIW, I also don't think this change is necessary. If anything, I'd
> > say we'd be better off _removing_ the text about writel from this
> > section and extending the reference to the "KERNEL I/O BARRIER
> > EFFECTS" section, as you could make similar comments about e.g.
> > readb() and subsequent barriers.
> >
> > For example, something like the diff below.
> 
> I do like this change, but we might be dealing with two different groups of
> readers.  Will and Arnd implemented significant parts of the current
> MMIO/DMA ordering infrastructure.  It is thus quite possible that wording
> which suffices to remind them of how things work might or might not help
> someone new to Linux who is trying to figure out what is required to make
> their driver work.
> 
> The traditional resolution of this sort of thing is to provide the
> documentation to a newbie and take any resulting confusion seriously.
> 
> Parav, thoughts?

I am ok with the change from Will that removes the writel() description.
However, it removes useful short description from the example of "why" writel() is used.
This is useful for newbie and experienced developers both.

So how about below additional change on top of Will's change?
This also aligns to rest of the short C comments in this example pseudo code.

If ok, I will take Will's and mine below change to v5.

index 4d24d39f5e42..5939c5e09570 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1919,7 +1919,9 @@ There are some more advanced barrier functions:
                /* assign ownership */
                desc->status = DEVICE_OWN;

-               /* notify device of new descriptors */
+               /* Make descriptor status visible to the device followed by
+                * notify device of new descriptors
+                */
                writel(DESC_NOTIFY, doorbell);