Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Corey Minyard]

On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 26/11/18 21:04, address@hidden wrote:
> > From: Corey Minyard <address@hidden>
> >
> > Reset the contents to init data and reset the offset on a machine
> > reset.
> >
> > Signed-off-by: Corey Minyard <address@hidden>
> > ---
> >   hw/i2c/smbus_eeprom.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> > index a0dcadbd60..de3a492df4 100644
> > --- a/hw/i2c/smbus_eeprom.c
> > +++ b/hw/i2c/smbus_eeprom.c
> > @@ -107,7 +107,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
> >       }
> >   };
> >   
> > -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> > +static void smbus_eeprom_reset(DeviceState *dev)
> >   {
> >       SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> >   
> 'git diff -U4' also shows this line:
>
>         memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>
> I don't think this is correct.
>
> One test I'd like to have is a machine booting, updating the EPROM then
> rebooting calling hw reset() to use the new values (BIOS use this).
>
> With this patch this won't work, you'll restore the EPROM content on
> each machine reset.
>
> I'd move the memcpy() call to the realize() function.
>
> What do you think?

There was some debate on this in the earlier patch set.  The general 
principle
is that a reset is the same as starting up qemu from scratch, so I added 
this
code based on that principle.  But I'm not really sure.

> >       eeprom->offset = 0;
> This is correct, the offset reset belongs to the reset() function.

Actually, on a real system, a hardware reset will generally not affect the
eeprom current offset register.  So if we don't take the above code, then
IMHO this is wrong, too.

-corey


> Regards,
>
> Phil.
>
> >   }
> >   
> > +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> > +{
> > +    smbus_eeprom_reset(dev);
> > +}
> > +
> >   static Property smbus_eeprom_properties[] = {
> >       DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> >       DEFINE_PROP_END_OF_LIST(),
> > @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, 
> > void *data)
> >       SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
> >   
> >       dc->realize = smbus_eeprom_realize;
> > +    dc->reset = smbus_eeprom_reset;
> >       sc->receive_byte = eeprom_receive_byte;
> >       sc->write_data = eeprom_write_data;
> >       dc->props = smbus_eeprom_properties;