[U-Boot] [PATCH] nios2: convert cache flush to use dm cpu data

[Thomas Chou]

Hi Marek,

On 10/11/2015 08:15 PM, Marek Vasut wrote:
> On Sunday, October 11, 2015 at 02:38:35 AM, Thomas Chou wrote:
>> Hi Marek,
>
> Hi,
>
>> On 10/11/2015 02:18 AM, Marek Vasut wrote:
>>> Then you'd also need means to allocate variables to aligned memory
>>> location to prevent invalid cache flush. (Linux does this with it's DMA
>>> API). We are much simpler and thus this abstraction is still not
>>> available. I wonder if the overhead of DMA API would be high or not for
>>> U-Boot.
>>
>> I see most people use memalign(ARCH_DMA_MINALIGN, len) in u-boot to
>> allocate DMA buffers so that they are cache aligned.
>
> That and include/memalign.h , which contains macros that are used to align
> variables.

Yes. It is malloc_cache_aligned(), which should be used to allocate DMA 
buffer. Thank you for the pointer.

>
>>> It is even worse if the cache flush operators permit incorrect cache
>>> flushes or invalidations. Like I mentioned before, this can lead to hard
>>> to debug problems when using DMA (at least on ARM).
>>
>> I would suggest debug check should be left as for debug only. The
>> definition of common functions should be kept as it is more important
>> than coding style.
>
> Uh yes, that's what arm926 cache functions do, they're debug only.
>
>> I debugged DMA issues a lot in the past until I realized the importance
>> of aligned buffers. So there should be a developer's guideline.
>
> For what exactly?

For u-boot, every DMA buffer must be allocated with 
malloc_cache_aligned(). Then there will be not variables and DMA buffers 
cache racing issues as you describe below.

>
>> But it is even much more difficult when something you believed does not
>> work as expected, what is taken as common sense. It will trap a lot of
>> developers when they called your flush cache functions but was skipped
>> just because, eg, the end of packets are not aligned which is usually
>> the case.
>
> This is good, it should bite them, because this is a bug. If, on the other
> hand, you will paper over such bugs by adding crap to the cache ops, there
> will be even worse bugs coming for you, like variables which are sitting in
> the same cacheline as your unaligned buffer that you want to invalidate or
> flush will possibly get trashed by such cache operation.
>
> Consider this:
>
> cacheline 0: [ variable A ; buffer B ......... ]
> cacheline 1: [ buffer B ......... ; Empty .... ]
>
> Now you do the following:
>
> 1) Variable A = 0;
> 2) Flush buffer B (which is unaligned, so flush cacheline 0 and 1)
> 3) Start DMA to buffer B
> 4) Variable A = 1;
> 5) Check if DMA finished, it did
> 6) Invalidate buffer B ... oh, but it's unaligned, let's invalidate
>     everything around it, which is cacheline 0 and 1.
> 7) What is the value of variable A ? Oh, it's fetched from memory and
>     it's 0 there, even though we did set it to 1 ...
>
>> I would suggest that, with the best of my knowledge, please change the
>> range check to a debug probe, and restore the cache flush functions to
>> the common definition.
>
> See above, does my example make it clear why we should never ever hide
> bugs in the cache ops code ?

It is the drivers' responsibility to follow the guide line above. If 
there is such a bug, it is not the cache flush ops bug. It is a driver's 
bug. You may add a probe to show the bug from caller, but you may not 
call it a bug of cache ops and skip the flush. Given that it is quite 
common that the return of such cache ops is not checked, few (if not 
none) will ever know that the flush was skipped.

Best regards,
Thomas