Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable. variable.

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Jul 18, 2019 at 08:52:38AM -0700, Mark Balantzyan wrote:
> ---

Subject and description are all messed up.

>  drivers/watchdog/alim1535_wdt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
> index 60f0c2eb..4ba2b860 100644
> --- a/drivers/watchdog/alim1535_wdt.c
> +++ b/drivers/watchdog/alim1535_wdt.c
> @@ -107,6 +107,7 @@ static void ali_keepalive(void)
>  
>  static int ali_settimer(int t)
>  {
> +    spin_lock(&ali_lock);
>      if (t < 0)
>          return -EINVAL;
>      else if (t < 60)
> @@ -117,7 +118,7 @@ static int ali_settimer(int t)
>          ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
>      else
>          return -EINVAL;

This return and the return above will exit the function with the
spinlock still active, which will guarantee a hangup if/when the
function is re-entered.

> -
> +    spin_unlock(&ali_lock);
>      timeout = t;

timeout is still unprotected and may have no relation to the
stored value of ali_timeout_bits.

Overall your patch would introduce much more severe problems
than the problem it tries to fix, and it doesn't even completely
fix that problem either.

I would suggest to leave the driver alone, unless you have the hardware
to test your changes. And, if you do, it would be much more valuable
to convert the driver to use the watchdog subsystem.

Thanks,
Guenter