Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 18 Aug 2022 09:57:43 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Alexey Klimov <aklimov@redhat.com>
Cc:     linux-watchdog@vger.kernel.org, wim@linux-watchdog.org,
        linux-kernel@vger.kernel.org, klimov.linux@gmail.com
Subject: Re: [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to
 watchdog_open()
Message-ID: <20220818165743.GB923105@roeck-us.net>
References: <20220818020624.2386475-1-aklimov@redhat.com>
 <20220818020624.2386475-2-aklimov@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220818020624.2386475-2-aklimov@redhat.com>
Precedence: bulk

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@redhat.com>
> ---
>  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
>