[U-Boot] [PATCH v2] memsize: Fix for bug in memory sizing code

[Gerd Hoffmann]

On Di, 2014-10-21 at 22:14 +0200, Wolfgang Denk wrote:
> Dear Gerd Hoffmann,
> 
> In message <1413910153-5907-1-git-send-email-kraxel at redhat.com> you wrote:
> > The original memory sizing code in get_ram_size clobbers the word
> > at the base address, but forgets to restore it.
> 
> The funny thing is that it avtually works on some boards.  Do you have
> an explanation for that?

Sure.  If the same piece of memory appears multiple times in the address
space the base address value is saved and restored multiple times too.

> > -	long           save[32];
> > +	long           save[32], save_base;
> 
> Why do you need another variable? The original code stores the value
> as last entry in save[] - what's wrong with that?

last entry index isn't known at the places where I save/restore the
value, so I did it this way.  Alternative approach would be to use the
first index instead and shift all the other values by one.  Matter of
taste, both will get the job done.

> > +	save_base = base[0];
> > +	sync ();
> 
> You add this code here, but...
> 
> >  	for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) {
> >  		addr = base + cnt;	/* pointer arith! */
> >  		sync ();
> > @@ -43,8 +46,6 @@ long get_ram_size(long *base, long maxsize)
> >  
> >  	addr = base;
> >  	sync ();
> > -	save[i] = *addr;
> > -	sync ();
> 
> ...remove it's equivalent here.  Why would your code be any better
> than the existing one?

There can be multiple saves/restores for the base memory location (see
above).  Therefore the ordering matters, restore must use the reverse
order of save.  No exception for base[0], so when we restore it last we
must save it first.

> 
> > -		*addr = save[i];
> >  		for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
> >  			addr  = base + cnt;
> >  			sync ();
> >  			*addr = save[--i];
> >  		}
> > +		base[0] = save_base;
> 
> Same here - the line you removed above does the very same as the one
> you add here.  In which way is the new code supposed to be different
> or even better?

... and we must restore it last everywhere.

> > @@ -73,10 +74,12 @@ long get_ram_size(long *base, long maxsize)
> >  				addr  = base + cnt;
> >  				*addr = save[--i];
> >  			}
> > +			base[0] = save_base;
> >  			return (size);
> >  		}
> >  	}
> >  
> > +	base[0] = save_base;
> >  	return (maxsize);
> 
> Agreed here. These two make sense to me. I still wonder why it works
> on the boards I used for testing, while it's failing on yours.
> 
> Which exit path are you taking? The one at the end of the function,
> i. e. "return (maxsize)" ?  What happens if you double the memory
> size to be checked?

Both cases can happen, depending on how much memory I configure for
qemu, and both are working correctly for me.

cheers,
  Gerd