On Monday 20 June 2011, Nicolas Pitre wrote:
On Mon, 20 Jun 2011, Arnd Bergmann wrote:
On Saturday 18 June 2011, Nicolas Pitre wrote: Well, in case of readl/writel, we need the memory clobber for other reasons: They might trigger a DMA operation, or the readl might be used to wait for completion of a DMA operation. In either case, you need to have a barrier to prevent the reordering of the MMIO instruction w.r.t the DMA memory that it is referring to. On in-order CPUs, this does not always require a synchronisation instruction, but it strill requires a compiler barrier, which is typically expressed as a full memory clobber.
We don't have any of that now though.
We do have __iormb() in there, which is both a CPU and a compiler barrier. Unfortunately, in case of !CONFIG_ARM_DMA_MEM_BUFFERABLE it degrades to no barrier at all, which is a bug.
And I think this would be much more sensible if the barrier was explicit, so not to generate awful code as I demonstrated, in the vast majority of the cases where there is no such issues.
Yes, we can recommend to people to use the _relaxed versions, especially for drivers that don't do any DMA.
And the fact that the inline asm is marked volatile should be enough to prevent reordering already, isn't it?
No. It only makes sure that the asm will get executed, but does not prevent the compiler from reordering other statements around it.
static inline unsigned int readl(volatile unsigned int *a) { unsigned int v; asm volatile ("ldr\t%1, %0" : : "+o" (*a), "=r" (v)); return v; }
The bidirectional qualifier indicates that we're going to modify the memory pointed by a, and yet it is going to be different after that, which pretty much matches how hardware registers behave.
I don't see how the "+" is needed here, because the compiler won't be accessing the pointers otherwise, and doesn't have the option of omitting any accesses either.
It could prevents the compiler from assuming that the memory content at that location is still containing the same data that was just written/read, even if the volatile was omitted (note I'm not suggesting to remove 'volatile').
But the compiler doesn't know teh contents of the pointer anyway, so even if it assumes that it hasn't changed, that won't mean anything at all.
Arnd