Re: [PATCH RFC] watchdog: core: don't try to stop device if not running

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

On Mon, Apr 08, 2013 at 09:48:57AM +0200, Hector Palacios wrote:
> On 04/05/2013 08:34 PM, Guenter Roeck wrote:
> >On Fri, Apr 05, 2013 at 06:09:44PM +0200, Hector Palacios wrote:
> >>A watchdog device may be stopped from userspace using WDIOC_SETOPTIONS
> >>ioctl and flag WDIOS_DISABLECARD. If the device is closed after this
> >>operation, watchdog_release() is called and status bits checked for
> >>stopping it. Besides, if the device has not been unregistered a critical
> >>message "watchdog did not stop!" is printed, although the ioctl may have
> >>successfully stopped it already.
> >>
> >>Without the patch a user application sample code like this will successfully
> >>stop the watchdog, but the kernel will output the message
> >>"watchdog did not stop!":
> >>
> >>	wd_fd = open("/dev/watchdog", O_RDWR);
> >>
> >>	flags = WDIOS_DISABLECARD;
> >>	ioctl(wd_fd, WDIOC_SETOPTIONS, &flags);
> >>
> >>	close(wd_fd);
> >>
> >>Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> >
> >How about the following patch instead ?
> >
> >diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> >index 08b48bb..9775e8d 100644
> >--- a/drivers/watchdog/watchdog_dev.c
> >+++ b/drivers/watchdog/watchdog_dev.c
> >@@ -469,7 +469,9 @@ static int watchdog_release(struct inode *inode, struct file *file)
> >  	 * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then
> >  	 * watchdog_stop will fail.
> >  	 */
> >-	if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
> >+	if (!test_bit(WDOG_ACTIVE, &wdd->status))
> >+		err = 0;
> >+	else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
> >  	    !(wdd->info->options & WDIOF_MAGICCLOSE))
> >  		err = watchdog_stop(wdd);
> >
> >Much less invasive and the result is the same.
> 
> I like the simplicity but it is kind of inverted logic to initially
> define err = -EBUSY only to turn it to zero later, so I'm rebuilding
> your approach like this:
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index ef8edec..a4163cd 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -463,16 +463,19 @@ out:
>  static int watchdog_release(struct inode *inode, struct file *file)
>  {
>         struct watchdog_device *wdd = file->private_data;
> -       int err = -EBUSY;
> +       int err = 0;
> 
>         /*
>          * We only stop the watchdog if we received the magic character
>          * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then
>          * watchdog_stop will fail.
>          */
> -       if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
> -           !(wdd->info->options & WDIOF_MAGICCLOSE))
> +       if (test_bit(WDOG_ACTIVE, &wdd->status))
> +               err = -EBUSY;
> +       else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
> +           !(wdd->info->options & WDIOF_MAGICCLOSE)) {
>                 err = watchdog_stop(wdd);
> +       }

Ok, but the added { } are unnecessary and violate coding style rules.

Guenter