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

[Parav Pandit]

> From: Akira Yokosawa <akiyks@xxxxxxxxx>
> 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