Hi Marek,
First, a (long) preamble with some general considerations:
A. This patch does not fix an actual issues; it is a prospective patch, modifying code which so far has not malfunctioned, or at least has not been reported to malfunction.
B. My comments on the patch below are based on the general consideration that the effect of a memory clobber is to contrain the reordering of statements around the clobbering. For the sake of simplicity -- and serenity :) -- my comments are also made under the assumption that the clobber prevents any access (volatile or not, write or read) from crossing it.
C. Another general comment is that adding clobber to instructions other than barriers is IMO not a good thing and isb() should be used instead, for two reasons:
1) it mixes an implicit secondary purpose into a statement written for another, explicit purpose; this can drown the implicit purpose into oblivion, when it should actually be emphasized, which is the goal and effect of isb();
2) it mixes the ends and the means. The end of your patch is to put instruction barriers between statements so that their relative order is preserved during optimization; adding "memory" to the clobber list of an asm instruction that happens to be one of the statements is a means, but so is isb() with the added benefit that using isb() allows architectures to use whatever means (memory clobber, specialized instruction, other) are best for the arch.
D. My comments on the patch below are based on the current source code.
One could argue that this may change if the function becomes inline. While this is true, I do not consider this right now because 1) the function is a strong replacement of a weak symbol, which AFAIU is not compatible with inlining, and 2) this patch is against the current tree, not a potential future tree. If/when a patch arrives to make the function inline, we'll consider the implications and how to solve them *then*.
Ther may be another impact on this function, namely LTO; but --again, AFAIU -- LTO does not rewrite the inside of functions; it only optimizes between functions. And again, we'll deal with LTO if/when patches for LTO get submitted.
... and, there may be future changes not imagined here that will break things. We'll deal with them then.
On Wed, 10 Oct 2012 00:44:29 +0200, Marek Vasut marex@denx.de wrote:
Add memory barrier to cache invalidate and flush calls.
Signed-off-by: Marek Vasut marex@denx.de CC: Albert Aribaud albert.u.boot@aribaud.net Cc: Fabio Estevam festevam@gmail.com Cc: Otavio Salvador otavio@ossystems.com.br Cc: Stefano Babic sbabic@denx.de
arch/arm/cpu/arm926ejs/cache.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c index 2740ad7..1c67608 100644 --- a/arch/arm/cpu/arm926ejs/cache.c +++ b/arch/arm/cpu/arm926ejs/cache.c @@ -30,7 +30,7 @@
void invalidate_dcache_all(void) {
- asm volatile("mcr p15, 0, %0, c7, c6, 0\n" : : "r"(0));
- asm volatile("mcr p15, 0, %0, c7, c6, 0\n" : : "r"(0) : "memory");
}
This one is useless since there are no accesses in the function to be reordered.
void flush_dcache_all(void) @@ -67,7 +67,8 @@ void invalidate_dcache_range(unsigned long start, unsigned long stop) return;
while (start < stop) {
asm volatile("mcr p15, 0, %0, c7, c6, 1\n" : : "r"(start));
asm volatile("mcr p15, 0, %0, c7, c6, 1\n": : "r"(start) : "memory");}
This one is useless too, as the only access it could constrain is the one affecting start, which is also affected by the would-be-clobbered statement (and the enclosing while's condition, thus already preventing the compiler from reordering.
@@ -78,11 +79,12 @@ void flush_dcache_range(unsigned long start, unsigned long stop) return;
while (start < stop) {
asm volatile("mcr p15, 0, %0, c7, c14, 1\n" : : "r"(start));
asm volatile("mcr p15, 0, %0, c7, c14, 1\n": : "r"(start) : "memory");
Here again, the only access the clobber could constrain is the one affecting start, which is also affected by the would-be-clobbered statement (and the enclosing while's condition, thus already preventing the compiler from reordering.
- asm volatile("mcr p15, 0, %0, c7, c10, 4\n" : : "r"(0));
- asm volatile("mcr p15, 0, %0, c7, c10, 4\n" : : "r"(0) : "memory");
}
Now this asm statement might potentially move around as it does not have input or output dependencies that the compiler could possibly use to assess ordering constraints. I would thus suggest replacing the memory clobber with an 'isb();' placed on the line before the asm volatile, for the reasons indicated in part C of my (long) preamble above.
void flush_cache(unsigned long start, unsigned long size)
Amicalement,