Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus

[Philippe Mathieu-Daudé]

On 1/12/18 12:57, Peter Maydell wrote:
> On Fri, 30 Nov 2018 at 20:47, Corey Minyard <address@hidden> wrote:
>>
>> On 11/30/18 11:39 AM, Peter Maydell wrote:
>>> On Mon, 26 Nov 2018 at 20:04, <address@hidden> wrote:
>>>> From: Philippe Mathieu-Daudé <address@hidden>
>>>>        /* XXX: make this persistent */
>>>> -    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
>>>> +    eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE);
>>> So if we allocate N buffers as the caller requests, what
>>> is the thing that means that more than 8 won't work ?
>>>
>>> We've now changed from allocating always 8 lots of
>>> the EEPROM size to possibly allocating fewer than that.
>>> How does the code in the device know what the size
>>> of the buffer we're passing as the "data" property is
>>> now? We don't pass it the number of EEPROMs as a property.
>>
>> It doesn't have to.  Each EEPROM is 256 bytes and that's all the data
>> it has.
>>
>> But this whole thing is confusing, I agree.  The more I look at this
>> particular file, the less I like it.  But the caller is basically saying:
>> "I need N EEPROMs, here's the initialization data".  That's not
>> really very flexible, IMHO the EEPROM devices should be created
>> individually using standard qemu methods.
> 
> Oh, yes, I see now. We pass in a block of N * 256 bytes, and
> the function then hands a pointer to offsets 0, 256, ...
> to each device it creates.
> 
> I definitely don't see why we need to say "only a maximum of
> 8" now, though -- where does that limit come from? If we
> get passed in an arbitrary number of EEPROMs X, and we allocate
> 256 * X bytes, and create X devices which each get one slice of
> the buffer, what goes wrong when X > 8 ?
> 
>> I'm tempted to rewrite this whole thing to make it cleaner.
> 
> It's certainly pretty awkward code. I think personally given
> this patchset is already pretty big I'd go for getting it into
> master first and then doing a followup with further cleanup.

As suggested Peter, I'd drop this patch (in favor of a later clean
rewrite) and simply add an assert().

Regards,

Phil.