[U-Boot] [PATCH 2/4] net: round up before calling flush_cache

[Mike Frysinger]

On Sunday 01 April 2012 23:34:28 Marek Vasut wrote:
> Dear Mike Frysinger,
> > On Sunday 01 April 2012 21:44:39 Marek Vasut wrote:
> > > Dear Mike Frysinger,
> > > > On Sunday 01 April 2012 17:00:56 Marek Vasut wrote:
> > > > > Dear Mike Frysinger,
> > > > > > On Sunday 01 April 2012 09:22:59 Stefano Babic wrote:
> > > > > > > If the range passed to flush_cache is not multiple
> > > > > > > of ARCH_DMA_MINALIGN, a warning due to mislaignment
> > > > > > > is printed.
> > > > > > > Detected with fec_mxc, mx35 boards:
> > > > > > > 
> > > > > > > CACHE: Misaligned operation at range [80800000, 8083c310]
> > > > > > 
> > > > > > warning on flushing is broken.  the
> > > > > > arch/arm/cpu/arm926ejs/cache.c code should probably be fixed
> > > > > > instead.
> > > > > 
> > > > > Why exactly?
> > > > 
> > > > the flush isn't harmful (ignoring the fact that a few extra bytes
> > > > might get written back to external memory), and the data isn't
> > > > evicted from cache. after all, we aren't talking about invalidate
> > > > here, we're talking about flush.
> > > 
> > > Right ... and can you be sure nothing important is overwritten in RAM?
> > 
> > except i'd bet money you're already running dcache in writethrough mode,
> > so the flush is largely irrelevant to this line of reasoning
> 
> Sure, but you can write your data, then the DMA happens and then you flush
> your stuff again. This is when you're screwed (all right, it's 5:30am
> here, I'm not confident anymore).

that's not how cache/dma works.  if you're going to dma into external memory, 
you *invalidate* it first, then dma it in.  there is no flush before/after that.  
if you're going to dma from external memory, then you *flush* it, then dma it 
out.  even then, the extra flushing is rarely a problem.

> > > > plus, no other arch (linux or u-boot) does this.
> > > > 
> > > > so the better question is, why exactly should you be warning ?  you
> > > > should provide justification when doing something unusual ...
> > > 
> > > Because you can destroy data in DRAM that arrived there by DMA transfer
> > > for example?
> > 
> > that isn't the problem of the flush functions.  there would have been an
> > invalidate call at some point with misaligned addresses, iff it actually
> > mattered.  you could argue for invalidation triggering a warning, but
> > that isn't what we're talking about.  and still, linux doesn't trigger
> > warnings, and i think only one other arm soc does atm in u-boot.
> 
> Ain't that because linux uses aligned buffers ?

not always because it isn't always necessary.  and even if they aren't 
aligned, linux doesn't check in the cache functions for alignment.

> > as already shown here, the flush call was perfectly fine, and adding
> > roundup to that call site is a waste of code space to "fix" something
> > that isn't a problem.
> 
> I believe unaligned flush in uboot should always trigger a warning, not do
> alignment for the programmer.

the hardware is going to do the alignment already.  you can't flush explicit 
bytes, only cache lines.  the API says that you must do the fix ups because 
you're required to operate on at least the range given to you.  if the 
hardware means you end up doing a few bytes before or after, then so be it.  
if that behavior is incorrect in the calling code, then the calling code 
should be fixed, but *only* if it's actually a bad thing.

> The programmer knows the best how the buffer looks (aka. if you embed the
> buffer in some structure, it might be problem).

this statement goes against your previous one.  as the caller, the programmer 
*does* know best as to when the alignment is required, or flushing too much is 
OK.  in this case, the proposed patch is useless overhead.

i don't know what problem you're trying to solve here, but it doesn't seem to 
be a real one.  other arches have been running just fine in linux/u-boot -- 
Blackfin certainly has been running with i/d cache for as long as i've been 
working on it (which is probably over 5 years at this point).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120402/b441f677/attachment.pgp>