[PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Timur Tabi]

Normally, the watchdog is disabled when dev/watchdog is closed, but if
CONFIG_WATCHDOG_NOWAYOUT is defined, then it means that the watchdog should
remain enabled.  So we should disable it only if CONFIG_WATCHDOG_NOWAYOUT is
not defined.

Also ensure that /dev/watchdog is only opened by one process at a time.  That
way, a second process can't accidentally disable the watchdog while the first
process has it open.  There shouldn't be any need for more than one process to
open /dev/watchdog anyway.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

Kumar, please pick up this patch for 2.6.37.

 drivers/watchdog/booke_wdt.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c
index d11ffb0..636e013 100644
--- a/drivers/watchdog/booke_wdt.c
+++ b/drivers/watchdog/booke_wdt.c
@@ -193,8 +193,15 @@ static long booke_wdt_ioctl(struct file *file,
 	return 0;
 }
 
+/* wdt_is_active stores wether or not the /dev/watchdog device is opened */
+static unsigned long wdt_is_active;
+
 static int booke_wdt_open(struct inode *inode, struct file *file)
 {
+	/* /dev/watchdog can only be opened once */
+	if (test_and_set_bit(0, &wdt_is_active))
+		return -EBUSY;
+
 	spin_lock(&booke_wdt_lock);
 	if (booke_wdt_enabled == 0) {
 		booke_wdt_enabled = 1;
@@ -210,8 +217,17 @@ static int booke_wdt_open(struct inode *inode, struct file *file)
 
 static int booke_wdt_release(struct inode *inode, struct file *file)
 {
+#ifndef CONFIG_WATCHDOG_NOWAYOUT
+	/* Normally, the watchdog is disabled when /dev/watchdog is closed, but
+	 * if CONFIG_WATCHDOG_NOWAYOUT is defined, then it means that the
+	 * watchdog should remain enabled.  So we disable it only if
+	 * CONFIG_WATCHDOG_NOWAYOUT is not defined.
+	 */
 	on_each_cpu(__booke_wdt_disable, NULL, 0);
 	booke_wdt_enabled = 0;
+#endif
+
+	clear_bit(0, &wdt_is_active);
 
 	return 0;
 }
-- 
1.7.2.3

Re: [PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Josh Boyer]

On Fri, Dec 03, 2010 at 10:51:43AM -0600, Timur Tabi wrote:
>Normally, the watchdog is disabled when dev/watchdog is closed, but if
>CONFIG_WATCHDOG_NOWAYOUT is defined, then it means that the watchdog should
>remain enabled.  So we should disable it only if CONFIG_WATCHDOG_NOWAYOUT is
>not defined.
>
>Also ensure that /dev/watchdog is only opened by one process at a time.  That
>way, a second process can't accidentally disable the watchdog while the first
>process has it open.  There shouldn't be any need for more than one process to
>open /dev/watchdog anyway.
>
>Signed-off-by: Timur Tabi <timur@freescale.com>
>---
>
>Kumar, please pick up this patch for 2.6.37.
>
> drivers/watchdog/booke_wdt.c |   16 ++++++++++++++++
> 1 files changed, 16 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c
>index d11ffb0..636e013 100644
>--- a/drivers/watchdog/booke_wdt.c
>+++ b/drivers/watchdog/booke_wdt.c
>@@ -193,8 +193,15 @@ static long booke_wdt_ioctl(struct file *file,
> 	return 0;
> }
>
>+/* wdt_is_active stores wether or not the /dev/watchdog device is opened */
>+static unsigned long wdt_is_active;
>+
> static int booke_wdt_open(struct inode *inode, struct file *file)
> {
>+	/* /dev/watchdog can only be opened once */
>+	if (test_and_set_bit(0, &wdt_is_active))
>+		return -EBUSY;
>+
> 	spin_lock(&booke_wdt_lock);
> 	if (booke_wdt_enabled == 0) {
> 		booke_wdt_enabled = 1;

I'm confused why you can't use booke_wdt_enabled for the purposes of the
device having been opened.  It seems the use of the wdt_is_active
basically duplicates this functionalit (and oddly with the bit
manipulation instead of just atomic_inc/dec).

>@@ -210,8 +217,17 @@ static int booke_wdt_open(struct inode *inode, struct file *file)
>
> static int booke_wdt_release(struct inode *inode, struct file *file)
> {
>+#ifndef CONFIG_WATCHDOG_NOWAYOUT
>+	/* Normally, the watchdog is disabled when /dev/watchdog is closed, but
>+	 * if CONFIG_WATCHDOG_NOWAYOUT is defined, then it means that the
>+	 * watchdog should remain enabled.  So we disable it only if
>+	 * CONFIG_WATCHDOG_NOWAYOUT is not defined.
>+	 */
> 	on_each_cpu(__booke_wdt_disable, NULL, 0);
> 	booke_wdt_enabled = 0;
>+#endif
>+
>+	clear_bit(0, &wdt_is_active);

If you were to keep this variable instead of just using
booke_wdt_enabled, wouldn't it be more correct to have the clear_bit
only done inside the #ifndef?  The timer is very much still active if
NOWAYOUT is set...

josh

Re: [PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Timur Tabi]

Josh Boyer wrote:
> I'm confused why you can't use booke_wdt_enabled for the purposes of the
> device having been opened.  It seems the use of the wdt_is_active
> basically duplicates this functionalit (and oddly with the bit
> manipulation instead of just atomic_inc/dec).

Because the watchdog can be enabled even when the driver is not open.
booke_wdt_enabled is also initialized in setup_32.c.  So booke_wdt_enabled
represents the watchdog hardwre, whereas wdt_is_active represents the open
condition of /dev/watchdog.

However, now that I think about it, maybe that just causes confusion.  If the
watchdog is already running because of a command-line parameter, should we
prevent /dev/watchdog from ever being opened?  If you're okay with that, then I
can combine the two variables.

> If you were to keep this variable instead of just using
> booke_wdt_enabled, wouldn't it be more correct to have the clear_bit
> only done inside the #ifndef?  The timer is very much still active if
> NOWAYOUT is set...

In this case, yes.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Josh Boyer]

On Fri, Dec 03, 2010 at 12:22:30PM -0600, Timur Tabi wrote:
>Josh Boyer wrote:
>> I'm confused why you can't use booke_wdt_enabled for the purposes of the
>> device having been opened.  It seems the use of the wdt_is_active
>> basically duplicates this functionalit (and oddly with the bit
>> manipulation instead of just atomic_inc/dec).
>
>Because the watchdog can be enabled even when the driver is not open.
>booke_wdt_enabled is also initialized in setup_32.c.  So booke_wdt_enabled
>represents the watchdog hardwre, whereas wdt_is_active represents the open
>condition of /dev/watchdog.

Doh!  The big fat comment right above the variable would have told me
that had I bothered to read it.

(As an aside, the "For E500 cpus.." part of that comment also applies to
4xx.)

>However, now that I think about it, maybe that just causes confusion.  If the
>watchdog is already running because of a command-line parameter, should we
>prevent /dev/watchdog from ever being opened?  If you're okay with that, then I
>can combine the two variables.

No, you definitely want to allow something to open it.  If it's running
and nothing opens the device and calls write, then you have no way of
preventing the watchdog from just resetting your board every time.

I had a driver for a different mechanism that would do the equivalent of
booke_wdt_ping internally to the kernel until somthing opened the
device.  That way you could have the watchdog active during bootup and
allow userspace to takeover control when it was ready.  Then NOWAYOUT
dictated what happened if the userspace process exited or died (or
otherwise closed the device).  I'm not sure we want to do that for this
hardware, but it does illustrate the main use of a watchdog on a number
of boards.

I guess I don't really care either way if multiple processes have the
device open, as long as none of them can "disable" the watchdog if
NOWAYOUT is set.

>
>> If you were to keep this variable instead of just using
>> booke_wdt_enabled, wouldn't it be more correct to have the clear_bit
>> only done inside the #ifndef?  The timer is very much still active if
>> NOWAYOUT is set...
>
>In this case, yes.

ok.

josh

Re: [PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Timur Tabi]

Josh Boyer wrote:
> No, you definitely want to allow something to open it.  If it's running
> and nothing opens the device and calls write, then you have no way of
> preventing the watchdog from just resetting your board every time.

Now it's my turn to go Doh!  Of course you have to be able to open the driver in
order to ping the timer.

> I had a driver for a different mechanism that would do the equivalent of
> booke_wdt_ping internally to the kernel until somthing opened the
> device.  That way you could have the watchdog active during bootup and
> allow userspace to takeover control when it was ready.  Then NOWAYOUT
> dictated what happened if the userspace process exited or died (or
> otherwise closed the device).  I'm not sure we want to do that for this
> hardware, but it does illustrate the main use of a watchdog on a number
> of boards.
> 
> I guess I don't really care either way if multiple processes have the
> device open, as long as none of them can "disable" the watchdog if
> NOWAYOUT is set.

Well, this patch takes care of that problem.

So just to be clear, do you still have any issues with my patch?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Josh Boyer]

On Fri, Dec 03, 2010 at 01:10:02PM -0600, Timur Tabi wrote:
>> I guess I don't really care either way if multiple processes have the
>> device open, as long as none of them can "disable" the watchdog if
>> NOWAYOUT is set.
>
>Well, this patch takes care of that problem.
>
>So just to be clear, do you still have any issues with my patch?

Just the moving of the clear_bit inside the #ifndef
CONFIG_WATCHDOG_NOWAYOUT.

josh

Re: [PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Timur Tabi]

Josh Boyer wrote:
> Just the moving of the clear_bit inside the #ifndef
> CONFIG_WATCHDOG_NOWAYOUT.

If I move the clear_bit() call inside the #ifndef, then when
CONFIG_WATCHDOG_NOWAYOUT is defined, after a process closes /dev/watchdog, no
process will ever be able to open it again.  Are you saying that once
wd_keepalive exits, you don't want anyone to be able to open /dev/watchdog and
ping the timer again?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] watchdog: add CONFIG_WATCHDOG_NOWAYOUT support to PowerPC Book-E watchdog driver

[Josh Boyer]

On Fri, Dec 03, 2010 at 01:43:37PM -0600, Timur Tabi wrote:
>Josh Boyer wrote:
>> Just the moving of the clear_bit inside the #ifndef
>> CONFIG_WATCHDOG_NOWAYOUT.
>
>If I move the clear_bit() call inside the #ifndef, then when
>CONFIG_WATCHDOG_NOWAYOUT is defined, after a process closes /dev/watchdog, no
>process will ever be able to open it again.  Are you saying that once
>wd_keepalive exits, you don't want anyone to be able to open /dev/watchdog and
>ping the timer again?

Oh, good point.  No, I don't think we want that.  So in that case:

Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

and sorry for the hassle.

josh