linux-kernel - RE: [PATCH v2] 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: <PH0PR12MB5481D87865AA1F5A21A334DFDC5C9@PH0PR12MB5481.namprd12.prod.outlook.com>
Date:   Thu, 6 Oct 2022 03:25:48 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Akira Yokosawa <akiyks@...il.com>
CC:     "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "parri.andrea@...il.com" <parri.andrea@...il.com>,
        "will@...nel.org" <will@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "boqun.feng@...il.com" <boqun.feng@...il.com>,
        "npiggin@...il.com" <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>,
        "paulmck@...nel.org" <paulmck@...nel.org>,
        Dan Lustig <dlustig@...dia.com>,
        "joel@...lfernandes.org" <joel@...lfernandes.org>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: RE: [PATCH v2] locking/memory-barriers.txt: Improve documentation for
 writel() example


> From: Akira Yokosawa <akiyks@...il.com>
> Sent: Thursday, October 6, 2022 8:18 AM
> 
> Hi,
> 
> Maybe I'm too nit-picky, but see below:
> 
> On Wed, 5 Oct 2022 13:47:49 +0300, 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")
> 
> You can cite the commit in the Changelog text.
> Just say:
>
I prefer to annotate the long commit as a separate line. This makes the description sentence easy to read.
 
>     Commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs.
>     MMIO ordering example") describes that when using writel(), ...
> 
> Also, a blank line is needed above S-o-b tags as a delimiter.
> 
Adding blank line in v3.

[..]
> If you permit a slightly long line here, this hunk would be much easier to
> compare:
> 
True, but document crosses the 80 lines. So I will keep the existing alignment of 80 characters per line.

> +     a dma_wmb().  Note that, when using writel(), a prior barrier is
> + not needed
>       to guarantee that the cache coherent memory writes have completed
> before
>       writing to the MMIO region.  The cheaper writel_relaxed() does not
> provide
> +     this guarantee and must not be used here. Hence, writeX() is always
> +     preferred which inserts needed platform specific barrier before writing
> to
> +     the specified MMIO region.
> 
> That said, I don't feel comfortable with the sentence you added.
> It looks to me it is redundant because such a guarantee of writeX() is already
> covered in the section of "KERNEL I/O BARRIER EFFECTS".
> See the relevant explanation quoted below:
>
I observed that too, but I thought it is worth short note to add along with this example.
But I agree since the next para refers to it anyway, I will drop this 2nd addition in v3.
 
> 	3. A writeX() by a CPU thread to the peripheral will first wait for the
> 	   completion of all prior writes to memory either issued by, or
> 	   propagated to, the same thread. This ensures that writes by the
> CPU
> 	   to an outbound DMA buffer allocated by dma_alloc_coherent() will
> be
> 	   visible to a DMA engine when the CPU writes to its MMIO control
> 	   register to trigger the transfer.
> 
> Also please not that this document should not describe any implementation
> details of those accessors. This document is not meant as an implementation
> guide, but a guide for kernel developers who use them. This is clearly
> mentioned in "DISCLAIMER" at the top of this file.
> 
In the disclaimer section I didn’t see any objection to "describe any implementation". :)
For example, disclaimer itself tells that a barrier may be no-op talking about a possible implementation on a platform.

But I agree to your above point that above #3 already covers with my addition.
Hence, I will drop that part in v3.
Thanks for the review.

>         Thanks, Akira
> 
> > +     needed to guarantee that the cache coherent memory writes have
> completed
> > +     before writing to the MMIO region.  The cheaper writel_relaxed() does
> not
> > +     provide this guarantee and must not be used here. Hence, writeX() is
> always
> > +     preferred which inserts needed platform specific barrier before writing
> to
> > +     the specified MMIO region.
> >
> >       See the subsection "Kernel I/O barrier effects" for more information on
> >       relaxed I/O accessors and the Documentation/core-api/dma-api.rst
> > file for