Messages in this thread Patch in this message | | | Date | Mon, 8 Apr 2013 09:48:57 +0200 | From | Hector Palacios <> | Subject | Re: [PATCH RFC] watchdog: core: don't try to stop device if not running |
| |
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); + }
/* If the watchdog was not stopped, send a keepalive ping */
which makes the code more readable. Thanks, -- Héctor Palacios -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |