Re: [Qemu-devel] [PATCH 03/14] timer: ds1338 persist 12-hour mode selection

[David Gibson]

On Thu, Jul 05, 2018 at 11:19:50AM -0700, Michael Davidsaver wrote:
11;rgb:ffff/ffff/ffff> Need to save HOUR[HOUR12] bit to keep
> track of guest selection of 12-hour mode.
> Write through current time registers to
> achieve this.  Will be overwritten
> by the next read/latch.
> 
> This was only being done in two of three
> arms of this conditional block.
> 
> Signed-off-by: Michael Davidsaver <address@hidden>

This looks dubious to me, or at least the explanation of it does.  The
other branch of the conditional is covering different registers in the
device, which are part of the RTC component, rather than the NVRAM
area.  I wouldn't necessarily expect them to persist data as a rule
the way the rest of the block does, even if this specific bit does
need to be preserved.

> ---
>  hw/timer/ds1338.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
> index 7298c5af43..b56db5852e 100644
> --- a/hw/timer/ds1338.c
> +++ b/hw/timer/ds1338.c
> @@ -220,10 +220,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
>             value unchanged. */
>          data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
>  
> -        s->nvram[s->ptr] = data;
> -    } else {
> -        s->nvram[s->ptr] = data;
>      }
> +    s->nvram[s->ptr] = data;
>      inc_regptr(s);
>      return 0;
>  }

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature