LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 1/2] MIPS: io: prevent compiler reordering on the default writeX() implementation
@ 2018-04-03 12:55 Sinan Kaya
  2018-04-03 12:55 ` [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX() Sinan Kaya
  0 siblings, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-04-03 12:55 UTC (permalink / raw)
  To: linux-mips, arnd, timur, sulrich
  Cc: linux-arm-msm, linux-arm-kernel, Sinan Kaya, Ralf Baechle,
	James Hogan, Paul Burton, linux-kernel

writeX() has a strong ordering semantics with respect to memory updates.
In the abscence of a write barrier or a compiler barrier, commpiler can
reorder register and memory update instructions. This breaks the writeX()
API.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 arch/mips/include/asm/io.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
index 0cbf3af..fd00ddaf 100644
--- a/arch/mips/include/asm/io.h
+++ b/arch/mips/include/asm/io.h
@@ -307,7 +307,7 @@ static inline void iounmap(const volatile void __iomem *addr)
 #if defined(CONFIG_CPU_CAVIUM_OCTEON) || defined(CONFIG_LOONGSON3_ENHANCEMENT)
 #define war_io_reorder_wmb()		wmb()
 #else
-#define war_io_reorder_wmb()		do { } while (0)
+#define war_io_reorder_wmb()		barrier()
 #endif
 
 #define __BUILD_MEMORY_SINGLE(pfx, bwlq, type, irq)			\
-- 
2.7.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-03 12:55 [PATCH v3 1/2] MIPS: io: prevent compiler reordering on the default writeX() implementation Sinan Kaya
@ 2018-04-03 12:55 ` Sinan Kaya
  2018-04-06  1:34   ` Sinan Kaya
  2018-04-12 21:51   ` James Hogan
  0 siblings, 2 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-04-03 12:55 UTC (permalink / raw)
  To: linux-mips, arnd, timur, sulrich
  Cc: linux-arm-msm, linux-arm-kernel, Sinan Kaya, Ralf Baechle,
	James Hogan, Paul Burton, linux-kernel

While a barrier is present in writeX() function before the register write,
a similar barrier is missing in the readX() function after the register
read. This could allow memory accesses following readX() to observe
stale data.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reported-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/mips/include/asm/io.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
index fd00ddaf..6ac502f 100644
--- a/arch/mips/include/asm/io.h
+++ b/arch/mips/include/asm/io.h
@@ -377,6 +377,7 @@ static inline type pfx##read##bwlq(const volatile void __iomem *mem)	\
 		BUG();							\
 	}								\
 									\
+	rmb();								\
 	return pfx##ioswab##bwlq(__mem, __val);				\
 }
 
-- 
2.7.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-03 12:55 ` [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX() Sinan Kaya
@ 2018-04-06  1:34   ` Sinan Kaya
  2018-04-06 18:15     ` Sinan Kaya
  2018-04-12 21:51   ` James Hogan
  1 sibling, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-04-06  1:34 UTC (permalink / raw)
  To: linux-mips, arnd, timur, sulrich
  Cc: linux-arm-msm, linux-arm-kernel, Ralf Baechle, James Hogan,
	Paul Burton, linux-kernel

On 4/3/2018 8:55 AM, Sinan Kaya wrote:
> While a barrier is present in writeX() function before the register write,
> a similar barrier is missing in the readX() function after the register
> read. This could allow memory accesses following readX() to observe
> stale data.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/mips/include/asm/io.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
> index fd00ddaf..6ac502f 100644
> --- a/arch/mips/include/asm/io.h
> +++ b/arch/mips/include/asm/io.h
> @@ -377,6 +377,7 @@ static inline type pfx##read##bwlq(const volatile void __iomem *mem)	\
>  		BUG();							\
>  	}								\
>  									\
> +	rmb();								\
>  	return pfx##ioswab##bwlq(__mem, __val);				\
>  }
>  
> 

Can we get these merged to 4.17? 

There was a consensus to fix the architectures having API violation issues.
https://www.mail-archive.com/netdev@vger.kernel.org/msg225971.html


-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-06  1:34   ` Sinan Kaya
@ 2018-04-06 18:15     ` Sinan Kaya
  2018-04-06 21:26       ` James Hogan
  0 siblings, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-04-06 18:15 UTC (permalink / raw)
  To: linux-mips, arnd, timur, sulrich
  Cc: linux-arm-msm, linux-arm-kernel, Ralf Baechle, James Hogan,
	Paul Burton, linux-kernel

On 4/5/2018 9:34 PM, Sinan Kaya wrote:
> On 4/3/2018 8:55 AM, Sinan Kaya wrote:
>> While a barrier is present in writeX() function before the register write,
>> a similar barrier is missing in the readX() function after the register
>> read. This could allow memory accesses following readX() to observe
>> stale data.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> Reported-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  arch/mips/include/asm/io.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
>> index fd00ddaf..6ac502f 100644
>> --- a/arch/mips/include/asm/io.h
>> +++ b/arch/mips/include/asm/io.h
>> @@ -377,6 +377,7 @@ static inline type pfx##read##bwlq(const volatile void __iomem *mem)	\
>>  		BUG();							\
>>  	}								\
>>  									\
>> +	rmb();								\
>>  	return pfx##ioswab##bwlq(__mem, __val);				\
>>  }
>>  
>>
> 
> Can we get these merged to 4.17? 
> 
> There was a consensus to fix the architectures having API violation issues.
> https://www.mail-archive.com/netdev@vger.kernel.org/msg225971.html
> 
> 

Any news on the MIPS front? Is this something that Arnd can merge? or does it have
to go through the MIPS tree.

It feels like the MIPS is dead since nobody replied to me in the last few weeks on
a very important topic.

-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-06 18:15     ` Sinan Kaya
@ 2018-04-06 21:26       ` James Hogan
  2018-04-07 21:43         ` Sinan Kaya
  2018-04-11 17:04         ` Maciej W. Rozycki
  0 siblings, 2 replies; 16+ messages in thread
From: James Hogan @ 2018-04-06 21:26 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

On Fri, Apr 06, 2018 at 02:15:57PM -0400, Sinan Kaya wrote:
> On 4/5/2018 9:34 PM, Sinan Kaya wrote:
> > Can we get these merged to 4.17? 
> > 
> > There was a consensus to fix the architectures having API violation issues.
> > https://www.mail-archive.com/netdev@vger.kernel.org/msg225971.html
> > 
> > 
> 
> Any news on the MIPS front? Is this something that Arnd can merge? or does it have
> to go through the MIPS tree.

It needs some MIPS input really. I'll try and take a look soon. Thanks
for the nudge.

> It feels like the MIPS is dead since nobody replied to me in the last few weeks on
> a very important topic.

Not dead, just both maintainers heavily distracted by real life right
now (which sadly, for me at least, trumps this very important topic) and
doing the best they can given the circumstances.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-06 21:26       ` James Hogan
@ 2018-04-07 21:43         ` Sinan Kaya
  2018-04-11 17:10           ` Sinan Kaya
  2018-04-11 17:04         ` Maciej W. Rozycki
  1 sibling, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-04-07 21:43 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

On 4/6/2018 5:26 PM, James Hogan wrote:
> On Fri, Apr 06, 2018 at 02:15:57PM -0400, Sinan Kaya wrote:
>> On 4/5/2018 9:34 PM, Sinan Kaya wrote:
>>> Can we get these merged to 4.17? 
>>>
>>> There was a consensus to fix the architectures having API violation issues.
>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg225971.html
>>>
>>>
>>
>> Any news on the MIPS front? Is this something that Arnd can merge? or does it have
>> to go through the MIPS tree.
> 
> It needs some MIPS input really. I'll try and take a look soon. Thanks
> for the nudge.
> 
>> It feels like the MIPS is dead since nobody replied to me in the last few weeks on
>> a very important topic.
> 
> Not dead, just both maintainers heavily distracted by real life right
> now (which sadly, for me at least, trumps this very important topic) and
> doing the best they can given the circumstances.

Thanks for the reply. Glad to hear somebody cares.

> 
> Cheers
> James
> 


-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-06 21:26       ` James Hogan
  2018-04-07 21:43         ` Sinan Kaya
@ 2018-04-11 17:04         ` Maciej W. Rozycki
  1 sibling, 0 replies; 16+ messages in thread
From: Maciej W. Rozycki @ 2018-04-11 17:04 UTC (permalink / raw)
  To: James Hogan
  Cc: Sinan Kaya, linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

Hi James,

> > Any news on the MIPS front? Is this something that Arnd can merge? or does it have
> > to go through the MIPS tree.
> 
> It needs some MIPS input really. I'll try and take a look soon. Thanks
> for the nudge.
> 
> > It feels like the MIPS is dead since nobody replied to me in the last few weeks on
> > a very important topic.
> 
> Not dead, just both maintainers heavily distracted by real life right
> now (which sadly, for me at least, trumps this very important topic) and
> doing the best they can given the circumstances.

 Can I help move this change forward anyhow?

  Maciej

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-07 21:43         ` Sinan Kaya
@ 2018-04-11 17:10           ` Sinan Kaya
  2018-04-11 20:26             ` James Hogan
  0 siblings, 1 reply; 16+ messages in thread
From: Sinan Kaya @ 2018-04-11 17:10 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

On 4/7/2018 5:43 PM, Sinan Kaya wrote:
> On 4/6/2018 5:26 PM, James Hogan wrote:
>> On Fri, Apr 06, 2018 at 02:15:57PM -0400, Sinan Kaya wrote:
>>> On 4/5/2018 9:34 PM, Sinan Kaya wrote:
>>>> Can we get these merged to 4.17? 
>>>>
>>>> There was a consensus to fix the architectures having API violation issues.
>>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg225971.html
>>>>
>>>>
>>>
>>> Any news on the MIPS front? Is this something that Arnd can merge? or does it have
>>> to go through the MIPS tree.
>>
>> It needs some MIPS input really. I'll try and take a look soon. Thanks
>> for the nudge.
>>
>>> It feels like the MIPS is dead since nobody replied to me in the last few weeks on
>>> a very important topic.
>>
>> Not dead, just both maintainers heavily distracted by real life right
>> now (which sadly, for me at least, trumps this very important topic) and
>> doing the best they can given the circumstances.
> 
> Thanks for the reply. Glad to hear somebody cares.


How is the likelihood of getting this fixed on 4.17 kernel? There was an agreement
to fix all architectures. MIPS is the only one left.

> 
>>
>> Cheers
>> James
>>
> 
> 


-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-11 17:10           ` Sinan Kaya
@ 2018-04-11 20:26             ` James Hogan
  2018-04-11 20:48               ` Sinan Kaya
  0 siblings, 1 reply; 16+ messages in thread
From: James Hogan @ 2018-04-11 20:26 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 143 bytes --]

On Wed, Apr 11, 2018 at 01:10:41PM -0400, Sinan Kaya wrote:
> How is the likelihood of getting this fixed on 4.17 kernel?

High.

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-11 20:26             ` James Hogan
@ 2018-04-11 20:48               ` Sinan Kaya
  0 siblings, 0 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-04-11 20:48 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

On 4/11/2018 4:26 PM, James Hogan wrote:
> On Wed, Apr 11, 2018 at 01:10:41PM -0400, Sinan Kaya wrote:
>> How is the likelihood of getting this fixed on 4.17 kernel?
> 
> High.
> 

Thanks for the confirmation.

> Thanks
> James
> 


-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-03 12:55 ` [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX() Sinan Kaya
  2018-04-06  1:34   ` Sinan Kaya
@ 2018-04-12 21:51   ` James Hogan
  2018-04-12 21:58     ` James Hogan
                       ` (2 more replies)
  1 sibling, 3 replies; 16+ messages in thread
From: James Hogan @ 2018-04-12 21:51 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On Tue, Apr 03, 2018 at 08:55:04AM -0400, Sinan Kaya wrote:
> While a barrier is present in writeX() function before the register write,
> a similar barrier is missing in the readX() function after the register
> read. This could allow memory accesses following readX() to observe
> stale data.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Reported-by: Arnd Bergmann <arnd@arndb.de>

Both patches look like obvious improvements to me, so I'm happy to apply
to my fixes branch.

I'm guessing the case of a write to DMA buffer (i.e. reusing it) after a
MMIO readX() (checking DMA complete) being visible to DMA reads prior to
the readX() is precluded by a control dependency (you shouldn't reuse
buffer until you've checked DMA is complete).

But why don't we always use wmb() in the writeX() case? Might not the
cached write to DMA buffer be reordered with the uncached write to MMIO
register from the coherent DMA point of view? I'm waiting on feedback
from MIPS hardware folk on this topic.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-12 21:51   ` James Hogan
@ 2018-04-12 21:58     ` James Hogan
  2018-04-12 22:38       ` Sinan Kaya
  2018-04-12 22:20     ` Sinan Kaya
  2018-04-13 15:41     ` David Laight
  2 siblings, 1 reply; 16+ messages in thread
From: James Hogan @ 2018-04-12 21:58 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

On Thu, Apr 12, 2018 at 10:51:49PM +0100, James Hogan wrote:
> On Tue, Apr 03, 2018 at 08:55:04AM -0400, Sinan Kaya wrote:
> > While a barrier is present in writeX() function before the register write,
> > a similar barrier is missing in the readX() function after the register
> > read. This could allow memory accesses following readX() to observe
> > stale data.
> > 
> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> 
> Both patches look like obvious improvements to me, so I'm happy to apply
> to my fixes branch.

Though having said that, a comment to go with the rmb() (as suggested by
checkpatch) to detail the situation we're concerned about would be nice
to have.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-12 21:51   ` James Hogan
  2018-04-12 21:58     ` James Hogan
@ 2018-04-12 22:20     ` Sinan Kaya
  2018-04-13 15:41     ` David Laight
  2 siblings, 0 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-04-12 22:20 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

On 4/12/2018 5:51 PM, James Hogan wrote:
> But why don't we always use wmb() in the writeX() case? Might not the
> cached write to DMA buffer be reordered with the uncached write to MMIO
> register from the coherent DMA point of view? I'm waiting on feedback
> from MIPS hardware folk on this topic.

Are you asking about this?

 #if defined(CONFIG_CPU_CAVIUM_OCTEON) || defined(CONFIG_LOONGSON3_ENHANCEMENT)
 #define war_io_reorder_wmb()		wmb()
 #else
-#define war_io_reorder_wmb()		do { } while (0)
+#define war_io_reorder_wmb()		barrier()
 #endif

There is a write barrier in writeX() but seem to be different from platform
to platform. 

I'm not familiar with the MIPS architecture. We can always use a wmb() but it
could hurt performance where it is not needed. 

This is the kind of input we need from the MIPS folks if compiler barrier is
enough or we need a wmb() for all cases.

-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-12 21:58     ` James Hogan
@ 2018-04-12 22:38       ` Sinan Kaya
  0 siblings, 0 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-04-12 22:38 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

On 4/12/2018 5:58 PM, James Hogan wrote:
> On Thu, Apr 12, 2018 at 10:51:49PM +0100, James Hogan wrote:
>> On Tue, Apr 03, 2018 at 08:55:04AM -0400, Sinan Kaya wrote:
>>> While a barrier is present in writeX() function before the register write,
>>> a similar barrier is missing in the readX() function after the register
>>> read. This could allow memory accesses following readX() to observe
>>> stale data.
>>>
>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>>> Reported-by: Arnd Bergmann <arnd@arndb.de>
>>
>> Both patches look like obvious improvements to me, so I'm happy to apply
>> to my fixes branch.
> 
> Though having said that, a comment to go with the rmb() (as suggested by
> checkpatch) to detail the situation we're concerned about would be nice
> to have.

I can send a new version. No worries. 

Functional correctness is more important at this moment. I can accommodate
any nice to haves.

> 
> Cheers
> James
> 


-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-12 21:51   ` James Hogan
  2018-04-12 21:58     ` James Hogan
  2018-04-12 22:20     ` Sinan Kaya
@ 2018-04-13 15:41     ` David Laight
  2018-04-13 16:36       ` Sinan Kaya
  2 siblings, 1 reply; 16+ messages in thread
From: David Laight @ 2018-04-13 15:41 UTC (permalink / raw)
  To: 'James Hogan', Sinan Kaya
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

From: James Hogan
> Sent: 12 April 2018 22:52
> On Tue, Apr 03, 2018 at 08:55:04AM -0400, Sinan Kaya wrote:
> > While a barrier is present in writeX() function before the register write,
> > a similar barrier is missing in the readX() function after the register
> > read. This could allow memory accesses following readX() to observe
> > stale data.
> >
> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> 
> Both patches look like obvious improvements to me, so I'm happy to apply
> to my fixes branch.

Don't you also need at least barrier() between the register write in writeX()
and the register read in readX()?
On ppc you probably need eieio.
Or are drivers expected to insert that one?
If they need to insert that one then why not all the others??

	David

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
  2018-04-13 15:41     ` David Laight
@ 2018-04-13 16:36       ` Sinan Kaya
  0 siblings, 0 replies; 16+ messages in thread
From: Sinan Kaya @ 2018-04-13 16:36 UTC (permalink / raw)
  To: David Laight, 'James Hogan'
  Cc: linux-mips, arnd, timur, sulrich, linux-arm-msm,
	linux-arm-kernel, Ralf Baechle, Paul Burton, linux-kernel

On 4/13/2018 11:41 AM, David Laight wrote:
> From: James Hogan
>> Sent: 12 April 2018 22:52
>> On Tue, Apr 03, 2018 at 08:55:04AM -0400, Sinan Kaya wrote:
>>> While a barrier is present in writeX() function before the register write,
>>> a similar barrier is missing in the readX() function after the register
>>> read. This could allow memory accesses following readX() to observe
>>> stale data.
>>>
>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>>> Reported-by: Arnd Bergmann <arnd@arndb.de>
>>
>> Both patches look like obvious improvements to me, so I'm happy to apply
>> to my fixes branch.
> 
> Don't you also need at least barrier() between the register write in writeX()
> and the register read in readX()?
> On ppc you probably need eieio.
> Or are drivers expected to insert that one?
> If they need to insert that one then why not all the others??
> 

Good question. The volatile in here should prevent compiler from reordering the
register read or write instructions. 

static inline type pfx##read##bwlq(const volatile void __iomem *mem)

This is the solution all other architectures rely on especially via
__raw_readX() and __raw_writeX() API.

Now, things can get reordered when it leaves the CPU. This is guaranteed by
embedding wmb() and rmb() into the writeX() and readX() functions in other
architectures.

This ordering guarantee has been agreed to be the responsibility of the
architecture not drivers.

-- 
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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-04-13 16:36 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-03 12:55 [PATCH v3 1/2] MIPS: io: prevent compiler reordering on the default writeX() implementation Sinan Kaya
2018-04-03 12:55 ` [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX() Sinan Kaya
2018-04-06  1:34   ` Sinan Kaya
2018-04-06 18:15     ` Sinan Kaya
2018-04-06 21:26       ` James Hogan
2018-04-07 21:43         ` Sinan Kaya
2018-04-11 17:10           ` Sinan Kaya
2018-04-11 20:26             ` James Hogan
2018-04-11 20:48               ` Sinan Kaya
2018-04-11 17:04         ` Maciej W. Rozycki
2018-04-12 21:51   ` James Hogan
2018-04-12 21:58     ` James Hogan
2018-04-12 22:38       ` Sinan Kaya
2018-04-12 22:20     ` Sinan Kaya
2018-04-13 15:41     ` David Laight
2018-04-13 16:36       ` Sinan Kaya

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).