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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
