lkml.org 
[lkml]   [2019]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable. variable.
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

\
 
 \ /
  Last update: 2019-07-18 18:35    [W:0.101 / U:0.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site