diff mbox series

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

Message ID 20210709053107.1829304-3-andrew@aj.id.au (mailing list archive)
State New, archived
Headers show
Series wdt_aspeed: Fix behaviour of control register | expand

Commit Message

Andrew Jeffery July 9, 2021, 5:31 a.m. UTC
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(+)

Comments

Philippe Mathieu-Daudé July 9, 2021, 7:29 a.m. UTC | #1
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);
               }

>          }
>          break;
>      case WDT_RESET_WIDTH:
>
Andrew Jeffery July 13, 2021, 2:41 a.m. UTC | #2
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
Cédric Le Goater July 19, 2021, 3:54 p.m. UTC | #3
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>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  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;
>          }
>          break;
>      case WDT_RESET_WIDTH:
>
diff mbox series

Patch

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;
         }
         break;
     case WDT_RESET_WIDTH: