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

[Philippe Mathieu-Daudé]

On 26/11/18 23:41, Corey Minyard wrote:
> 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

Hmm I missed it and can't find it (quick basic search). I only find
references about VMState.

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

Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle would.

Maybe we can argue QEMU system reset doesn't work correctly yet to use
this feature. Personally I wouldn't expect the EEPROM content be be
reset after a reset, but maybe I should rely on a block backend for a
such feature, and not the current simple approach.

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