[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SM
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus |
Date: |
Sat, 1 Dec 2018 18:43:24 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
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.