[U-Boot] [PATCH 1/5] mips: cache: Bulletproof the code against cornercases

[Marek Vasut]

On Wednesday, January 27, 2016 at 03:56:36 PM, Daniel Schwierzeck wrote:
> Am 27.01.2016 um 03:13 schrieb Marek Vasut:
> > This patch makes sure that the flush/invalidate_dcache_range() functions
> > can handle corner-case calls like this -- invalidate_dcache_range(0, 0,
> > 0); This call is valid and is happily produced by USB EHCI code for
> > example. The expected behavior of the cache function(s) in this case is
> > that they will do no operation, since the size is zero.
> > 
> > The current implementation though has a problem where such invocation
> > will result in a hard CPU hang. This is because under such conditions,
> > where the start_addr = 0 and stop = 0, the addr = 0 and aend =
> > 0xffffffe0 . The loop will then try to iterate over the entire address
> > space, which in itself is wrong. But iterating over the entire address
> > space might also hit some odd address which will cause bus hang. The
> > later happens on the Atheros MIPS.
> > 
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> > Cc: Hans de Goede <hdegoede at redhat.com>
> > ---
> > 
> >  arch/mips/lib/cache.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/mips/lib/cache.c b/arch/mips/lib/cache.c
> > index bf8ff59..7482005 100644
> > --- a/arch/mips/lib/cache.c
> > +++ b/arch/mips/lib/cache.c
> > @@ -95,6 +95,10 @@ void flush_dcache_range(ulong start_addr, ulong stop)
> > 
> >  	const void *addr = (const void *)(start_addr & ~(lsize - 1));
> >  	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
> > 
> > +	/* aend will be miscalculated when size is zero, so we return here */
> > +	if (start_addr == stop)
> > +		return;
> > +
> 
> you could additionally move the initialization of addr and aend behind
> this check like it's done in flush_cache()? That would save some CPU
> cycles.

The compiler would do the reordering anyway, it's not volatile so it's placement
would not save any cycles.

> >  	while (1) {
> >  	
> >  		mips_cache(HIT_WRITEBACK_INV_D, addr);
> >  		if (addr == aend)
> > 
> > @@ -109,6 +113,10 @@ void invalidate_dcache_range(ulong start_addr, ulong
> > stop)
> > 
> >  	const void *addr = (const void *)(start_addr & ~(lsize - 1));
> >  	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
> > 
> > +	/* aend will be miscalculated when size is zero, so we return here */
> > +	if (start_addr == stop)
> > +		return;
> > +
> > 
> >  	while (1) {
> >  	
> >  		mips_cache(HIT_INVALIDATE_D, addr);
> >  		if (addr == aend)

Best regards,
Marek Vasut