[PATCH v3 1/2] MIPS: io: prevent compiler reordering on the default writeX() implementation

[PATCH v3 1/2] MIPS: io: prevent compiler reordering on the default writeX() implementation

[Sinan Kaya]

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

[PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[James Hogan]

[-- 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 --]

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[James Hogan]

[-- Attachment #1.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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Maciej W. Rozycki]

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

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[James Hogan]

[-- Attachment #1.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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[James Hogan]

[-- 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 --]

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.

RE: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[David Laight]

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

Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()

[Sinan Kaya]

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.