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 <mdavidsa...@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 <da...@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
signature.asc
Description: PGP signature