The write operation to "wdt->timeout" is protected by
the lock on line 118, but the read operation to
this data on line 105 is not protected by the lock.
Thus, there may exist a data race for "wdt->timeout".
To fix this data race, the read operation to "wdt->timeout"
should be also protected by the lock.
Signed-off-by: Jia-Ju Bai <[email protected]>
---
drivers/watchdog/mena21_wdt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
index 25d5d2b8cfbe..05ca69042829 100644
--- a/drivers/watchdog/mena21_wdt.c
+++ b/drivers/watchdog/mena21_wdt.c
@@ -102,14 +102,15 @@ static int a21_wdt_set_timeout(struct watchdog_device *wdt,
return -EINVAL;
}
+ mutex_lock(&drv->lock);
+
if (timeout == 30 && wdt->timeout == 1) {
+ mutex_unlock(&drv->lock);
dev_err(wdt->parent,
"Transition from fast to slow mode not allowed\n");
return -EINVAL;
}
- mutex_lock(&drv->lock);
-
if (timeout == 1)
gpio_set_value(drv->gpios[GPIO_WD_FAST], 1);
else
--
2.17.0
On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
> The write operation to "wdt->timeout" is protected by
> the lock on line 118, but the read operation to
> this data on line 105 is not protected by the lock.
> Thus, there may exist a data race for "wdt->timeout".
>
> To fix this data race, the read operation to "wdt->timeout"
> should be also protected by the lock.
>
There is no race. There is already a mutex in the watchdog core which serializes
calls to the various API functions. It would make more sense to drop drv->lock
from the driver.
Guenter
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> drivers/watchdog/mena21_wdt.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
> index 25d5d2b8cfbe..05ca69042829 100644
> --- a/drivers/watchdog/mena21_wdt.c
> +++ b/drivers/watchdog/mena21_wdt.c
> @@ -102,14 +102,15 @@ static int a21_wdt_set_timeout(struct watchdog_device *wdt,
> return -EINVAL;
> }
>
> + mutex_lock(&drv->lock);
> +
> if (timeout == 30 && wdt->timeout == 1) {
> + mutex_unlock(&drv->lock);
> dev_err(wdt->parent,
> "Transition from fast to slow mode not allowed\n");
> return -EINVAL;
> }
>
> - mutex_lock(&drv->lock);
> -
> if (timeout == 1)
> gpio_set_value(drv->gpios[GPIO_WD_FAST], 1);
> else
>
On 2018/5/8 11:28, Guenter Roeck wrote:
> On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
>> The write operation to "wdt->timeout" is protected by
>> the lock on line 118, but the read operation to
>> this data on line 105 is not protected by the lock.
>> Thus, there may exist a data race for "wdt->timeout".
>>
>> To fix this data race, the read operation to "wdt->timeout"
>> should be also protected by the lock.
>>
>
> There is no race. There is already a mutex in the watchdog core which
> serializes
> calls to the various API functions. It would make more sense to drop
> drv->lock
> from the driver.
>
Thanks for your reply :)
Need I submit a patch of dropping all calls to "drv->lock"?
Best wishes,
Jia-Ju Bai
On 05/07/2018 08:32 PM, Jia-Ju Bai wrote:
>
>
> On 2018/5/8 11:28, Guenter Roeck wrote:
>> On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
>>> The write operation to "wdt->timeout" is protected by
>>> the lock on line 118, but the read operation to
>>> this data on line 105 is not protected by the lock.
>>> Thus, there may exist a data race for "wdt->timeout".
>>>
>>> To fix this data race, the read operation to "wdt->timeout"
>>> should be also protected by the lock.
>>>
>>
>> There is no race. There is already a mutex in the watchdog core which serializes
>> calls to the various API functions. It would make more sense to drop drv->lock
>> from the driver.
>>
>
> Thanks for your reply :)
> Need I submit a patch of dropping all calls to "drv->lock"?
>
You don't _need_ to, but I would happily give it my Reviewed-by: tag if you do.
Guenter
On 2018/5/8 11:42, Guenter Roeck wrote:
> On 05/07/2018 08:32 PM, Jia-Ju Bai wrote:
>>
>>
>> On 2018/5/8 11:28, Guenter Roeck wrote:
>>> On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
>>>> The write operation to "wdt->timeout" is protected by
>>>> the lock on line 118, but the read operation to
>>>> this data on line 105 is not protected by the lock.
>>>> Thus, there may exist a data race for "wdt->timeout".
>>>>
>>>> To fix this data race, the read operation to "wdt->timeout"
>>>> should be also protected by the lock.
>>>>
>>>
>>> There is no race. There is already a mutex in the watchdog core
>>> which serializes
>>> calls to the various API functions. It would make more sense to drop
>>> drv->lock
>>> from the driver.
>>>
>>
>> Thanks for your reply :)
>> Need I submit a patch of dropping all calls to "drv->lock"?
>>
>
> You don't _need_ to, but I would happily give it my Reviewed-by: tag
> if you do.
>
Okay, I will write a patch today :)
Best wishes,
Jia-Ju Bai