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

[Marek Vasut <marex@denx.de>]

On Monday, October 12, 2015 at 02:34:16 AM, Thomas Chou wrote:
> Hi Marek,

Hi!

> 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.

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER()
macros which can be used to allocate such stuff on stack. And you sometimes
do want to allocate things on stack instead of using malloc().

> >>> 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.

Sometimes you might want to allocate DMA buffers on stack, for example if
you don't have mallocator running yet or if it's more convenient for some
reason. So forcing everyone to allocate DMA buffers using malloc is not
gonna slide I'm afraid.

> >> 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.

The cache flush ops is the best place to scream death and murder if someone
tries such unaligned cache operation, so maybe you should even do a printf()
there to weed such crappy drivers out for the 2016.01 release.

I agree it's the responsibility of the driver, so if the driver doesn't do
things right, it's a bug and the behavior of cache ops is undefined, which
might as well be that we do the safer thing here and flush nothing.

Best regards,
Marek Vasut