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

[Philippe Mathieu-Daudé]

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?

>      eeprom->offset = 0;

This is correct, the offset reset belongs to the reset() function.

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