From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Message-ID: <542AA8E7.3030804@elproma.com.pl> Date: Tue, 30 Sep 2014 14:58:15 +0200 From: =?UTF-8?B?SmFudXN6IFXFvHlja2k=?= MIME-Version: 1.0 To: Guenter Roeck CC: Wim Van Sebroeck , linux-watchdog@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 1/6] watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature References: <542A3392.8020903@roeck-us.net> <1412081190-30699-1-git-send-email-j.uzycki@elproma.com.pl> <1412081190-30699-2-git-send-email-j.uzycki@elproma.com.pl> In-Reply-To: <1412081190-30699-2-git-send-email-j.uzycki@elproma.com.pl> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-ID: I sent also the changelog: [PATCH 1/6] watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature * clean up old comments * watchdog_keepon_start() renamed to watchdog_timer_start() * watchdog_keepon_stop() renamed to watchdog_timer_stop() * watchdog_timer_register() added * watchdog_timer_unregister() added * watchdog_timer_register() is the last action in watchdog_dev_register(). FIXME: Really should be in probe()? * TODO: should watchdog_timer_start() call watchdog_start()? * watchdog_release() uses watchdog_timer_restart() helper function * TODO: change WDOG_KEEP_ON to autostart module parameter Here I am still not sure about module parameters. > autostart: > Set to 0 to disable, -1 to enable with unlimited timeout, > or for an initial timeout of seconds. [1/6]: support of 0 and -1, where -1 is instead of WDOG_KEEP_ON? [2/6]: support of instead of boottime what about: * int init_timeout; /* initial timeout in seconds */ It looks as so why duplicate the autostart parameter? * keep_running It looks like -1 without watchdog_start() * this_watchdog_is_running() Where defined? * watchdog_set_autostart() How to split new patches? Janusz W dniu 2014-09-30 14:46, Janusz Uzycki pisze: > Some applications require to start watchdog before userspace software. > This patch enables such feature. Only the flag is necessary > to enable it. > Moreover kernel's ping is re-enabled when userspace software closed > watchdog using the magic character. The features improves kernel's > reliability if hardware watchdog is available. > > Signed-off-by: Janusz Uzycki > --- > drivers/watchdog/watchdog_dev.c | 65 ++++++++++++++++++++++++++- > include/linux/watchdog.h | 4 ++ > 2 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index 6aaefba..afc14f7 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -41,6 +41,7 @@ > #include /* For handling misc devices */ > #include /* For __init/__exit/... */ > #include /* For copy_to_user/put_user/... */ > +#include /* for ping timer */ > > #include "watchdog_core.h" > > @@ -277,6 +278,60 @@ out_ioctl: > return err; > } > > +static void watchdog_ping_timer_cb(unsigned long data) > +{ > + struct watchdog_device *wdd = (struct watchdog_device *)data; > + watchdog_ping(wdd); > + /* call next ping half the timeout value */ > + mod_timer(&wdd->ping_timer, > + jiffies + msecs_to_jiffies(wdd->timeout * 500)); > +} > + > +static void watchdog_timer_start(struct watchdog_device *wdd) > +{ > + /* TODO: watchdog_start(): > + * it should probably be handled by the caller, to let us separate > + * cases where the watchdog is already running (for example on close). */ > + watchdog_start(wdd); > + watchdog_ping_timer_cb((unsigned long)wdd); > +} > + > +static void watchdog_timer_stop(struct watchdog_device *wdd) > +{ > + del_timer_sync(&wdd->ping_timer); > +} > + > +static int watchdog_timer_restart(struct watchdog_device *wdd) > +{ > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + watchdog_timer_start(wdd); > + return 0; > + } > + return watchdog_stop(wdd); > +} > + > +static int watchdog_timer_register(struct watchdog_device *wdd) > +{ > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + if (!try_module_get(wdd->ops->owner)) > + return -ENODEV; > + setup_timer(&wdd->ping_timer, watchdog_ping_timer_cb, > + (unsigned long)wdd); > + watchdog_timer_start(wdd); > + } > + return 0; > +} > + > +static int watchdog_timer_unregister(struct watchdog_device *wdd) > +{ > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + watchdog_timer_stop(wdd); > + watchdog_stop(wdd); > + module_put(wdd->ops->owner); > + } > + return 0; > +} > + > /* > * watchdog_write: writes to the watchdog. > * @file: file from VFS > @@ -430,6 +485,9 @@ static int watchdog_open(struct inode *inode, struct file *file) > if (!try_module_get(wdd->ops->owner)) > goto out; > > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) > + watchdog_timer_stop(wdd); > + > err = watchdog_start(wdd); > if (err < 0) > goto out_mod; > @@ -473,7 +531,7 @@ static int watchdog_release(struct inode *inode, struct file *file) > err = 0; > else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > !(wdd->info->options & WDIOF_MAGICCLOSE)) > - err = watchdog_stop(wdd); > + err = watchdog_timer_restart(wdd); > > /* If the watchdog was not stopped, send a keepalive ping */ > if (err < 0) { > @@ -553,7 +611,10 @@ int watchdog_dev_register(struct watchdog_device *watchdog) > misc_deregister(&watchdog_miscdev); > old_wdd = NULL; > } > + return err; > } > + > + err = watchdog_timer_register(watchdog); > return err; > } > > @@ -575,6 +636,8 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) > misc_deregister(&watchdog_miscdev); > old_wdd = NULL; > } > + > + watchdog_timer_unregister(watchdog); > return 0; > } > > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h > index 2a3038e..0afeabd 100644 > --- a/include/linux/watchdog.h > +++ b/include/linux/watchdog.h > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include /* for ping timer */ > #include > > struct watchdog_ops; > @@ -95,6 +96,8 @@ struct watchdog_device { > #define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */ > #define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */ > #define WDOG_UNREGISTERED 4 /* Has the device been unregistered */ > +#define WDOG_KEEP_ON 5 /* Is 'keep on' feature set? */ > + struct timer_list ping_timer; /* timer to keep on hardware ping */ > }; > > #ifdef CONFIG_WATCHDOG_NOWAYOUT > @@ -104,6 +107,7 @@ struct watchdog_device { > #define WATCHDOG_NOWAYOUT 0 > #define WATCHDOG_NOWAYOUT_INIT_STATUS 0 > #endif > +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) > > /* Use the following function to check whether or not the watchdog is active */ > static inline bool watchdog_active(struct watchdog_device *wdd) From mboxrd@z Thu Jan 1 00:00:00 1970 From: j.uzycki@elproma.com.pl (=?UTF-8?B?SmFudXN6IFXFvHlja2k=?=) Date: Tue, 30 Sep 2014 14:58:15 +0200 Subject: [PATCH 1/6] watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature In-Reply-To: <1412081190-30699-2-git-send-email-j.uzycki@elproma.com.pl> References: <542A3392.8020903@roeck-us.net> <1412081190-30699-1-git-send-email-j.uzycki@elproma.com.pl> <1412081190-30699-2-git-send-email-j.uzycki@elproma.com.pl> Message-ID: <542AA8E7.3030804@elproma.com.pl> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org I sent also the changelog: [PATCH 1/6] watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature * clean up old comments * watchdog_keepon_start() renamed to watchdog_timer_start() * watchdog_keepon_stop() renamed to watchdog_timer_stop() * watchdog_timer_register() added * watchdog_timer_unregister() added * watchdog_timer_register() is the last action in watchdog_dev_register(). FIXME: Really should be in probe()? * TODO: should watchdog_timer_start() call watchdog_start()? * watchdog_release() uses watchdog_timer_restart() helper function * TODO: change WDOG_KEEP_ON to autostart module parameter Here I am still not sure about module parameters. > autostart: > Set to 0 to disable, -1 to enable with unlimited timeout, > or for an initial timeout of seconds. [1/6]: support of 0 and -1, where -1 is instead of WDOG_KEEP_ON? [2/6]: support of instead of boottime what about: * int init_timeout; /* initial timeout in seconds */ It looks as so why duplicate the autostart parameter? * keep_running It looks like -1 without watchdog_start() * this_watchdog_is_running() Where defined? * watchdog_set_autostart() How to split new patches? Janusz W dniu 2014-09-30 14:46, Janusz Uzycki pisze: > Some applications require to start watchdog before userspace software. > This patch enables such feature. Only the flag is necessary > to enable it. > Moreover kernel's ping is re-enabled when userspace software closed > watchdog using the magic character. The features improves kernel's > reliability if hardware watchdog is available. > > Signed-off-by: Janusz Uzycki > --- > drivers/watchdog/watchdog_dev.c | 65 ++++++++++++++++++++++++++- > include/linux/watchdog.h | 4 ++ > 2 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index 6aaefba..afc14f7 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -41,6 +41,7 @@ > #include /* For handling misc devices */ > #include /* For __init/__exit/... */ > #include /* For copy_to_user/put_user/... */ > +#include /* for ping timer */ > > #include "watchdog_core.h" > > @@ -277,6 +278,60 @@ out_ioctl: > return err; > } > > +static void watchdog_ping_timer_cb(unsigned long data) > +{ > + struct watchdog_device *wdd = (struct watchdog_device *)data; > + watchdog_ping(wdd); > + /* call next ping half the timeout value */ > + mod_timer(&wdd->ping_timer, > + jiffies + msecs_to_jiffies(wdd->timeout * 500)); > +} > + > +static void watchdog_timer_start(struct watchdog_device *wdd) > +{ > + /* TODO: watchdog_start(): > + * it should probably be handled by the caller, to let us separate > + * cases where the watchdog is already running (for example on close). */ > + watchdog_start(wdd); > + watchdog_ping_timer_cb((unsigned long)wdd); > +} > + > +static void watchdog_timer_stop(struct watchdog_device *wdd) > +{ > + del_timer_sync(&wdd->ping_timer); > +} > + > +static int watchdog_timer_restart(struct watchdog_device *wdd) > +{ > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + watchdog_timer_start(wdd); > + return 0; > + } > + return watchdog_stop(wdd); > +} > + > +static int watchdog_timer_register(struct watchdog_device *wdd) > +{ > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + if (!try_module_get(wdd->ops->owner)) > + return -ENODEV; > + setup_timer(&wdd->ping_timer, watchdog_ping_timer_cb, > + (unsigned long)wdd); > + watchdog_timer_start(wdd); > + } > + return 0; > +} > + > +static int watchdog_timer_unregister(struct watchdog_device *wdd) > +{ > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + watchdog_timer_stop(wdd); > + watchdog_stop(wdd); > + module_put(wdd->ops->owner); > + } > + return 0; > +} > + > /* > * watchdog_write: writes to the watchdog. > * @file: file from VFS > @@ -430,6 +485,9 @@ static int watchdog_open(struct inode *inode, struct file *file) > if (!try_module_get(wdd->ops->owner)) > goto out; > > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) > + watchdog_timer_stop(wdd); > + > err = watchdog_start(wdd); > if (err < 0) > goto out_mod; > @@ -473,7 +531,7 @@ static int watchdog_release(struct inode *inode, struct file *file) > err = 0; > else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > !(wdd->info->options & WDIOF_MAGICCLOSE)) > - err = watchdog_stop(wdd); > + err = watchdog_timer_restart(wdd); > > /* If the watchdog was not stopped, send a keepalive ping */ > if (err < 0) { > @@ -553,7 +611,10 @@ int watchdog_dev_register(struct watchdog_device *watchdog) > misc_deregister(&watchdog_miscdev); > old_wdd = NULL; > } > + return err; > } > + > + err = watchdog_timer_register(watchdog); > return err; > } > > @@ -575,6 +636,8 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) > misc_deregister(&watchdog_miscdev); > old_wdd = NULL; > } > + > + watchdog_timer_unregister(watchdog); > return 0; > } > > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h > index 2a3038e..0afeabd 100644 > --- a/include/linux/watchdog.h > +++ b/include/linux/watchdog.h > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include /* for ping timer */ > #include > > struct watchdog_ops; > @@ -95,6 +96,8 @@ struct watchdog_device { > #define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */ > #define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */ > #define WDOG_UNREGISTERED 4 /* Has the device been unregistered */ > +#define WDOG_KEEP_ON 5 /* Is 'keep on' feature set? */ > + struct timer_list ping_timer; /* timer to keep on hardware ping */ > }; > > #ifdef CONFIG_WATCHDOG_NOWAYOUT > @@ -104,6 +107,7 @@ struct watchdog_device { > #define WATCHDOG_NOWAYOUT 0 > #define WATCHDOG_NOWAYOUT_INIT_STATUS 0 > #endif > +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) > > /* Use the following function to check whether or not the watchdog is active */ > static inline bool watchdog_active(struct watchdog_device *wdd)