Ordered I/O accessors

This patch introduces readl*_relaxed()/write*_relaxed() as the main I/O accessors (when __mem_pci is defined). The standard read*()/write*() macros are now based on the relaxed accessors.

Quoted 3 lines · Catalin Marinas

This patch introduces readl*_relaxed()/write*_relaxed() as the main I/O accessors (when __mem_pci is defined). The standard read*()/write*() macros are now based on the relaxed accessors.

Are these new macros valid for both PCI and non-PCI mmio addresses? The way I understand it, the regular readl/writel family is only valid for __iomem addresses in PCI BARs, while anything else has to go through either ioread32/iowrite32 or something arch specific.

Does this mean we also need an ioread32_releaxed etc?

Quoted 6 lines · Arnd Bergmann

Are these new macros valid for both PCI and non-PCI mmio addresses? The way I understand it, the regular readl/writel family is only valid for __iomem addresses in PCI BARs, while anything else has to go through either ioread32/iowrite32 or something arch specific.

On ARM we seem to use the readl/writel accessors for a lot more than the PCI (pretty much anything that is ioremap'ed). Not sure why but this has been the case for a long time.

Quoted 1 line

Does this mean we also need an ioread32_releaxed etc?

Most memory ordering problems that I've seen were with PCI devices and DMA coherent buffers. For other DMA engines used together with ioread*(), we could indeed introduce ioread32_relaxed(). But I'm not sure there are so many (a quick grep for dma_alloc_coherent and iowrite shows about 14 drivers).

Quoted 3 lines · Catalin Marinas

This patch introduces readl*_relaxed()/write*_relaxed() as the main I/O accessors (when __mem_pci is defined). The standard read*()/write*() macros are now based on the relaxed accessors.

Are these new macros valid for both PCI and non-PCI mmio addresses? The way I understand it, the regular readl/writel family is only valid for __iomem addresses in PCI BARs, while anything else has to go through either ioread32/iowrite32 or something arch specific.

Does this mean we also need an ioread32_releaxed etc?

Quoted 5 lines · Arnd Bergmann

Quoted 3 lines · Catalin Marinas

This patch introduces readl*_relaxed()/write*_relaxed() as the main I/O accessors (when __mem_pci is defined). The standard read*()/write*() macros are now based on the relaxed accessors.

Quoted 7 lines

Are these new macros valid for both PCI and non-PCI mmio addresses? The way I understand it, the regular readl/writel family is only valid for __iomem addresses in PCI BARs, while anything else has to go through either ioread32/iowrite32 or something arch specific.

Does this mean we also need an ioread32_releaxed etc?

Only if you want to deal with PCI IO accesses as well. The ioread*/iowrite* interfaces are more complex implementations than plain read/write[bwl], because they have to work out whether the void __iomem * cookie relates to an ioremapped cookie or a PCI IO cookie. (That's the only reason to use the io* variants - if you want a driver which can portably access its registers via either PCI MEM or PCI IO access methods.)

Plain read/write[bwl] are much simpler, and on ARM are just pointer dereferences (and now with an additional barrier), so we allow their use in architecture specific drivers.

Quoted 14 lines · On Friday 09 July 2010 20:24:17 Russell King - ARM Linux wrote:

Quoted 5 lines · Arnd Bergmann

Quoted 3 lines · Catalin Marinas

This patch introduces readl*_relaxed()/write*_relaxed() as the main I/O accessors (when __mem_pci is defined). The standard read*()/write*() macros are now based on the relaxed accessors.

Quoted 7 lines

Are these new macros valid for both PCI and non-PCI mmio addresses? The way I understand it, the regular readl/writel family is only valid for __iomem addresses in PCI BARs, while anything else has to go through either ioread32/iowrite32 or something arch specific.

Does this mean we also need an ioread32_releaxed etc?

Quoted 7 lines

Only if you want to deal with PCI IO accesses as well. The ioread*/iowrite* interfaces are more complex implementations than plain read/write[bwl], because they have to work out whether the void __iomem * cookie relates to an ioremapped cookie or a PCI IO cookie. (That's the only reason to use the io* variants - if you want a driver which can portably access its registers via either PCI MEM or PCI IO access methods.)

Right. IMHO the PCI IO variants should get the same barriers that Catalin is introducing in the PCI MEM variants. The ordering requirements for IO accesses are stricter than those for MEM, the main difference being that MEM writes are posted while IO writes are synchronizing.

Thinking about it from this angle, I'm not even sure that x86 compatibility requires arm to add wmb() after writel(). IIRC, PCI memory space writes are required to be ordered with regard to each other, but not necessarily with regard to other CPU instructions or DMA transfers, unlike memory space reads and IO space read/write accesses.

Quoted 4 lines

Right. IMHO the PCI IO variants should get the same barriers that Catalin is introducing in the PCI MEM variants. The ordering requirements for IO accesses are stricter than those for MEM, the main difference being that MEM writes are posted while IO writes are synchronizing.

Linux docs don't clearly define the ordering requirements. There are various e-mail threads but not a finalised document. In principle, trying to be as close to x86 as possible, in which case we probably need barriers here as well.

Quoted 5 lines

Thinking about it from this angle, I'm not even sure that x86 compatibility requires arm to add wmb() after writel(). IIRC, PCI memory space writes are required to be ordered with regard to each other, but not necessarily with regard to other CPU instructions or DMA transfers, unlike memory space reads and IO space read/write accesses.

We don't need a wmb() after writel(), my patch only adds wmb() before writel(). We need previous DMA buffer writes to be visible before writel(), otherwise we get corrupted DMA transfers.

Ah, that's right: writel and outl both need the barrier before the access, but writel will never need a barrier after the access.
The x86 variant of outl also has the implicit ordering after the access, but I'm not sure if we need to emulate that. I can't currently think of a case where it's strictly required because any later access to the same PCI function will be ordered anyway.

Quoted 6 lines · Arnd Bergmann

Ah, that's right: writel and outl both need the barrier before the access, but writel will never need a barrier after the access.
The x86 variant of outl also has the implicit ordering after the access, but I'm not sure if we need to emulate that. I can't currently think of a case where it's strictly required because any later access to the same PCI function will be ordered anyway.

What about those ARMs which can buffer a write for an indefinite period? Do any drivers expect writes to be posted in a reasonably short time?

Quoted 8 lines · Jamie Lokier

Quoted 6 lines · Arnd Bergmann

Ah, that's right: writel and outl both need the barrier before the access, but writel will never need a barrier after the access.
The x86 variant of outl also has the implicit ordering after the access, but I'm not sure if we need to emulate that. I can't currently think of a case where it's strictly required because any later access to the same PCI function will be ordered anyway.

Quoted 2 lines

What about those ARMs which can buffer a write for an indefinite period? Do any drivers expect writes to be posted in a reasonably short time?

Writing to any device is not guaranteed to succeed (i.e. change the state of the device) in a certain amount of time (this is probably the case on x86 as well). If you need this certainty in the code, you do a read back from the device. Since Device memory accesses are ordered in ARM, we don't need additional barriers for such situations.

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Jamie Lokier

Quoted 6 lines · Arnd Bergmann

Ah, that's right: writel and outl both need the barrier before the access, but writel will never need a barrier after the access.
The x86 variant of outl also has the implicit ordering after the access, but I'm not sure if we need to emulate that. I can't currently think of a case where it's strictly required because any later access to the same PCI function will be ordered anyway.

Quoted 8 lines

Quoted 2 lines

What about those ARMs which can buffer a write for an indefinite period? Do any drivers expect writes to be posted in a reasonably short time?

Writing to any device is not guaranteed to succeed (i.e. change the state of the device) in a certain amount of time (this is probably the case on x86 as well). If you need this certainty in the code, you do a read back from the device. Since Device memory accesses are ordered in ARM, we don't need additional barriers for such situations.

There's a time between a "certain amount" and "infinite".

I'm pretty sure the write buffering time for x86 is guaranteed to be finite and quite short (without specifying exactly how short - just like instructions don't specify how long they take to execute), as in it's just a buffer, whose delay is a similar order of magnitude to bus transactions. It's not a cache.

A recent commit changes ARM cpu_relax() because it can keep writes buffered indefinitely. The commit message does say it's because reads are prioritised over writes, so perhaps that's only an issue in loops which use cpu_relax() anyway.

Quoted 9 lines · Catalin Marinas

Quoted 4 lines

Right. IMHO the PCI IO variants should get the same barriers that Catalin is introducing in the PCI MEM variants. The ordering requirements for IO accesses are stricter than those for MEM, the main difference being that MEM writes are posted while IO writes are synchronizing.

Linux docs don't clearly define the ordering requirements. There are various e-mail threads but not a finalised document. In principle, trying to be as close to x86 as possible, in which case we probably need barriers here as well.

Right, emulating x86 behaviour for any PCI access with inl/readl/ioread32 and their counterparts seems to be the only practical answer. It would be nice to have an architecture independent way of doing non-PCI register access, and more importantly relaxed accesses to those.

Note that no other architecture currently defines write*_relaxed(), only read*_relaxed().

Maybe we should add them everywhere, not just on ARM.

Quoted 9 lines

Quoted 5 lines

Thinking about it from this angle, I'm not even sure that x86 compatibility requires arm to add wmb() after writel(). IIRC, PCI memory space writes are required to be ordered with regard to each other, but not necessarily with regard to other CPU instructions or DMA transfers, unlike memory space reads and IO space read/write accesses.

We don't need a wmb() after writel(), my patch only adds wmb() before writel(). We need previous DMA buffer writes to be visible before writel(), otherwise we get corrupted DMA transfers.

Ah, that's right: writel and outl both need the barrier before the access, but writel will never need a barrier after the access.
The x86 variant of outl also has the implicit ordering after the access, but I'm not sure if we need to emulate that. I can't currently think of a case where it's strictly required because any later access to the same PCI function will be ordered anyway.

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

index 97fb9aa..8f3edef 100644
@@ -191,6 +191,15 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
 #define writel_relaxed(v,c)    __raw_writel((__force u32) \

This part requires a small change. It looks like the e1000e driver doesn't like the do {...} while (0) constructs in the write*() accessors (the E1000_WRITE_REG_ARRAY macro). Thanks to Giuseppe for finding this.

Quoted 3 lines · Catalin Marinas

This part requires a small change. It looks like the e1000e driver doesn't like the do {...} while (0) constructs in the write*() accessors (the E1000_WRITE_REG_ARRAY macro). Thanks to Giuseppe for finding this.

Hmm. I think those additional parens in drivers/net/e1000e/hw.h should be killed off instead. They aren't serving any useful purpose.

In fact, this is potentially dangerous - it causes write[bwl]() not to be 'void' - in that if you do use write[bwl]() in an expression that wants its return value, write[bwl]() will generate a read. (While you might not use the value, you're relying on the compiler being able to spot that you're not using the value.)

Keeping the do { } while() there prevents write[bwl]() being used as an expression, and therefore avoids nasty surprises when the compiler decides to read the register after writing.

Quoted 14 lines · Russell King - ARM Linux

Quoted 3 lines · Catalin Marinas

This part requires a small change. It looks like the e1000e driver doesn't like the do {...} while (0) constructs in the write*() accessors (the E1000_WRITE_REG_ARRAY macro). Thanks to Giuseppe for finding this.

Hmm. I think those additional parens in drivers/net/e1000e/hw.h should be killed off instead. They aren't serving any useful purpose.

In fact, this is potentially dangerous - it causes write[bwl]() not to be 'void' - in that if you do use write[bwl]() in an expression that wants its return value, write[bwl]() will generate a read. (While you might not use the value, you're relying on the compiler being able to spot that you're not using the value.)

Quoted 3 lines

Keeping the do { } while() there prevents write[bwl]() being used as an expression, and therefore avoids nasty surprises when the compiler decides to read the register after writing.

I haven't thought of that but yes, it could cause problems. An better solution may be to use a "static inline void writel()" function. If you are ok with this, I'll post a new set of patches.

The patch below changes the __raw_* accessors to inline functions. Do you see any problem with this? This would be needed even without the I/O ordering patches.

Quoted 3 lines · Catalin Marinas

The patch below changes the __raw_* accessors to inline functions. Do you see any problem with this? This would be needed even without the I/O ordering patches.

I'd suggest you try building a kernel before and after this patch and compare the sizes - and then look at the disassembly associated with these accessors.

This updated series fixes a bug in the L2x0 code causing a deadlock (thanks to Adrian Kwong at Trident). The solution is to convert the entire cache-l2x0.c file to the *_relaxed() I/O accessors.