| Message ID | 1291395103-12394-1-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Kumar Gala |
| Headers | show |
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
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.
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
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?
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
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?
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
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; }
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(-)