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] [day] [month] [year] [list]

Message-ID: <PH0PR12MB5481D4417D82F130804ABA4FDC339@PH0PR12MB5481.namprd12.prod.outlook.com>
Date:   Thu, 27 Oct 2022 18:24:35 +0000
From:   Parav Pandit <parav@...dia.com>
To:     "paulmck@...nel.org" <paulmck@...nel.org>
CC:     Will Deacon <will@...nel.org>, 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



> From: Paul E. McKenney <paulmck@...nel.org>
> Sent: Thursday, October 27, 2022 2:15 PM
> 
> On Tue, Oct 18, 2022 at 08:33:08PM +0000, Parav Pandit wrote:
> > 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);
> 
> Hearing no objections, please proceed.
> 
> 							Thanx, Paul
Thanks Paul, I will respin v5 shortly.