diff mbox series

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

Message ID 20221027201000.219731-1-parav@nvidia.com
State New
Headers
Series [v5] locking/memory-barriers.txt: Improve documentation for writel() example |

Commit Message

Parav Pandit Oct. 27, 2022, 8:10 p.m. UTC 
  The cited commit describes that when using writel(), explicit wmb()
is not needed. wmb() is an expensive barrier. writel() uses the needed
platform specific barrier instead of wmb().

writeX() section of "KERNEL I/O BARRIER EFFECTS" already describes
ordering of I/O accessors with MMIO writes.

Hence add the comment for pseudo code of writel() and remove confusing
text around writel() and wmb().

commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")

Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v4->v5:
- Used suggested documentation update from Will
- Added comment to the writel() pseudo code example
- updated commit log for newer changes
v3->v4:
- further trimmed the documentation for redundant description
v2->v3:
- removed redundant description for writeX()
- updated text for alignment and smaller change lines
- updated commit log with blank line before signed-off-by line
v1->v2:
- Further improved description of writel() example
- changed commit subject from 'usage' to 'example'
v0->v1:
- Corrected to mention I/O barrier instead of dma_wmb().
- removed numbered references in commit log
- corrected typo 'explcit' to 'explicit' in commit log
---
 Documentation/memory-barriers.txt | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Paul E. McKenney Oct. 28, 2022, 5:56 p.m. UTC | #1 
On Thu, Oct 27, 2022 at 11:10:00PM +0300, Parav Pandit wrote:
> The cited commit describes that when using writel(), explicit wmb()
> is not needed. wmb() is an expensive barrier. writel() uses the needed
> platform specific barrier instead of wmb().
> 
> writeX() section of "KERNEL I/O BARRIER EFFECTS" already describes
> ordering of I/O accessors with MMIO writes.
> 
> Hence add the comment for pseudo code of writel() and remove confusing
> text around writel() and wmb().
> 
> commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Hearing no immediate objections, I have pulled this in for further
review.  If all goes well, I intend to submit this during the upcoming
v6.2 merge window.

							Thanx, Paul

> ---
> changelog:
> v4->v5:
> - Used suggested documentation update from Will
> - Added comment to the writel() pseudo code example
> - updated commit log for newer changes
> v3->v4:
> - further trimmed the documentation for redundant description
> v2->v3:
> - removed redundant description for writeX()
> - updated text for alignment and smaller change lines
> - updated commit log with blank line before signed-off-by line
> v1->v2:
> - Further improved description of writel() example
> - changed commit subject from 'usage' to 'example'
> v0->v1:
> - Corrected to mention I/O barrier instead of dma_wmb().
> - removed numbered references in commit log
> - corrected typo 'explcit' to 'explicit' in commit log
> ---
>  Documentation/memory-barriers.txt | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index 06f80e3785c5..e698093bade1 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1910,7 +1910,8 @@ There are some more advanced barrier functions:
>  
>       These are for use with consistent memory to guarantee the ordering
>       of writes or reads of shared memory accessible to both the CPU and a
> -     DMA capable device.
> +     DMA capable device. See Documentation/core-api/dma-api.rst file for more
> +     information about consistent memory.
>  
>       For example, consider a device driver that shares memory with a device
>       and uses a descriptor status value to indicate if the descriptor belongs
> @@ -1931,22 +1932,21 @@ 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 descriptor
> +		 */
>  		writel(DESC_NOTIFY, doorbell);
>  	}
>  
> -     The dma_rmb() allows us guarantee the device has released ownership
> +     The dma_rmb() allows us to guarantee that the device has released ownership
>       before we read the data from the descriptor, and the dma_wmb() allows
>       us to guarantee the data is written to the descriptor before the device
>       can see it now has ownership.  The dma_mb() implies both a dma_rmb() and
> -     a dma_wmb().  Note that, when using writel(), a prior wmb() 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.
> -
> -     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
> -     more information on consistent memory.
> +     a dma_wmb().
> +
> +     Note that the dma_*() barriers do not provide any ordering guarantees for
> +     accesses to MMIO regions.  See the later "KERNEL I/O BARRIER EFFECTS"
> +     subsection for more information about I/O accessors and MMIO ordering.
>  
>   (*) pmem_wmb();
>  
> -- 
> 2.26.2
>
Parav Pandit Oct. 28, 2022, 5:59 p.m. UTC | #2 
Hi Will, Paul,

> From: Paul E. McKenney <paulmck@kernel.org>
> Sent: Friday, October 28, 2022 1:56 PM
> 
> On Thu, Oct 27, 2022 at 11:10:00PM +0300, Parav Pandit wrote:
> > The cited commit describes that when using writel(), explicit wmb() is
> > not needed. wmb() is an expensive barrier. writel() uses the needed
> > platform specific barrier instead of wmb().
> >
> > writeX() section of "KERNEL I/O BARRIER EFFECTS" already describes
> > ordering of I/O accessors with MMIO writes.
> >
> > Hence add the comment for pseudo code of writel() and remove confusing
> > text around writel() and wmb().
> >
> > commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs.
> > MMIO ordering example")
> >
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> Hearing no immediate objections, I have pulled this in for further review.  If
> all goes well, I intend to submit this during the upcoming
> v6.2 merge window.
> 
> 							Thanx, Paul

I have taken Will's email patch suggestion with small editorial modification in it.
But I was afraid to add his Signed-off-by myself in v5 without consulting him.

Will,
Can you please reply to add your Signed-off-by as well or not?
Will Deacon Nov. 2, 2022, 6:05 a.m. UTC | #3 
On Thu, Oct 27, 2022 at 11:10:00PM +0300, Parav Pandit wrote:
> The cited commit describes that when using writel(), explicit wmb()
> is not needed. wmb() is an expensive barrier. writel() uses the needed
> platform specific barrier instead of wmb().
> 
> writeX() section of "KERNEL I/O BARRIER EFFECTS" already describes
> ordering of I/O accessors with MMIO writes.
> 
> Hence add the comment for pseudo code of writel() and remove confusing
> text around writel() and wmb().
> 
> commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
> changelog:
> v4->v5:
> - Used suggested documentation update from Will
> - Added comment to the writel() pseudo code example
> - updated commit log for newer changes

Sorry for the delay on this, I'm really behind on patches at the moment.
This patch looks good to me, so thanks for doing it. You can either add
my:

Acked-by: Will Deacon <will@kernel.org>

or, since we worked on this together:

Co-developed-by: Will Deacon <will@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>

Cheers,

Will
Paul E. McKenney Nov. 2, 2022, 8:30 p.m. UTC | #4 
On Wed, Nov 02, 2022 at 06:05:54AM +0000, Will Deacon wrote:
> On Thu, Oct 27, 2022 at 11:10:00PM +0300, Parav Pandit wrote:
> > The cited commit describes that when using writel(), explicit wmb()
> > is not needed. wmb() is an expensive barrier. writel() uses the needed
> > platform specific barrier instead of wmb().
> > 
> > writeX() section of "KERNEL I/O BARRIER EFFECTS" already describes
> > ordering of I/O accessors with MMIO writes.
> > 
> > Hence add the comment for pseudo code of writel() and remove confusing
> > text around writel() and wmb().
> > 
> > commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
> > 
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> > changelog:
> > v4->v5:
> > - Used suggested documentation update from Will
> > - Added comment to the writel() pseudo code example
> > - updated commit log for newer changes
> 
> Sorry for the delay on this, I'm really behind on patches at the moment.
> This patch looks good to me, so thanks for doing it. You can either add
> my:
> 
> Acked-by: Will Deacon <will@kernel.org>
> 
> or, since we worked on this together:
> 
> Co-developed-by: Will Deacon <will@kernel.org>
> Signed-off-by: Will Deacon <will@kernel.org>

Thank you!  I will apply these tags on the next rebase.

							Thanx, Paul
diff mbox series

Patch

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 06f80e3785c5..e698093bade1 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1910,7 +1910,8 @@  There are some more advanced barrier functions:
 
      These are for use with consistent memory to guarantee the ordering
      of writes or reads of shared memory accessible to both the CPU and a
-     DMA capable device.
+     DMA capable device. See Documentation/core-api/dma-api.rst file for more
+     information about consistent memory.
 
      For example, consider a device driver that shares memory with a device
      and uses a descriptor status value to indicate if the descriptor belongs
@@ -1931,22 +1932,21 @@  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 descriptor
+		 */
 		writel(DESC_NOTIFY, doorbell);
 	}
 
-     The dma_rmb() allows us guarantee the device has released ownership
+     The dma_rmb() allows us to guarantee that the device has released ownership
      before we read the data from the descriptor, and the dma_wmb() allows
      us to guarantee the data is written to the descriptor before the device
      can see it now has ownership.  The dma_mb() implies both a dma_rmb() and
-     a dma_wmb().  Note that, when using writel(), a prior wmb() 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.
-
-     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
-     more information on consistent memory.
+     a dma_wmb().
+
+     Note that the dma_*() barriers do not provide any ordering guarantees for
+     accesses to MMIO regions.  See the later "KERNEL I/O BARRIER EFFECTS"
+     subsection for more information about I/O accessors and MMIO ordering.
 
  (*) pmem_wmb();