Re: [PATCH 2/2] watchdog: aspeed: Fix sequential control writes

["Andrew Jeffery" <andrew@aj.id.au>]

On Fri, 9 Jul 2021, at 16:59, Philippe Mathieu-Daudé wrote:
> On 7/9/21 7:31 AM, Andrew Jeffery wrote:
> > The logic in the handling for the control register required toggling the
> > enable state for writes to stick. Rework the condition chain to allow
> > sequential writes that do not update the enable state.
> > 
> > Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  hw/watchdog/wdt_aspeed.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> > index faa3d35fdf21..69c37af9a6e9 100644
> > --- a/hw/watchdog/wdt_aspeed.c
> > +++ b/hw/watchdog/wdt_aspeed.c
> > @@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
> >          } else if (!enable && aspeed_wdt_is_enabled(s)) {
> >              s->regs[WDT_CTRL] = data;
> >              timer_del(s->timer);
> > +        } else {
> > +            s->regs[WDT_CTRL] = data;
> 
> What about simplifying by moving here:
> 
>                if (!enable && aspeed_wdt_is_enabled(s)) {
>                    timer_del(s->timer);
>                }
> 

I don't think that works, as aspeed_wdt_is_enabled() tests the value of 
s->regs[WDT_CTRL]. If you set it before you test then you end up in the 
wrong state.

Andrew