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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/20/20 3:28 PM, 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.
> 
> 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*

We would like to but lack testing so instead keep old (now duplicated) 
code :(

It should be easy to write SPD EEPROM tests although.

> 
> 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);
>