diff mbox series

[v3,16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

Message ID 20181126200435.23408-17-minyard@acm.org (mailing list archive)
State New, archived
Headers show
Series Fix/add vmstate handling in some I2C code | expand

Commit Message

Corey Minyard Nov. 26, 2018, 8:04 p.m. UTC
From: Corey Minyard <cminyard@mvista.com>

Reset the contents to init data and reset the offset on a machine
reset.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/smbus_eeprom.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Nov. 26, 2018, 8:42 p.m. UTC | #1
Hi Corey,

On 26/11/18 21:04, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Reset the contents to init data and reset the offset on a machine
> reset.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  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;
>
Corey Minyard Nov. 26, 2018, 10:41 p.m. UTC | #2
On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 26/11/18 21:04, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Reset the contents to init data and reset the offset on a machine
>> reset.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   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;
>>
Philippe Mathieu-Daudé Nov. 26, 2018, 11:01 p.m. UTC | #3
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, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> Reset the contents to init data and reset the offset on a machine
>>> reset.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> ---
>>>   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;
>>>
>
Corey Minyard Nov. 26, 2018, 11:58 p.m. UTC | #4
On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
> 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, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Reset the contents to init data and reset the offset on a machine
>>>> reset.
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>    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.


It starts at 
http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html

The patch set was fairly different at that point.


>> 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.
>
Yeah, it was mentioned that to do this correctly would require a block 
backend.
I'll let others comment on the correctness of this, I guess.  It's a 
separate patch
so it can be easily dropped.

The current code is far too broken for anyone to be using it, so we won't be
breaking any current users, I don't think.

-corey

>> -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;
>>>>
Philippe Mathieu-Daudé Nov. 27, 2018, 10:54 a.m. UTC | #5
On 27/11/18 0:58, Corey Minyard wrote:
> On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>> 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, minyard@acm.org wrote:
>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>
>>>>> Reset the contents to init data and reset the offset on a machine
>>>>> reset.
>>>>>
>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>> ---
>>>>>    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.
> 
> 
> It starts at
> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html

Thank you, very helpful.

> 
> The patch set was fairly different at that point.
> 
> 
>>> 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.
>>
> Yeah, it was mentioned that to do this correctly would require a block
> backend.
> I'll let others comment on the correctness of this, I guess.  It's a
> separate patch
> so it can be easily dropped.

Since modelling eeprom data retention on hardware reset isn't the goal
of your series, we can have a consensus, adding a comment explaining why
we choose this simpler way, and eeprom retention simulation requieres
more work with block backend.

> 
> The current code is far too broken for anyone to be using it, so we
> won't be
> breaking any current users, I don't think.
> 
> -corey
> 
>>> -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;
>>>>>
>
Corey Minyard Nov. 27, 2018, 12:58 p.m. UTC | #6
On 11/27/18 4:54 AM, Philippe Mathieu-Daudé wrote:
> On 27/11/18 0:58, Corey Minyard wrote:
>> On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>>> 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, minyard@acm.org wrote:
>>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>>
>>>>>> Reset the contents to init data and reset the offset on a machine
>>>>>> reset.
>>>>>>
>>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>>> ---
>>>>>>     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.
>>
>> It starts at
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html
> Thank you, very helpful.
>
>> The patch set was fairly different at that point.
>>
>>
>>>> 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.
>>>
>> Yeah, it was mentioned that to do this correctly would require a block
>> backend.
>> I'll let others comment on the correctness of this, I guess.  It's a
>> separate patch
>> so it can be easily dropped.
> Since modelling eeprom data retention on hardware reset isn't the goal
> of your series, we can have a consensus, adding a comment explaining why
> we choose this simpler way, and eeprom retention simulation requieres
> more work with block backend.

Good idea, I've done that.  Thanks for the reviews.  Is this a 
"reviewed-by"?


-corey

>> The current code is far too broken for anyone to be using it, so we
>> won't be
>> breaking any current users, I don't think.
>>
>> -corey
>>
>>>> -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;
>>>>>>
Philippe Mathieu-Daudé Nov. 27, 2018, 1:27 p.m. UTC | #7
On 27/11/18 13:58, Corey Minyard wrote:
> On 11/27/18 4:54 AM, Philippe Mathieu-Daudé wrote:
>> On 27/11/18 0:58, Corey Minyard wrote:
>>> On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>>>> 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, minyard@acm.org wrote:
>>>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>>>
>>>>>>> Reset the contents to init data and reset the offset on a machine
>>>>>>> reset.
>>>>>>>
>>>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>>>> ---
>>>>>>>     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.
>>>
>>> It starts at
>>> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html
>> Thank you, very helpful.
>>
>>> The patch set was fairly different at that point.
>>>
>>>
>>>>> 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.
>>>>
>>> Yeah, it was mentioned that to do this correctly would require a block
>>> backend.
>>> I'll let others comment on the correctness of this, I guess.  It's a
>>> separate patch
>>> so it can be easily dropped.
>> Since modelling eeprom data retention on hardware reset isn't the goal
>> of your series, we can have a consensus, adding a comment explaining why
>> we choose this simpler way, and eeprom retention simulation requieres
>> more work with block backend.
> 
> Good idea, I've done that.  Thanks for the reviews.  Is this a
> "reviewed-by"?

Why not ;)

With a comment:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> 
> -corey
> 
>>> The current code is far too broken for anyone to be using it, so we
>>> won't be
>>> breaking any current users, I don't think.
>>>
>>> -corey
>>>
>>>>> -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;
>>>>>>>
>
diff mbox series

Patch

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);
 
@@ -115,6 +115,11 @@  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
     eeprom->offset = 0;
 }
 
+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;