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

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<address@hidden> 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 <address@hidden>

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?

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