Re: RFC on writel and writel_relaxed
From: Arnd Bergmann <arnd@arndb.de>
Date: 2018-03-27 10:05:09
Also in:
linux-rdma
On Tue, Mar 27, 2018 at 11:57 AM, Will Deacon [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From db0daeaf94f0f6232f8206fc07a74211324b11d9 Mon Sep 17 00:00:00 2001 From: Will Deacon <redacted> Date: Tue, 27 Mar 2018 10:49:58 +0100 Subject: [PATCH] docs/memory-barriers.txt: Fix broken DMA vs MMIO ordering example The section of memory-barriers.txt that describes the dma_Xmb() barriers has an incorrect example claiming that a wmb() is required after writing to coherent memory in order for those writes to be visible to a device before a subsequent MMIO access using writel() can reach the device. In fact, this ordering guarantee is provided (at significant cost on some architectures such as arm and power) by writel, so the wmb() is not necessary. writel_relaxed exists for cases where this ordering is not required. Fix the example and update the text to make this clearer. Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: "Paul E. McKenney" <redacted> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Jonathan Corbet <corbet@lwn.net> Reported-by: Sinan Kaya <redacted> Signed-off-by: Will Deacon <redacted> --- Documentation/memory-barriers.txt | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt index a863009849a3..2556b4b0e6f9 100644 --- a/Documentation/memory-barriers.txt +++ b/Documentation/memory-barriers.txt@@ -1909,9 +1909,6 @@ There are some more advanced barrier functions: /* assign ownership */ desc->status = DEVICE_OWN; - /* force memory to sync before notifying device via MMIO */ - wmb(); - /* notify device of new descriptors */ writel(DESC_NOTIFY, doorbell); }@@ -1919,11 +1916,16 @@ There are some more advanced barrier functions: The dma_rmb() allows us guarantee 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 wmb() is needed to guarantee that the - cache coherent memory writes have completed before attempting a write to - the cache incoherent MMIO region. - - See Documentation/DMA-API.txt for more information on consistent memory. + can see it now has ownership. 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 cache incoherent MMIO region. + If this ordering between incoherent MMIO and coherent memory regions + is not required, writel_relaxed() can be used instead and is significantly + cheaper on some weakly-ordered architectures.
I think that's a great improvement, but I'm a bit worried about recommending
writel_relaxed() too much: I've seen a lot of drivers that just always use
writel_relaxed() over write(), and some of them get that wrong when they
don't understand the difference but end up using DMA without explicit
barriers anyway.
Also, having an architecture-independent driver use wmb()+writel_relaxed()
ends up being more expensive than just using write(). Not sure how to
best phrase it though.
Arnd