Hi,
On Sat, Mar 01, 2014 at 09:11:18PM -0800, Guenter Roeck wrote:
> On 02/28/2014 08:16 AM, Markus Pargmann wrote:
> >Many watchdog driver reset the watchdog device on initialization. This
> >is a problem if the watchdog is activated by the bootloader and should
> >be active the whole time until the userspace can write to it.
> >
>
> Shouldn't those watchdog drivers be fixed to keep the watchdog running
> if it was already active ? Otherwise there is still the time between
> driver initialization, which disables the watchdog, and it being re-enabled
> by watchdog registration.
Yes there is still a time between stopping watchdog and reenabling it.
But as mentioned in the other mail, I think this should be some generic
solution. Also the watchdog core would not know that the watchdog device
is active when the watchdog driver keeps it running.
>
> Also, does this affect or impact drivers which _do_ keep the watchdog
> enabled ?
I don't know of such a driver. However those drivers would have to
expect that they are started at some point through the watchdog core. At
least when someone opens the watchdog device and pings it.
>
> >This patch adds a module parameter (watchdog.activate_first) that
> >activates the first registered watchdog. Using this parameter it is
> >possible to have an active watchdog during the whole boot process.
> >
> >Signed-off-by: Markus Pargmann <mpa@xxxxxxxxxxxxxx>
> >---
> > drivers/watchdog/watchdog_core.c | 4 ++++
> > drivers/watchdog/watchdog_core.h | 4 ++++
> > drivers/watchdog/watchdog_dev.c | 21 ++++++++++++++++++++-
> > 3 files changed, 28 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> >index cec9b55..6db16eb 100644
> >--- a/drivers/watchdog/watchdog_core.c
> >+++ b/drivers/watchdog/watchdog_core.c
> >@@ -43,6 +43,10 @@
> > static DEFINE_IDA(watchdog_ida);
> > static struct class *watchdog_class;
> >
> >+unsigned int activate_first = 0;
>
> Bad variable name for a global variable.
>
> >+module_param(activate_first, uint, 0);
> >+MODULE_PARM_DESC(activate_first, "Activation timeout for first watchdog registered. 0 means no activation.");
> >+
>
> Does that _have_ to be in this file ? Would be much easier
> if it was in watchdog_dev.c, where it is used.
As far as I know, it has to be in this file, as this is the watchdog
module. Perhaps I can try to move it to watchdog_dev.c somehow.
I will fix the other comments if there is a second version of this
patch.
Thanks,
Markus
>
> > static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> > {
> > /*
> >diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> >index 6c95141..4526170 100644
> >--- a/drivers/watchdog/watchdog_core.h
> >+++ b/drivers/watchdog/watchdog_core.h
> >@@ -28,6 +28,10 @@
> >
> > #define MAX_DOGS 32 /* Maximum number of watchdog devices */
> >
> >+/* Activate first registered watchdog and set this as the timeout value
> >+ * (only != 0) */
>
> Coding style.
>
> >+extern unsigned int activate_first;
> >+
> > /*
> > * Functions/procedures to be called by the core
> > */
> >diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> >index 6aaefba..fe60fdc 100644
> >--- a/drivers/watchdog/watchdog_dev.c
> >+++ b/drivers/watchdog/watchdog_dev.c
> >@@ -553,8 +553,27 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
> > misc_deregister(&watchdog_miscdev);
> > old_wdd = NULL;
> > }
> >+
>
> Is that extra empty line really necessary ?
>
> >+ return err;
> > }
> >- return err;
> >+
> >+ if (activate_first && watchdog->id == 0) {
> >+ err = watchdog_set_timeout(watchdog, activate_first);
> >+ if (err && err != -EOPNOTSUPP) {
> >+ pr_err("watchdog%d failed to set timeout for first activation\n",
> >+ watchdog->id);
> >+ return err;
> >+ }
> >+
> >+ err = watchdog_start(watchdog);
> >+ if (err) {
> >+ pr_err("watchdog%d failed to start first watchdog\n",
> >+ watchdog->id);
> >+ return err;
> >+ }
> >+ }
> >+
> >+ return 0;
> > }
> >
> > /*
> >
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: Digital signature
