| Message ID | 20191018154212.13458-10-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: remove QDEV_PROP_PTR | expand |
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? > --- > 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
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
Hi On Fri, Oct 18, 2019 at 5:43 PM 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. Actually, the commit message is wrong. The patch used to introduce a init_data[256] array, and copy initial data there, but I changed my mind because of the risk to introduce regression as the original buffer may have changed. I will rephrase. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > 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 >
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); }
Instead, set the initial data field directly. Since it is only 256 bytes, let's simply copy it to avoid invalid pointers issues. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/i2c/smbus_eeprom.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)