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

[Michael Davidsaver]

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

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

signature.asc
Description: OpenPGP digital signature