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

The original memory sizing code in get_ram_size clobbers the word
at the base address, but forgets to restore it.

Unlike the (reverted) commit b8496cced856ff411f1eb2e4eff20f5abe7080b0
we'll go save and restore the base address value at the start and then
end of the function.  It needs to stay cleared until the detection is
done, otherwise we'll fail to detect the same piece of memory being
mapped multiple times into the address space.

Cc: Wolfgang Denk <wd@denx.de>
Cc: Iwo Mergler <Iwo.Mergler@netcommwireless.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 common/memsize.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Dear Gerd Hoffmann,

In message <1413910153-5907-1-git-send-email-kraxel@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?

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

> +	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?

> -		*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?

> @@ -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?


I'll resend a slightly reworked patch in a minute; could you please
test if this works for you, too?

Thanks.

Best regards,

Wolfgang Denk

On Di, 2014-10-21 at 22:14 +0200, Wolfgang Denk wrote:
> Dear Gerd Hoffmann,
> 
> In message <1413910153-5907-1-git-send-email-kraxel@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