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

[Markus Armbruster]

BALATON Zoltan <address@hidden> writes:

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

Yes, SPD data does not require the number of banks to be a power of two.
But that's not why the loop above is wrong.  To see, let's execute it on
e-paper for type = SDR (thus max_log2 = 9) and ram_size = 2048 MiB:

    iteration   sz_log2  nbanks  bank size  total size
    0           11       1       2048 MiB   2048 MiB
    1           10       2       1024 MiB   2048 MiB
    2            9       3        512 MiB   1536 MiB    Oops!

The loop is wrong, because it fails to maintain its invariant

    nbanks * (1ull << sz_log2) == size

If you ever need magic to come up with nbanks that aren't powers of two,
you'll have to replace this loop.

But I'd rip it out instead, and ...

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

... have the board code pass the number of banks.

>> 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 <address@hidden>
>> ---
>> 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);
>>