MIPS: io: prevent compiler reordering on the default writeX() implementation
Subsystems (2)
PATCH v3 1/2 MIPS: io: prevent compiler reordering on the default writeX() implementation
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.
--- arch/mips/include/asm/io.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/mips/include/asm/io.h +1 -1
@@ -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) \
Signature · 1 line
2.7.4
PATCH v3 2/2 MIPS: io: add a barrier after register read in readX()
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.
--- arch/mips/include/asm/io.h | 1 + 1 file changed, 1 insertion(+)
arch/mips/include/asm/io.h +1 -0
@@ -377,6 +377,7 @@ static inline type pfx##read##bwlq(const volatile void __iomem *mem) \BUG (); \ } \ \ +rmb (); \ return pfx##ioswab##bwlq(__mem, __val); \ }
Signature · 1 line
2.7.4
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
Quoted 17 lines · Sinan Kaya
While a barrier is present in
writeX()function before the register write, a similar barrier is missing in thereadX()function after the register read. This could allow memory accesses followingreadX()to observe stale data.Signed-off-by Sinan Kaya <[email protected]>Reported-by Arnd Bergmann <[email protected]>--- arch/mips/include/asm/io.h | 1 + 1 file changed, 1 insertion(+)arch/mips/include/asm/io.h +0
-0
arch/mips/include/asm/io.h static inline type pfx##read##bwlq(const volatile void __iomem *mem) \ · line 383
Quoted 7 lines
arch/mips/include/asm/io.h +1
-0@@ -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 at vger.kernel.org/msg225971.html
Signature · 3 lines
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()
Quoted 28 lines · Sinan Kaya
Quoted 17 lines · Sinan Kaya
While a barrier is present in
writeX()function before the register write, a similar barrier is missing in thereadX()function after the register read. This could allow memory accesses followingreadX()to observe stale data.Signed-off-by Sinan Kaya <[email protected]>Reported-by Arnd Bergmann <[email protected]>--- arch/mips/include/asm/io.h | 1 + 1 file changed, 1 insertion(+)arch/mips/include/asm/io.h +0
-0arch/mips/include/asm/io.h static inline type pfx##read##bwlq(const volatile void __iomem *mem) \ · line 383
Quoted 7 lines
arch/mips/include/asm/io.h +1
-0@@ -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?
Quoted 4 lines
There was a consensus to fix the architectures having API violation issues.
https://www.mail-archive.com/netdev at 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.
Signature · 3 lines
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()
Quoted 10 lines · Sinan Kaya
Quoted 6 lines · Sinan Kaya
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 at vger.kernel.org/msg225971.htmlAny 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.
Quoted 2 lines
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180406/50aa7ed9/attachment.sig>
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
Quoted 15 lines · James Hogan
Quoted 10 lines · Sinan Kaya
Quoted 6 lines · Sinan Kaya
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 at vger.kernel.org/msg225971.htmlAny 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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 4 lines
Cheers
James
Signature · 3 lines
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()
Quoted 16 lines · Sinan Kaya
Quoted 15 lines · James Hogan
Quoted 10 lines · Sinan Kaya
Quoted 6 lines · Sinan Kaya
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 at vger.kernel.org/msg225971.htmlAny 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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 5 lines
Cheers
James
Signature · 3 lines
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()
Quoted 1 line · Sinan Kaya
How is the likelihood of getting this fixed on 4.17 kernel?
High.
Thanks
James
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180411/a68a6239/attachment.sig>
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
Quoted 5 lines · James Hogan
Quoted 1 line · Sinan Kaya
How is the likelihood of getting this fixed on 4.17 kernel?
High.
Thanks for the confirmation.
Quoted 3 lines
Thanks
James
Signature · 3 lines
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()
In reply to James HoganHi James,
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 2 lines
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()
In reply to Sinan KayaQuoted 7 lines · Sinan Kaya
While a barrier is present in
writeX()function before the register write, a similar barrier is missing in thereadX()function after the register read. This could allow memory accesses followingreadX()to observe stale data.Signed-off-by Sinan Kaya <[email protected]>Reported-by Arnd Bergmann <[email protected]>
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180412/086384f7/attachment.sig>
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
Quoted 11 lines · James Hogan
Quoted 7 lines · Sinan Kaya
While a barrier is present in
writeX()function before the register write, a similar barrier is missing in thereadX()function after the register read. This could allow memory accesses followingreadX()to observe stale data.Signed-off-by Sinan Kaya <[email protected]>Reported-by Arnd Bergmann <[email protected]>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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180412/f9f87e90/attachment.sig>
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
Quoted 10 lines · James Hogan
Quoted 9 lines · James Hogan
Quoted 7 lines · Sinan Kaya
While a barrier is present in
writeX()function before the register write, a similar barrier is missing in thereadX()function after the register read. This could allow memory accesses followingreadX()to observe stale data.Signed-off-by Sinan Kaya <[email protected]>Reported-by Arnd Bergmann <[email protected]>
Quoted 6 lines
Quoted 2 lines
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.
Quoted 4 lines
Cheers
James
Signature · 3 lines
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()
In reply to James HoganQuoted 4 lines · James Hogan
But why don't we always use
wmb()in thewriteX()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.
Signature · 3 lines
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()
In reply to James HoganFrom: James Hogan
Quoted 7 lines
Sent: 12 April 2018 22:52
Quoted 5 lines · Sinan Kaya
While a barrier is present in
writeX()function before the register write, a similar barrier is missing in thereadX()function after the register read. This could allow memory accesses followingreadX()to observe stale data.
Quoted 5 lines
Quoted 2 lines
Signed-off-by Sinan Kaya <[email protected]>Reported-by Arnd Bergmann <[email protected]>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()
Quoted 11 lines · David Laight
From: James Hogan
Quoted 10 lines
Sent: 12 April 2018 22:52
Quoted 7 lines · Sinan Kaya
While a barrier is present in
writeX()function before the register write, a similar barrier is missing in thereadX()function after the register read. This could allow memory accesses followingreadX()to observe stale data.Signed-off-by Sinan Kaya <[email protected]>Reported-by Arnd Bergmann <[email protected]>
Quoted 9 lines
Quoted 2 lines
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 inwriteX()and the register read inreadX()?
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.
Signature · 3 lines
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.