Re: [PATCH 4/4] smbus: Fix spd_data_generate() for number of banks > 2

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

On Mon, 20 Apr 2020, Markus Armbruster wrote:
> spd_data_generate() splits @ram_size bytes into @nbanks RAM banks of
> 1 << sz_log2 MiB each, like this:
>
>    size = ram_size >> 20; /* work in terms of megabytes */
>    [...]
>    nbanks = 1;
>    while (sz_log2 > max_log2 && nbanks < 8) {
>        sz_log2--;
>        nbanks++;
>    }
>
> Each iteration halves the size of a bank, and increments the number of
> banks.  Wrong: it should double the number of banks.

Hmm, why? SPD data has: spd[5]: Number of RAM banks on module (1–255) (and 
for DDR2: Ranks-1 (0–7)). So nothing says it has to be power of 2 even if 
it's commonly 2 or 4. Does this break anything that needs this to be power 
of 2? Otherwise I thik this change is wrong.

> The bug goes back all the way to commit b296b664ab "smbus: Add a
> helper to generate SPD EEPROM data".
>
> It can't bite because spd_data_generate()'s current users pass only
> @ram_size that result in *zero* iterations:
>
>    machine     RAM size    #banks  type    bank size
>    fulong2e     256 MiB         1   DDR      256 MiB
>    sam460ex    2048 MiB         1   DDR2    2048 MiB
>                1024 MiB         1   DDR2    1024 MiB
>                 512 MiB         1   DDR2     512 MiB
>                 256 MiB         1   DDR2     256 MiB
>                 128 MiB         1   SDR      128 MiB
>                  64 MiB         1   SDR       64 MiB
>                  32 MiB         1   SDR       32 MiB
>
> Apply the obvious, minimal fix.  I admit I'm tempted to rip out the
> unused (and obviously untested) feature instead, because YAGNI.
>
> Note that this is not the final result, as spd_data_generate() next
> increases #banks from 1 to 2 if possible.  This is done "to avoid a
> bug in MIPS Malta firmware".  We don't even use this function with
> machine type malta.  *Shrug*

The code that is generalised here is originally from MIPS Malta and the 
idea was to change that as well to use this but nobody did that so far.

Regards,
BALATON Zoltan

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/i2c/smbus_eeprom.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 07fbbf87f1..e199fc8678 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -229,7 +229,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
>     nbanks = 1;
>     while (sz_log2 > max_log2 && nbanks < 8) {
>         sz_log2--;
> -        nbanks++;
> +        nbanks *= 2;
>     }
>
>     assert(size == (1ULL << sz_log2) * nbanks);
>