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 <mdavidsaver@gmail.com>
---
 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;
 }
-- 
2.11.0

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 <mdavidsaver@gmail.com>

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

On 07/05/2018 08:39 PM, David Gibson wrote:
> 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 <mdavidsaver@gmail.com>
> 
> 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.

The fact that the above capture_current_time() included the line

>     if (s->nvram[2] & HOURS_12) {

was enough to convince me that the original author intended to persist
the 12/24 hour mode in this way.  There are certainly other ways to
accomplish this, but they would involved adding to the vmstate,
which I've tried to avoid in this iteration.


Also, I though I had test coverage of this bug.  That's actually how I
noticed it to begin with.  But it seems my later change to allow for a
slow test runner also stopped testing readback of the 12/24 hour mode bit.
It just silently uses whichever it reads.  I'll be re-issuing an updated
version which restores this check.  Then you will be able to easily
see the effect of reverting 'timer: ds1338 persist 12-hour mode selection'.



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

On 07/05/2018 09:35 PM, Michael Davidsaver wrote:
> Also, I though I had test coverage of this bug.  That's actually how I
> noticed it to begin with.  But it seems my later change to allow for a
> slow test runner also stopped testing readback of the 12/24 hour mode bit.
> It just silently uses whichever it reads.  I'll be re-issuing an updated
> version which restores this check.  Then you will be able to easily
> see the effect of reverting 'timer: ds1338 persist 12-hour mode selection'.

I've posted revised versions of two of the three test patches #6 and #7
(which I've hopefully posted correctly...).  #6 again tests for this issue.
So you can demonstrate the problem by either applying 1,2,4-6, or just 1 and 6
to see that the issue is present in the original implementation.

The test failure should be:

> test_rtc_set: assertion failed (mode_expect == mode_actual): (1 == 0)

Which shows that a write with 12 hour mode is read back as 24 hour mode.

Similarly, omitting patch #5 will cause the tests added in #7 to fail.

On Thu, Jul 05, 2018 at 09:35:50PM -0700, Michael Davidsaver wrote:
> On 07/05/2018 08:39 PM, David Gibson wrote:
> > 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 <mdavidsaver@gmail.com>
> > 
> > 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.
> 
> The fact that the above capture_current_time() included the line
> 
> >     if (s->nvram[2] & HOURS_12) {
> 
> was enough to convince me that the original author intended to persist
> the 12/24 hour mode in this way.  There are certainly other ways to
> accomplish this, but they would involved adding to the vmstate,
> which I've tried to avoid in this iteration.

Ah, yes, I see your point.  I was more unsure about whether it was
safe to also persist the other early bytes of the region, which this
patch also does.  But I can't really see how it would do any harm, so:

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> 
> 
> Also, I though I had test coverage of this bug.  That's actually how I
> noticed it to begin with.  But it seems my later change to allow for a
> slow test runner also stopped testing readback of the 12/24 hour mode bit.
> It just silently uses whichever it reads.  I'll be re-issuing an updated
> version which restores this check.  Then you will be able to easily
> see the effect of reverting 'timer: ds1338 persist 12-hour mode selection'.
> 
> 
> 
> >> ---
> >>  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