Ensure inX() is ordered wrt delay() routines

The inX() I/O accessors must enforce ordering against subsequent calls to the delay() routines, so that a read-back from a device can be used to postpone a subsequent write to the same device.

On some architectures, including arm64, this ordering can only be achieved by creating a dependency on the value returned by the inX() operation, so we need to pass the value we read to the __io_par() macro in this case.

Reported-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 include/asm-generic/io.h | 8 ++++----

Quoted 10 lines · Will Deacon

The inX() I/O accessors must enforce ordering against subsequent calls to the delay() routines, so that a read-back from a device can be used to postpone a subsequent write to the same device.

On some architectures, including arm64, this ordering can only be achieved by creating a dependency on the value returned by the inX() operation, so we need to pass the value we read to the __io_par() macro in this case.

Quoted 4 lines

Reported-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 include/asm-generic/io.h | 8 ++++----

For changing the asm-generic file in the arm64 tree,

Acked-by Arnd Bergmann <[email protected]>

For all I can see, this should not conflict with the usage of the same macros on RISC-V, though it does make add a significant difference, so I'd like to see an Ack from the RISC-V folks as well (added to Cc), or possibly a change to arch/riscv/include/asm/io.h to do a corresponding change.

Quoted 5 lines · Arnd Bergmann

For all I can see, this should not conflict with the usage of the same macros on RISC-V, though it does make add a significant difference, so I'd like to see an Ack from the RISC-V folks as well (added to Cc), or possibly a change to arch/riscv/include/asm/io.h to do a corresponding change.

There's already a comment in that header which says that the accesses are ordered wrt timer reads, so I don't think anything needs to change there. For consistency with the macro arguments, I could augment their __io_par to take the read value as an unused argument, if that's what you mean?

Quoted 7 lines · Will Deacon

Quoted 5 lines · Arnd Bergmann

For all I can see, this should not conflict with the usage of the same macros on RISC-V, though it does make add a significant difference, so I'd like to see an Ack from the RISC-V folks as well (added to Cc), or possibly a change to arch/riscv/include/asm/io.h to do a corresponding change.

Quoted 4 lines

There's already a comment in that header which says that the accesses are ordered wrt timer reads, so I don't think anything needs to change there. For consistency with the macro arguments, I could augment their __io_par to take the read value as an unused argument, if that's what you mean?

Yes, that's what I meant, I should have been clearer there.

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · Will Deacon

Quoted 5 lines · Arnd Bergmann

For all I can see, this should not conflict with the usage of the same macros on RISC-V, though it does make add a significant difference, so I'd like to see an Ack from the RISC-V folks as well (added to Cc), or possibly a change to arch/riscv/include/asm/io.h to do a corresponding change.

Thanks, the original patches didn't make it through my filters.

Quoted 4 lines

Quoted 4 lines

There's already a comment in that header which says that the accesses are ordered wrt timer reads, so I don't think anything needs to change there. For consistency with the macro arguments, I could augment their __io_par to take the read value as an unused argument, if that's what you mean?

FWIW, we don't really have a way to mandate this in the ISA yet as there's no formal model for either CSR orderings or the IO memory space.

Quoted 1 line

Yes, that's what I meant, I should have been clearer there.

That sounds reasonable to me. It looks like we can also go ahead and delete a bunch of arch/riscv/include/asm/io.h now that this stuff is in asm-generic, which would cause us to actually start using these things. I didn't know this had all been moved to asm-generic otherwise I would have cleaned this up earlier.

I think this should do it, but this does bring up a bit of an issue: the RISC-V versions of reads and friends put barriers outside the loop, while the asm-generic version don't. What are these actually supposed to do?

Either way that resolves, feel free to consider something like

arch/riscv/include/asm/io.h +8 -8

index b269451e7e85..378975f180a7 100644
@@ -198,20 +198,20 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 * writes.
 */
 #define __io_pbr()	__asm__ __volatile__ ("fence io,i"  : : : "memory");
-#define __io_par()	__asm__ __volatile__ ("fence i,ior" : : : "memory");
+#define __io_par(v)	__asm__ __volatile__ ("fence i,ior" : : : "memory");
 #define __io_pbw()	__asm__ __volatile__ ("fence iow,o" : : : "memory");
 #define __io_paw()	__asm__ __volatile__ ("fence o,io"  : : : "memory");
 
-#define inb(c)		({ u8  __v; __io_pbr(); __v = readb_cpu((void*)(PCI_IOBASE + (c))); __io_par(); __v; })
-#define inw(c)		({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(); __v; })
-#define inl(c)		({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(); __v; })
+#define inb(c)		({ u8  __v; __io_pbr(); __v = readb_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
+#define inw(c)		({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
+#define inl(c)		({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
 
 #define outb(v,c)	({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
 #define outw(v,c)	({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
 #define outl(v,c)	({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
 
 #ifdef CONFIG_64BIT
-#define inq(c)		({ u64 __v; __io_pbr(); __v = readq_cpu((void*)(c)); __io_par(); __v; })
+#define inq(c)		({ u64 __v; __io_pbr(); __v = readq_cpu((void*)(c)); __io_par(__v); __v; })
 #define outq(v,c)	({ __io_pbw(); writeq_cpu((v),(void*)(c)); __io_paw(); })
 #endif
 

@@ -261,9 +261,9 @@ __io_reads_ins(reads, u32, l, __io_br(), __io_ar())
 #define readsw(addr, buffer, count) __readsw(addr, buffer, count)
 #define readsl(addr, buffer, count) __readsl(addr, buffer, count)
 
-__io_reads_ins(ins,  u8, b, __io_pbr(), __io_par())
-__io_reads_ins(ins, u16, w, __io_pbr(), __io_par())
-__io_reads_ins(ins, u32, l, __io_pbr(), __io_par())
+__io_reads_ins(ins,  u8, b, __io_pbr(), __io_par(addr))
+__io_reads_ins(ins, u16, w, __io_pbr(), __io_par(addr))
+__io_reads_ins(ins, u32, l, __io_pbr(), __io_par(addr))
 #define insb(addr, buffer, count) __insb((void __iomem *)(long)addr, buffer, count)
 #define insw(addr, buffer, count) __insw((void __iomem *)(long)addr, buffer, count)
 #define insl(addr, buffer, count) __insl((void __iomem *)(long)addr, buffer, count)

as

Revewied-by Palmer Dabbelt <[email protected]>

when included along with the other diff. That way we can at least keep the macro signatures matching, the cleanup can come later...

Ensure that inX() provides the same ordering guarantees as readX() by hooking up __io_par() so that it maps directly to __iormb().

Reported-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Quoted 5 lines · Will Deacon

Ensure that inX() provides the same ordering guarantees as readX() by hooking up __io_par() so that it maps directly to __iormb().

Reported-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Thanks for your patch!

Quoted 6 lines

arch/arm64/include/asm/io.h +0 -0

@@ -121,6 +121,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
                     : "memory");                                       \
 })
 

arch/arm64/include/asm/io.h static inline u64 __raw_readq(const volatile void __iomem *addr) · line 124

Quoted 1 line

arch/arm64/include/asm/io.h +1 -0

+#define __io_par               __iormb

I think it makes sense to make the parameter passing explicit, for documentation purposes:

    #define __io_par(v)        __iormb(v)