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

[Peter Maydell]

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.

thanks
-- PMM