Re: [PATCH 09/14] smbus-eeprom: remove PROP_PTR

[Corey Minyard <cminyard@mvista.com>]

On Fri, Oct 18, 2019 at 06:20:40PM +0100, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Instead, set the initial data field directly. Since it is only 256
> > bytes, let's simply copy it to avoid invalid pointers issues.
> 
> (Commit message says you copy the 256 bytes, but the code
> doesn't seem to do any copying?)
> 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Ah, the smbus code. Corey had a go at cleaning this up
> a while back; I can't remember if we found a reason why
> we had to keep the weird use of a pointer property here, or
> if we just wanted to avoid dragging yet another thing into
> an already large patchset.
> 
> What we're actually trying to set up here is always 256
> bytes of data. What's the right way to pass a QOM device
> a small chunk of data like that?

Yeah, we discussed this at the time, and IIRC we decided to leave it
like it is until someone came up with a use case.  My guess is that
the intent was to be able to pass in a buffer from the command line
or something like that, but that was never completed.

I'm ok with this change, the code is strange as it is, and I don't see
any uses of this propery in a quick scan of the code.

-corey

> 
> > ---
> >  hw/i2c/smbus_eeprom.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> > index 54c86a0112..533c728b3b 100644
> > --- a/hw/i2c/smbus_eeprom.c
> > +++ b/hw/i2c/smbus_eeprom.c
> > @@ -44,7 +44,7 @@
> >  typedef struct SMBusEEPROMDevice {
> >      SMBusDevice smbusdev;
> >      uint8_t data[SMBUS_EEPROM_SIZE];
> > -    void *init_data;
> > +    uint8_t *init_data;
> >      uint8_t offset;
> >      bool accessed;
> >  } SMBusEEPROMDevice;
> > @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev)
> >
> >  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> >  {
> > +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> > +
> >      smbus_eeprom_reset(dev);
> > +    if (eeprom->init_data == NULL) {
> > +        error_setg(errp, "init_data cannot be NULL");
> > +    }
> >  }
> >
> > -static Property smbus_eeprom_properties[] = {
> > -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> > -    DEFINE_PROP_END_OF_LIST(),
> > -};
> > -
> >  static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> >      dc->reset = smbus_eeprom_reset;
> >      sc->receive_byte = eeprom_receive_byte;
> >      sc->write_data = eeprom_write_data;
> > -    dc->props = smbus_eeprom_properties;
> >      dc->vmsd = &vmstate_smbus_eeprom;
> > -    /* Reason: pointer property "data" */
> > +    /* Reason: init_data */
> >      dc->user_creatable = false;
> >  }
> >
> > @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
> >
> >      dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
> >      qdev_prop_set_uint8(dev, "address", address);
> > -    qdev_prop_set_ptr(dev, "data", eeprom_buf);
> > +    SMBUS_EEPROM(dev)->init_data = eeprom_buf;
> >      qdev_init_nofail(dev);
> >  }
> >
> > --
> > 2.23.0.606.g08da6496b6
> 
> thanks
> -- PMM