The same statement is executed unconditionally right before the if statement.

Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>

---

The duplicate line may indicate a bug. I'm not familiar with the code, so this
patch may go into the wrong direction. Please check!
---
 hw/watchdog/wdt_imx2.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
index 6452fc4721..f9a7ea287f 100644
--- a/hw/watchdog/wdt_imx2.c
+++ b/hw/watchdog/wdt_imx2.c
@@ -39,7 +39,6 @@ static void imx2_wdt_expired(void *opaque)
 
     /* Perform watchdog action if watchdog is enabled */
     if (s->wcr & IMX2_WDT_WCR_WDE) {
-        s->wrsr = IMX2_WDT_WRSR_TOUT;
         watchdog_perform_action();
     }
 }
-- 
2.45.0

On 5/13/24 03:11, Bernhard Beschow wrote:
> The same statement is executed unconditionally right before the if statement.
> 
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> 
> ---
> 
> The duplicate line may indicate a bug. I'm not familiar with the code, so this
> patch may go into the wrong direction. Please check!

Should be ok. Technically the function should not be called to start with
if the watchdog isn't running. If it is, it might be useful to trace the content
of wcr and try to determine why the timer isn't stopped if  / when the watchdog
is disabled.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> ---
>   hw/watchdog/wdt_imx2.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
> index 6452fc4721..f9a7ea287f 100644
> --- a/hw/watchdog/wdt_imx2.c
> +++ b/hw/watchdog/wdt_imx2.c
> @@ -39,7 +39,6 @@ static void imx2_wdt_expired(void *opaque)
>   
>       /* Perform watchdog action if watchdog is enabled */
>       if (s->wcr & IMX2_WDT_WCR_WDE) {
> -        s->wrsr = IMX2_WDT_WRSR_TOUT;
>           watchdog_perform_action();
>       }
>   }

On 13/5/24 12:11, Bernhard Beschow wrote:
> The same statement is executed unconditionally right before the if statement.
> 
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> 
> ---
> 
> The duplicate line may indicate a bug. I'm not familiar with the code, so this
> patch may go into the wrong direction. Please check!

The bug might be in imx2_wdt_write() where the WRSR register is
overwritten, shouldn't we OR the SFTW bit, keeping other (such
TOUT) set?

     if (!(value & IMX2_WDT_WCR_SRS)) {
         s->wrsr = IMX2_WDT_WRSR_SFTW;
     }

> ---
>   hw/watchdog/wdt_imx2.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
> index 6452fc4721..f9a7ea287f 100644
> --- a/hw/watchdog/wdt_imx2.c
> +++ b/hw/watchdog/wdt_imx2.c
> @@ -39,7 +39,6 @@ static void imx2_wdt_expired(void *opaque)
>   
>       /* Perform watchdog action if watchdog is enabled */
>       if (s->wcr & IMX2_WDT_WCR_WDE) {
> -        s->wrsr = IMX2_WDT_WRSR_TOUT;
>           watchdog_perform_action();
>       }
>   }