On Thu, Aug 18, 2022 at 03:06:24AM +0100, Alexey Klimov wrote:
> watchdog_open() does not have wd_data->lock locks at all unlike
> the watchdog_release() for instance. Also watchdog_open() calls
> watchdog_start() that should be called with wd_data->lock acquired.
> The mutex lock should be acquired in the beginning of the function
> after getting struct watchdog_core_data wd_data pointer to deal with
> different status fields and be able to call watchdog_start(); and
> released on exit and on different error paths.
>
Again, I am missing which problem you are fixing here. You are making
a claim that a lock is needed, but you do not explain _why_ this is really
needed, why test_and_set_bit() is insufficient, and what problem is solved
that has been observed with the current code.
Guenter
> Signed-off-by: Alexey Klimov <aklimov@xxxxxxxxxx>
> ---
> drivers/watchdog/watchdog_dev.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 804236a094f6..d07a864036d9 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -836,7 +836,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
> struct watchdog_core_data *wd_data;
> struct watchdog_device *wdd;
> bool hw_running;
> - int err;
> + int err = -EBUSY;
>
> /* Get the corresponding watchdog device */
> if (imajor(inode) == MISC_MAJOR)
> @@ -845,9 +845,10 @@ static int watchdog_open(struct inode *inode, struct file *file)
> wd_data = container_of(inode->i_cdev, struct watchdog_core_data,
> cdev);
>
> + mutex_lock(&wd_data->lock);
> /* the watchdog is single open! */
> if (test_and_set_bit(_WDOG_DEV_OPEN, &wd_data->status))
> - return -EBUSY;
> + goto out_unlock;
>
> wdd = wd_data->wdd;
>
> @@ -856,10 +857,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
> * to be unloaded.
> */
> hw_running = watchdog_hw_running(wdd);
> - if (!hw_running && !try_module_get(wdd->ops->owner)) {
> - err = -EBUSY;
> + if (!hw_running && !try_module_get(wdd->ops->owner))
> goto out_clear;
> - }
>
> err = watchdog_start(wdd);
> if (err < 0)
> @@ -878,6 +877,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
> * applied.
> */
> wd_data->open_deadline = KTIME_MAX;
> + mutex_unlock(&wd_data->lock);
>
> /* dev/watchdog is a virtual (and thus non-seekable) filesystem */
> return stream_open(inode, file);
> @@ -886,6 +886,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
> module_put(wd_data->wdd->ops->owner);
> out_clear:
> clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
> +out_unlock:
> + mutex_unlock(&wd_data->lock);
> return err;
> }
>
> --
> 2.37.2
>
