> 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
