diff mbox

[v2,4/6] hw/ds1338.c: Ensure state is properly initialized.

Message ID 50C7BA7A.5060705@gmail.com
State New
Headers show

Commit Message

Antoine Mathys Dec. 11, 2012, 10:58 p.m. UTC 
Signed-off-by: Antoine Mathys <barsamin@gmail.com>
---
  hw/ds1338.c |   10 ++++++++++
  1 file changed, 10 insertions(+)

Comments

Peter Maydell Dec. 12, 2012, 12:05 p.m. UTC | #1 
On 11 December 2012 22:58, Antoine Mathys <barsamin@gmail.com> wrote:
> Signed-off-by: Antoine Mathys <barsamin@gmail.com>
> ---
>  hw/ds1338.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/ds1338.c b/hw/ds1338.c
> index 0f88720..b4d5b74 100644
> --- a/hw/ds1338.c
> +++ b/hw/ds1338.c
> @@ -179,6 +179,15 @@ static int ds1338_init(I2CSlave *i2c)
>      return 0;
>  }
>
> +static void ds1338_reset(DeviceState *dev)
> +{
> +    DS1338State *s = FROM_I2C_SLAVE(DS1338State, I2C_SLAVE_FROM_QDEV(dev));
> +
> +    /* The clock is running and synchronized with the host */
> +    s->offset = 0;

I think we need to reset ptr and addr_byte too...

> +    memset(s->nvram, 0, NVRAM_SIZE);
> +}

-- PMM
Antoine Mathys Dec. 12, 2012, 12:46 p.m. UTC | #2 
On 12/12/2012 01:05 PM, Peter Maydell wrote:
> I think we need to reset ptr and addr_byte too...
The initial value of the register pointer is unspecified.

addr_byte is only used when receiving user data. It is written in 
ds1338_event() then read in ds1338_send(). Setting it in ds1338_reset() 
is unnecessary and would break encapsulation.
Peter Maydell Dec. 12, 2012, 1:02 p.m. UTC | #3 
On 12 December 2012 12:46, Antoine Mathys <barsamin@gmail.com> wrote:
> On 12/12/2012 01:05 PM, Peter Maydell wrote:
>> I think we need to reset ptr and addr_byte too...
>
> The initial value of the register pointer is unspecified.
>
> addr_byte is only used when receiving user data. It is written in
> ds1338_event() then read in ds1338_send(). Setting it in ds1338_reset() is
> unnecessary and would break encapsulation.

It's true that it probably doesn't make much difference, but
both the addr_byte and ptr are part of the migration state
listed in the vmstate struct. I think it is cleaner and
more straightforward to reason about if resetting the
device returns it to an exact known state. The state may be
undefined according to the specification but that doesn't
mean that our implementation has to leave it undefined.

I don't understand your point about encapsulation -- these
are both fields of the device state, and ds1338_reset()
is a method of the device and has every right to access
them.

-- PMM
Antoine Mathys Dec. 12, 2012, 1:11 p.m. UTC | #4 
> It's true that it probably doesn't make much difference, but
> both the addr_byte and ptr are part of the migration state
> listed in the vmstate struct. I think it is cleaner and
> more straightforward to reason about if resetting the
> device returns it to an exact known state. The state may be
> undefined according to the specification but that doesn't
> mean that our implementation has to leave it undefined.
>
> I don't understand your point about encapsulation -- these
> are both fields of the device state, and ds1338_reset()
> is a method of the device and has every right to access
> them.

You are right. While it is not strictly necessary it is cleaner (and 
safer) to set every field to a reasonable value. In addition some guest 
code probably incorrectly assumes that the pointer is initially at zero 
so that wouldn't hurt.

Would it be ok to reply with a new version of this patch only, instead 
of resending the whole series ?
Peter Maydell Dec. 12, 2012, 1:31 p.m. UTC | #5 
On 12 December 2012 13:11, Antoine Mathys <barsamin@gmail.com> wrote:
> Would it be ok to reply with a new version of this patch only, instead of
> resending the whole series ?

We generally prefer the whole series to be resent, it avoids
confusion about which versions of which patches are most
current. You might like to also expand the 5/6 commit message
a little since you're resending anyway.

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 0f88720..b4d5b74 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -179,6 +179,15 @@  static int ds1338_init(I2CSlave *i2c)
      return 0;
  }

+static void ds1338_reset(DeviceState *dev)
+{
+    DS1338State *s = FROM_I2C_SLAVE(DS1338State, I2C_SLAVE_FROM_QDEV(dev));
+
+    /* The clock is running and synchronized with the host */
+    s->offset = 0;
+    memset(s->nvram, 0, NVRAM_SIZE);
+}
+
  static void ds1338_class_init(ObjectClass *klass, void *data)
  {
      DeviceClass *dc = DEVICE_CLASS(klass);
@@ -188,6 +197,7 @@  static void ds1338_class_init(ObjectClass *klass, 
void *data)
      k->event = ds1338_event;
      k->recv = ds1338_recv;
      k->send = ds1338_send;
+    dc->reset = ds1338_reset;
      dc->vmsd = &vmstate_ds1338;
  }