* [PATCH RFC] watchdog: core: don't try to stop device if not running
@ 2013-04-05 16:09 Hector Palacios
2013-04-05 18:34 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Hector Palacios @ 2013-04-05 16:09 UTC (permalink / raw)
To: linux-watchdog, wim; +Cc: linux-kernel@vger.kernel.org
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>
---
drivers/watchdog/watchdog_dev.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ef8edec..adadbdd 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -463,24 +463,28 @@ out:
static int watchdog_release(struct inode *inode, struct file *file)
{
struct watchdog_device *wdd = file->private_data;
- int err = -EBUSY;
- /*
- * 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))
- err = watchdog_stop(wdd);
-
- /* If the watchdog was not stopped, send a keepalive ping */
- if (err < 0) {
- mutex_lock(&wdd->lock);
- if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
- dev_crit(wdd->dev, "watchdog did not stop!\n");
- mutex_unlock(&wdd->lock);
- watchdog_ping(wdd);
+ if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+ int err = -EBUSY;
+
+ /*
+ * 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)) {
+ err = watchdog_stop(wdd);
+ }
+
+ /* If the watchdog was not stopped, send a keepalive ping */
+ if (err < 0) {
+ mutex_lock(&wdd->lock);
+ if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
+ dev_crit(wdd->dev, "watchdog did not stop!\n");
+ mutex_unlock(&wdd->lock);
+ watchdog_ping(wdd);
+ }
}
/* Allow the owner module to be unloaded again */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] watchdog: core: don't try to stop device if not running
2013-04-05 16:09 [PATCH RFC] watchdog: core: don't try to stop device if not running Hector Palacios
@ 2013-04-05 18:34 ` Guenter Roeck
2013-04-08 7:48 ` Hector Palacios
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2013-04-05 18:34 UTC (permalink / raw)
To: Hector Palacios; +Cc: linux-watchdog, wim, linux-kernel@vger.kernel.org
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.
Thanks,
Guenter
> ---
> drivers/watchdog/watchdog_dev.c | 38 +++++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index ef8edec..adadbdd 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -463,24 +463,28 @@ out:
> static int watchdog_release(struct inode *inode, struct file *file)
> {
> struct watchdog_device *wdd = file->private_data;
> - int err = -EBUSY;
>
> - /*
> - * 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))
> - err = watchdog_stop(wdd);
> -
> - /* If the watchdog was not stopped, send a keepalive ping */
> - if (err < 0) {
> - mutex_lock(&wdd->lock);
> - if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
> - dev_crit(wdd->dev, "watchdog did not stop!\n");
> - mutex_unlock(&wdd->lock);
> - watchdog_ping(wdd);
> + if (test_bit(WDOG_ACTIVE, &wdd->status)) {
> + int err = -EBUSY;
> +
> + /*
> + * 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)) {
> + err = watchdog_stop(wdd);
> + }
> +
> + /* If the watchdog was not stopped, send a keepalive ping */
> + if (err < 0) {
> + mutex_lock(&wdd->lock);
> + if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
> + dev_crit(wdd->dev, "watchdog did not stop!\n");
> + mutex_unlock(&wdd->lock);
> + watchdog_ping(wdd);
> + }
> }
>
> /* Allow the owner module to be unloaded again */
> --
> 1.7.9.5
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] watchdog: core: don't try to stop device if not running
2013-04-05 18:34 ` Guenter Roeck
@ 2013-04-08 7:48 ` Hector Palacios
2013-04-08 8:16 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Hector Palacios @ 2013-04-08 7:48 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog@vger.kernel.org, wim@iguana.be,
linux-kernel@vger.kernel.org
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] watchdog: core: don't try to stop device if not running
2013-04-08 7:48 ` Hector Palacios
@ 2013-04-08 8:16 ` Guenter Roeck
2013-04-08 8:43 ` Hector Palacios
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2013-04-08 8:16 UTC (permalink / raw)
To: Hector Palacios
Cc: linux-watchdog@vger.kernel.org, wim@iguana.be,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] watchdog: core: don't try to stop device if not running
2013-04-08 8:16 ` Guenter Roeck
@ 2013-04-08 8:43 ` Hector Palacios
0 siblings, 0 replies; 5+ messages in thread
From: Hector Palacios @ 2013-04-08 8:43 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog@vger.kernel.org, wim@iguana.be,
linux-kernel@vger.kernel.org
On 04/08/2013 10:16 AM, Guenter Roeck wrote:
> 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.
Oops. Remainders of a debug message. Thanks for pointing out.
--
Héctor Palacios
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-08 8:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-05 16:09 [PATCH RFC] watchdog: core: don't try to stop device if not running Hector Palacios
2013-04-05 18:34 ` Guenter Roeck
2013-04-08 7:48 ` Hector Palacios
2013-04-08 8:16 ` Guenter Roeck
2013-04-08 8:43 ` Hector Palacios
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox