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 <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Reported-by: Sinan Kaya <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
Documentation/memory-barriers.txt | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index a863009849a3..3247547d1c36 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,15 @@ 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 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/DMA-API.txt file for more
+ information on consistent memory.
MMIO WRITE BARRIER
--
2.1.4
On Tue, Mar 27, 2018 at 02:11:27PM +0100, Will Deacon wrote:
> 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 <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Reported-by: Sinan Kaya <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
Good catch, queued on my lkmm branch, thank you!
Thanx, Paul
> ---
> Documentation/memory-barriers.txt | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index a863009849a3..3247547d1c36 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,15 @@ 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 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/DMA-API.txt file for more
> + information on consistent memory.
>
>
> MMIO WRITE BARRIER
> --
> 2.1.4
>
+linux-ia64
On 3/27/2018 11:02 AM, Paul E. McKenney wrote:
> On Tue, Mar 27, 2018 at 02:11:27PM +0100, Will Deacon wrote:
>> 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 <[email protected]>
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: Jason Gunthorpe <[email protected]>
>> Cc: "Paul E. McKenney" <[email protected]>
>> Cc: Peter Zijlstra <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: Jonathan Corbet <[email protected]>
>> Reported-by: Sinan Kaya <[email protected]>
>> Signed-off-by: Will Deacon <[email protected]>
>
> Good catch, queued on my lkmm branch, thank you!
>
> Thanx, Paul
>
Does IA64 follow this requirement? If not, is implementation planned?
"no wmb() before writel()"
Linus asked us to get rid of wmb() in front of writel() for UC memory.
Just checking that we are not breaking anything for IA64.
>> ---
>> Documentation/memory-barriers.txt | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
>> index a863009849a3..3247547d1c36 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,15 @@ 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 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/DMA-API.txt file for more
>> + information on consistent memory.
>>
>>
>> MMIO WRITE BARRIER
>> --
>> 2.1.4
>>
>
>
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On Wed, Mar 28, 2018 at 6:02 AM, Sinan Kaya <[email protected]> wrote:
> +linux-ia64
> Does IA64 follow this requirement? If not, is implementation planned?
>
> "no wmb() before writel()"
>
> Linus asked us to get rid of wmb() in front of writel() for UC memory.
> Just checking that we are not breaking anything for IA64.
We should be OK on ia64, writel() uses a cast to:
*(volatile unsigned int __force *)
which the compiler takes as a request to use a "st4.rel" instruction
(meaning "store with release semantics"). So the value stored will
be visible to anything that follows.
-Tony
On Wed, Mar 28, 2018 at 10:57:11AM -0700, Tony Luck wrote:
> On Wed, Mar 28, 2018 at 6:02 AM, Sinan Kaya <[email protected]> wrote:
> > +linux-ia64
> > Does IA64 follow this requirement? If not, is implementation planned?
> >
> > "no wmb() before writel()"
> >
> > Linus asked us to get rid of wmb() in front of writel() for UC memory.
> > Just checking that we are not breaking anything for IA64.
>
> We should be OK on ia64, writel() uses a cast to:
>
> *(volatile unsigned int __force *)
>
> which the compiler takes as a request to use a "st4.rel" instruction
> (meaning "store with release semantics"). So the value stored will
> be visible to anything that follows.
Just to nitpick, regular release semantics don't guarantee anything like
that, but ia64 never actually got around to implementing proper release
and it's a full barrier and thus what you say is true.