| Message ID | 691b903c-e97d-0a25-28c5-690318bb215a@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 4, 2018 at 5:52 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 4/3/2018 6:29 PM, Palmer Dabbelt wrote: >> > > Are we looking for something like this? Yes, exactly, plus the same for write and in/out of course. > diff --git a/inc > #ifndef readb > #define readb readb > -static inline u8 readb(const volatile void __iomem *addr) > -{ > - return __raw_readb(addr); > -} > +#define readb(c) \ > + ({ u8 __v; \ > + __io_br(); \ > + __v = __raw_readb(c); \ > + __io_ar(); \ > + __v; }) > #endif I would prefer leaving these as inline functions, but that's only a cosmetic difference. Arnd
On 4/4/2018 11:55 AM, Arnd Bergmann wrote: > On Wed, Apr 4, 2018 at 5:52 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> On 4/3/2018 6:29 PM, Palmer Dabbelt wrote: >>> >> >> Are we looking for something like this? > > Yes, exactly, plus the same for write and in/out of course. > OK. I just wanted to double check first. >> diff --git a/inc >> #ifndef readb >> #define readb readb >> -static inline u8 readb(const volatile void __iomem *addr) >> -{ >> - return __raw_readb(addr); >> -} >> +#define readb(c) \ >> + ({ u8 __v; \ >> + __io_br(); \ >> + __v = __raw_readb(c); \ >> + __io_ar(); \ >> + __v; }) >> #endif > > I would prefer leaving these as inline functions, but that's only > a cosmetic difference. sure, I'll leave these as inline functions. > > Arnd >
On 4/4/2018 11:55 AM, Arnd Bergmann wrote:
> Yes, exactly, plus the same for write and in/out of course.
I was looking at this...
inb() and outb() seem to be calling writeb(). It gets the wmb/barrier automatically
when we fix writeb().
Did I miss something?
On Wed, Apr 4, 2018 at 7:48 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 4/4/2018 11:55 AM, Arnd Bergmann wrote: >> Yes, exactly, plus the same for write and in/out of course. > > I was looking at this... > > inb() and outb() seem to be calling writeb(). It gets the wmb/barrier automatically > when we fix writeb(). > > Did I miss something? At least outb() needs stricter barriers than writeb() in theory, what we want here is that outb() has not just made it out to the device but that the write has been confirmed completed by the device. Some architectures can't do it, but those that can should have an easy way to hook into that using a separate set of barriers. Using the riscv barrier names, we could do this like #ifndef __io_bw() #define __io_bw() wmb() #endif #ifndef __io_aw #define __io_aw() barrier() #endif #ifndef __io_pbw #define __io_pbw() __io_bw() #endif #ifndef __io_paw #define __io_paw() __io_aw() #endif and the same thing for reads. This way, an architecture could override any of those, but still get reasonable defaults for the others. For __io_bw(), I picked barrier() instead of do {} while (0), no idea if that's any better, I just play safe here. Arnd
On 4/4/2018 3:50 PM, Arnd Bergmann wrote: > On Wed, Apr 4, 2018 at 7:48 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> On 4/4/2018 11:55 AM, Arnd Bergmann wrote: >>> Yes, exactly, plus the same for write and in/out of course. >> >> I was looking at this... >> >> inb() and outb() seem to be calling writeb(). It gets the wmb/barrier automatically >> when we fix writeb(). >> >> Did I miss something? > > At least outb() needs stricter barriers than writeb() in theory, what > we want here > is that outb() has not just made it out to the device but that the > write has been > confirmed completed by the device. Some architectures can't do it, but those > that can should have an easy way to hook into that using a separate set of > barriers. > > Using the riscv barrier names, we could do this like > > #ifndef __io_bw() > #define __io_bw() wmb() > #endif > > #ifndef __io_aw > #define __io_aw() barrier() > #endif > > #ifndef __io_pbw > #define __io_pbw() __io_bw() > #endif > > #ifndef __io_paw > #define __io_paw() __io_aw() > #endif > > and the same thing for reads. This way, an architecture could override > any of those, but still get reasonable defaults for the others. > For __io_bw(), I picked barrier() instead of do {} while (0), no idea > if that's any better, I just play safe here. I posted V3. I hope I captured what you mean above correctly. > > Arnd >
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index e8c2078..693a82f 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -101,6 +101,16 @@ static inline void __raw_writeq(u64 value, volatile void __iomem *addr) #endif #endif /* CONFIG_64BIT */ +#ifndef __io_br() +#define __io_br() do {} while (0) +#endif + +#ifdef rmb +#define __io_ar() rmb(); +#else +#define __io_ar() barrier(); +#endif + /* * {read,write}{b,w,l,q}() access little endian memory and return result in * native endianness. @@ -108,35 +118,46 @@ static inline void __raw_writeq(u64 value, volatile void __iomem *addr) #ifndef readb #define readb readb -static inline u8 readb(const volatile void __iomem *addr) -{ - return __raw_readb(addr); -} +#define readb(c) \ + ({ u8 __v; \ + __io_br(); \ + __v = __raw_readb(c); \ + __io_ar(); \ + __v; }) #endif #ifndef readw #define readw readw -static inline u16 readw(const volatile void __iomem *addr) -{ - return __le16_to_cpu(__raw_readw(addr)); -} +#define readw(c) \ + ({ u16 __v; \ + \ + __io_br(); \ + __v = __le16_to_cpu(__raw_readw(c)); \ + __io_ar(); \ + __v; }) #endif #ifndef readl #define readl readl -static inline u32 readl(const volatile void __iomem *addr) -{ - return __le32_to_cpu(__raw_readl(addr)); -} +#define readl(c) \ + ({ u32 __v; \ + \ + __io_br(); \ + __v = __le32_to_cpu(__raw_readl(c)); \ + __io_ar(); \ + __v; }) #endif #ifdef CONFIG_64BIT #ifndef readq #define readq readq -static inline u64 readq(const volatile void __iomem *addr) -{ - return __le64_to_cpu(__raw_readq(addr)); -} +#define readq(c) \ + ({ u64 __v; \ + \ + __io_br(); \ + __v = __le64_to_cpu(__raw_readq(c)); \ + __io_ar(); \ + __v; }) #endif #endif /* CONFIG_64BIT */