[U-Boot] [PATCH v2] ARM926ejs: Add routines to invalidate D-Cache

[Albert ARIBAUD]

Hi Aneesh,

(cutting quotation for readability)

Le 05/08/2011 16:59, Aneesh V a écrit :
> Hi Albert,

> I don't dispute that having buffers aligned is the ideal scenario. The
> question is about error-handling the situation when this requirement is
> not met.

I understand what you're trying to achieve in this respect, that is, 
make the code as correct as it can be under unspecified conditions. I 
believe we are differing in how we construe 'as correct as it can be': 
you want to make the implementation of the called function as correct as 
it can be' at the expense of introducing a non-intuitive behavior (flush 
while invalidating), while I prefer the overall system to be as correct 
as it can be by 'doing exactly what it says on the tin', i.e. 
invalidating only.

I believe  that the added intelligence of trying to perform as 
resiliently as possible is perfectly understandable, desirable and 
achievable in an operating system, but that in a bootloader, which has 
tighter constraints of size and speed notably, adding intelligence just 
to supplement lack of conformance in the code should be avoided.

>> Especially, I don't buy the argument that "the person who commits the
>> mistake of invalidating the un-aligned buffer is the one who is affected
>> and is likely to fix the issue soon". The issue might not appear right
>> after the call to flush is added; it might appear quite later, after
>> several reorganizations of the ordering of data in RAM, and affect some
>> completely unrelated person doing something completely unrelated.
>
> I don't get how a flush can affect an un-related person unless the
> surrounding buffer also happens to be a DMA receive buffer.

There you have it: it can. :)

More seriously: both leaving out the flush and doing it have issues when 
unaligned buffers are involved. Of these to imperfect solutions, I 
prefer the one that performs more efficiently in nominal cases.

> Please note
> that flush is something that can happen to any dirty line as part of
> the cache line replacement done by hardware when new lines are brought
> into the cache, it's not such a dangerous thing for normal memory
> locations.

For normal memory indeed, it does not matter. But as soon as you start 
using -- needing -- flushes and invalidates, that means we're not 
dealing with normal memory any more, but with memory shared with other 
devices, DMAs or CPUs -- but that is true for doing flushes at the ends 
of an invalidate region... and for not doing them. Both solutions are 
simply imperfect due to the simple fact that the cache line just might 
contain parts of *two* different DMA buffers.

> If the buffers are aligned, the flush operation will not get executed.
> So, what is the risk?

If the operation is not performed in the nominal case, then let's just 
not do it -- things have already gone wrong anyway.

>> Between an implementation that "should cause no issue" and an
>> implementation that "cannot cause issues", I definitely favor the
>> latter: so that's a no on my side to any flushing while invalidating a
>> range.
>
> Let's look at the different cases:
>
> Let A be an un-aligned DMA receive buffer. Let B be a buffer lying
> next to A belonging to another program.
>
> Case I - A is aligned
> 1. Invalidate with flush - no issue
> 2. Invalidate without flush - no issue
>
> Case II - A is un-aligned, B is DMA receive buffer:
> 1. Invalidate with flush - corrupts B if DMA is on-going.
> 2. Invalidate without flush - no issue.
>
> Case III - A is un-aligned, B is normal memory.
> 1. Invalidate with flush - A corrupted, no issue for B.
> 2. Invalidate without flush - no issue for A, B is corrupted.
>
> Case I doesn't cause any issue either way. Case II would be rather
> rare. If it happens, it's more likely that B belongs to the same driver
> owning A. Case III is the more common error case and you can see that
> Invalidate with Flush is better because it doesn't affect B.

I do realize situations which will result in issues are rare. But I do 
realize that adding the extra logic of flushing is overkill compared to 
what the driver designer should have done, i.e. aligning some buffers, 
and that it will not fix all cases.

Also, I don't believe (any more...) in words like 'rare', 'likely' or 
'probably'. I've gone through some pains myself about 'rare' conditions 
which actually were not that rare and cost me days, if not weeks on... 
rare... occasions.

So let's not try to solve out-of-spec conditions with hypotheses about 
'probable' situations it would or would not handle -- we're out of 
specs, we don't handle things any more. Let's just do 'what it says on 
the tin', and since the function says it invalidates, let's just 
invalidate. The code will be simpler, cleaner, more understandable for 
nominal case, and will noisily signal out-of-spec cases and possibly 
misbehave then, but any code would, so that's ok.

And lets fix alignement issues where that belongs, i.e. in the driver 
buffers alignment. :)

> best regards,
> Aneesh

Amicalement,
-- 
Albert.