[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. Signed-off-by: Sinan Kaya Reported-by: Arnd Bergmann --- 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()
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 > Reported-by: Arnd Bergmann > --- > 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()
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 >> Reported-by: Arnd Bergmann >> --- >> 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()
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 signature.asc Description: Digital signature
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
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()
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()
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()
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 signature.asc Description: Digital signature
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
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()
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 > Reported-by: Arnd Bergmann 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 signature.asc Description: Digital signature
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
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 > > Reported-by: Arnd Bergmann > > 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 signature.asc Description: Digital signature
Re: [PATCH v3 2/2] MIPS: io: add a barrier after register read in readX()
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()
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 >>> Reported-by: Arnd Bergmann >> >> 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()
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 > > Reported-by: Arnd Bergmann > > 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()
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 >>> Reported-by: Arnd Bergmann >> >> 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.