[PATCH] memsize: Make get_ram_size() work with arbitary RAM size

[Bin Meng]

Hi Wolfgang,

On Tue, Jul 14, 2020 at 1:18 AM Wolfgang Denk <wd at denx.de> wrote:
>
> Dear Bin Meng,
>
> In message <1594378641-26360-1-git-send-email-bmeng.cn at gmail.com> you wrote:
> >
> > Currently get_ram_size() only works with certain RAM size like 1GiB,
> > 2GiB, 4GiB, 8GiB, etc. Chanage the codes to work with any RAM size.
>
> I'm afraid I don't understand this change,  Can you please explain a
> bit more detailed what "any RAM size" means?

I meant "any RAM size" that is not power of two.

>
> The existing code should work fine with any RAM size that is a power
> of two (and the algoithm used is based on this assumption, so the
> code would fail if it is not met).

Unfortunately this limitation is not documented clearly.

>
> > -     long           save[BITS_PER_LONG - 1];
> > -     long           save_base;
> > -     long           cnt;
> > -     long           val;
> > -     long           size;
> > -     int            i = 0;
> > +     long save[BITS_PER_LONG - 1];
> > +     long save_base;
> > +     long cnt;
> > +     long val;
> > +     long size;
> > +     int i = 0;
>
> Why do you change the formatting here?  I can see no need for that?

I see the above are the only places in this file that do not follow
our coding practices. Since I was modifying this function, I think
it's fine to do the updates while we are here.

>
>
> > +     long n = maxsize / sizeof(long);
> >
> > -     for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) {
> > +     n = __ffs(n);
> > +     n = BIT(n);
> > +
> > +     for (cnt = n >> 1; cnt > 0; cnt >>= 1) {
>
> I can only speculate - but do you think that this will work for a
> memory size of - for example - 2.5 GiB as might result from combining
> two banks of 2 GiB resp. 512 MiB ?   I bet it doesn't.

Correct. This issue was exposed when I was testing U-Boot with QEMU on
RISC-V. With QEMU we can instantiate arbitrary RAM size for a given
machine.

>
> For correct operation (as originally intended) you would always
> specify a maxsize twice as large as the maximum possible memory size
> of a bank of memory, and the function would return the real size it
> found.

I don't think this function can work as expected. Even if I pass a
power-of-two RAM size to this function, it could still fail. Imagine
the target has 2GiB memory size, and I pass 8GiB as the maxsize to
this function. The function will hang when it tries to read/write
something at an address that is beyond 2GiB.

>
> Any other use, especially not checking one bank of memory at a time,
> will not work as intended.  And I have yet to see systems where
> the size of a bank of memory is not a power of two.
>
> So I feel what you are doing here is not an improvement, but a
> "workaround" for some incorrect usage.
>

Given what you mentioned the function limitation, we should update the
codes with the above power of two assumptions documented.

> I don't think we should accept this patch.
>

Regards,
Bin