* [PATCH] watchdog: core: add option to avoid early handling of watchdog
@ 2017-05-11 12:52 Sebastian Reichel
2017-05-11 13:47 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Reichel @ 2017-05-11 12:52 UTC (permalink / raw)
To: Sebastian Reichel, Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Sebastian Reichel
On some systems its desirable to have watchdog reboot the system
when it does not come up fast enough. This adds a kernel parameter
to disable the auto-update of watchdog before userspace takes over
and a kernel option to set the default. The info messages were
added to shorten error searching on misconfigured systems.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
drivers/watchdog/Kconfig | 11 +++++++++++
drivers/watchdog/watchdog_dev.c | 16 ++++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 52a70ee6014f..4e08b2b97941 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -46,6 +46,17 @@ config WATCHDOG_NOWAYOUT
get killed. If you say Y here, the watchdog cannot be stopped once
it has been started.
+config WATCHDOG_HANDLE_BOOT_ENABLED
+ bool "Disable watchdog shutdown on close"
+ default y
+ help
+ The default watchdog behaviour (which you get if you say Y here) is
+ to ping watchdog devices, that were enabled before the driver has
+ been loaded until control is taken over from userspace using the
+ /dev/watchdog file. If you say N here, the kernel will not update
+ the watchdog on its own. Thus if your userspace does not start fast
+ enough your device will reboot.
+
config WATCHDOG_SYSFS
bool "Read different watchdog information through sysfs"
help
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d5d2bbd8f428..5e6d6d4db82f 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -80,6 +80,9 @@ static struct watchdog_core_data *old_wd_data;
static struct workqueue_struct *watchdog_wq;
+static bool handle_boot_enabled =
+ IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
+
static inline bool watchdog_need_worker(struct watchdog_device *wdd)
{
/* All variables in milli-seconds */
@@ -956,6 +959,14 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
* and schedule an immediate ping.
*/
if (watchdog_hw_running(wdd)) {
+ if (!handle_boot_enabled) {
+ pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
+ wdd->id);
+ return 0;
+ }
+
+ pr_info("auto-update boot-enabled watchdog%d until userspace takes over\n",
+ wdd->id);
__module_get(wdd->ops->owner);
kref_get(&wd_data->kref);
queue_delayed_work(watchdog_wq, &wd_data->work, 0);
@@ -1106,3 +1117,8 @@ void __exit watchdog_dev_exit(void)
class_unregister(&watchdog_class);
destroy_workqueue(watchdog_wq);
}
+
+module_param(handle_boot_enabled, bool, 0444);
+MODULE_PARM_DESC(handle_boot_enabled,
+ "Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
+ __MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] watchdog: core: add option to avoid early handling of watchdog
2017-05-11 12:52 [PATCH] watchdog: core: add option to avoid early handling of watchdog Sebastian Reichel
@ 2017-05-11 13:47 ` Guenter Roeck
2017-05-12 12:05 ` [PATCHv2] " Sebastian Reichel
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2017-05-11 13:47 UTC (permalink / raw)
To: Sebastian Reichel, Sebastian Reichel, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel
On 05/11/2017 05:52 AM, Sebastian Reichel wrote:
> On some systems its desirable to have watchdog reboot the system
> when it does not come up fast enough. This adds a kernel parameter
> to disable the auto-update of watchdog before userspace takes over
> and a kernel option to set the default. The info messages were
> added to shorten error searching on misconfigured systems.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
> drivers/watchdog/Kconfig | 11 +++++++++++
> drivers/watchdog/watchdog_dev.c | 16 ++++++++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 52a70ee6014f..4e08b2b97941 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -46,6 +46,17 @@ config WATCHDOG_NOWAYOUT
> get killed. If you say Y here, the watchdog cannot be stopped once
> it has been started.
>
> +config WATCHDOG_HANDLE_BOOT_ENABLED
> + bool "Disable watchdog shutdown on close"
Enable ?
> + default y
> + help
> + The default watchdog behaviour (which you get if you say Y here) is
> + to ping watchdog devices, that were enabled before the driver has
> + been loaded until control is taken over from userspace using the
> + /dev/watchdog file. If you say N here, the kernel will not update
> + the watchdog on its own. Thus if your userspace does not start fast
> + enough your device will reboot.
> +
> config WATCHDOG_SYSFS
> bool "Read different watchdog information through sysfs"
> help
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index d5d2bbd8f428..5e6d6d4db82f 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -80,6 +80,9 @@ static struct watchdog_core_data *old_wd_data;
>
> static struct workqueue_struct *watchdog_wq;
>
> +static bool handle_boot_enabled =
> + IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
> +
> static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> {
> /* All variables in milli-seconds */
> @@ -956,6 +959,14 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> * and schedule an immediate ping.
> */
> if (watchdog_hw_running(wdd)) {
> + if (!handle_boot_enabled) {
> + pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
> + wdd->id);
> + return 0;
I don't like the immediate return here; if some other code is added further below,
it may miss this return. Something like
if (handle_boot_enabled) {
// do everything here
} else {
// print message
}
would be better.
> + }
> +
> + pr_info("auto-update boot-enabled watchdog%d until userspace takes over\n",
> + wdd->id);
This message will add a lot of noise. If you really think it is useful please make it
a debug message.
> __module_get(wdd->ops->owner);
> kref_get(&wd_data->kref);
> queue_delayed_work(watchdog_wq, &wd_data->work, 0);
> @@ -1106,3 +1117,8 @@ void __exit watchdog_dev_exit(void)
> class_unregister(&watchdog_class);
> destroy_workqueue(watchdog_wq);
> }
> +
> +module_param(handle_boot_enabled, bool, 0444);
> +MODULE_PARM_DESC(handle_boot_enabled,
> + "Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
> + __MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2] watchdog: core: add option to avoid early handling of watchdog
2017-05-11 13:47 ` Guenter Roeck
@ 2017-05-12 12:05 ` Sebastian Reichel
2017-05-14 14:47 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Reichel @ 2017-05-12 12:05 UTC (permalink / raw)
To: Sebastian Reichel, Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Sebastian Reichel
On some systems its desirable to have watchdog reboot the system
when it does not come up fast enough. This adds a kernel parameter
to disable the auto-update of watchdog before userspace takes over
and a kernel option to set the default. The info messages were
added to shorten error searching on misconfigured systems.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
Changes since PATCHv1:
* fix Kconfig short description (previously copied from
WATCHDOG_NOWAYOUT)
* drop pr_info for auto-update case
* restructure code to avoid return
---
drivers/watchdog/Kconfig | 11 +++++++++++
drivers/watchdog/watchdog_dev.c | 19 ++++++++++++++++---
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 52a70ee6014f..9a1bd1e39a75 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -46,6 +46,17 @@ config WATCHDOG_NOWAYOUT
get killed. If you say Y here, the watchdog cannot be stopped once
it has been started.
+config WATCHDOG_HANDLE_BOOT_ENABLED
+ bool "Update boot-enabled watchdog until userspace takes over"
+ default y
+ help
+ The default watchdog behaviour (which you get if you say Y here) is
+ to ping watchdog devices, that were enabled before the driver has
+ been loaded until control is taken over from userspace using the
+ /dev/watchdog file. If you say N here, the kernel will not update
+ the watchdog on its own. Thus if your userspace does not start fast
+ enough your device will reboot.
+
config WATCHDOG_SYSFS
bool "Read different watchdog information through sysfs"
help
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d5d2bbd8f428..4bc7ab60b12c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -80,6 +80,9 @@ static struct watchdog_core_data *old_wd_data;
static struct workqueue_struct *watchdog_wq;
+static bool handle_boot_enabled =
+ IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
+
static inline bool watchdog_need_worker(struct watchdog_device *wdd)
{
/* All variables in milli-seconds */
@@ -956,9 +959,14 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
* and schedule an immediate ping.
*/
if (watchdog_hw_running(wdd)) {
- __module_get(wdd->ops->owner);
- kref_get(&wd_data->kref);
- queue_delayed_work(watchdog_wq, &wd_data->work, 0);
+ if (handle_boot_enabled) {
+ __module_get(wdd->ops->owner);
+ kref_get(&wd_data->kref);
+ queue_delayed_work(watchdog_wq, &wd_data->work, 0);
+ } else {
+ pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
+ wdd->id);
+ }
}
return 0;
@@ -1106,3 +1114,8 @@ void __exit watchdog_dev_exit(void)
class_unregister(&watchdog_class);
destroy_workqueue(watchdog_wq);
}
+
+module_param(handle_boot_enabled, bool, 0444);
+MODULE_PARM_DESC(handle_boot_enabled,
+ "Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
+ __MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2] watchdog: core: add option to avoid early handling of watchdog
2017-05-12 12:05 ` [PATCHv2] " Sebastian Reichel
@ 2017-05-14 14:47 ` Guenter Roeck
2017-05-15 22:08 ` Rasmus Villemoes
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2017-05-14 14:47 UTC (permalink / raw)
To: Sebastian Reichel, Sebastian Reichel, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel
On 05/12/2017 05:05 AM, Sebastian Reichel wrote:
> On some systems its desirable to have watchdog reboot the system
> when it does not come up fast enough. This adds a kernel parameter
> to disable the auto-update of watchdog before userspace takes over
> and a kernel option to set the default. The info messages were
> added to shorten error searching on misconfigured systems.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Minor nitpicks below (which I fixed up in my watchdog-next branch).
Otherwise
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Changes since PATCHv1:
> * fix Kconfig short description (previously copied from
> WATCHDOG_NOWAYOUT)
> * drop pr_info for auto-update case
> * restructure code to avoid return
> ---
> drivers/watchdog/Kconfig | 11 +++++++++++
> drivers/watchdog/watchdog_dev.c | 19 ++++++++++++++++---
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 52a70ee6014f..9a1bd1e39a75 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -46,6 +46,17 @@ config WATCHDOG_NOWAYOUT
> get killed. If you say Y here, the watchdog cannot be stopped once
> it has been started.
>
> +config WATCHDOG_HANDLE_BOOT_ENABLED
> + bool "Update boot-enabled watchdog until userspace takes over"
> + default y
> + help
> + The default watchdog behaviour (which you get if you say Y here) is
> + to ping watchdog devices, that were enabled before the driver has
My sense of English grammar isn't perfect, but I think the ',' isn't asked
for here.
> + been loaded until control is taken over from userspace using the
> + /dev/watchdog file. If you say N here, the kernel will not update
> + the watchdog on its own. Thus if your userspace does not start fast
> + enough your device will reboot.
> +
> config WATCHDOG_SYSFS
> bool "Read different watchdog information through sysfs"
> help
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index d5d2bbd8f428..4bc7ab60b12c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -80,6 +80,9 @@ static struct watchdog_core_data *old_wd_data;
>
> static struct workqueue_struct *watchdog_wq;
>
> +static bool handle_boot_enabled =
> + IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
> +
> static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> {
> /* All variables in milli-seconds */
> @@ -956,9 +959,14 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> * and schedule an immediate ping.
> */
> if (watchdog_hw_running(wdd)) {
> - __module_get(wdd->ops->owner);
> - kref_get(&wd_data->kref);
> - queue_delayed_work(watchdog_wq, &wd_data->work, 0);
> + if (handle_boot_enabled) {
> + __module_get(wdd->ops->owner);
> + kref_get(&wd_data->kref);
> + queue_delayed_work(watchdog_wq, &wd_data->work, 0);
> + } else {
> + pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
> + wdd->id);
Continuation line alignment is off.
> + }
> }
>
> return 0;
> @@ -1106,3 +1114,8 @@ void __exit watchdog_dev_exit(void)
> class_unregister(&watchdog_class);
> destroy_workqueue(watchdog_wq);
> }
> +
> +module_param(handle_boot_enabled, bool, 0444);
> +MODULE_PARM_DESC(handle_boot_enabled,
> + "Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default="
> + __MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2] watchdog: core: add option to avoid early handling of watchdog
2017-05-14 14:47 ` Guenter Roeck
@ 2017-05-15 22:08 ` Rasmus Villemoes
2017-05-20 16:31 ` Sebastian Reichel
0 siblings, 1 reply; 6+ messages in thread
From: Rasmus Villemoes @ 2017-05-15 22:08 UTC (permalink / raw)
To: linux-watchdog, linux-kernel
Cc: Sebastian Reichel, Guenter Roeck, Wim Van Sebroeck
On Sun, May 14 2017, Guenter Roeck wrote:
> On 05/12/2017 05:05 AM, Sebastian Reichel wrote:
>> On some systems its desirable to have watchdog reboot the system
>> when it does not come up fast enough. This adds a kernel parameter
>> to disable the auto-update of watchdog before userspace takes over
>> and a kernel option to set the default. The info messages were
>> added to shorten error searching on misconfigured systems.
>>
>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
>
> Minor nitpicks below (which I fixed up in my watchdog-next branch).
> Otherwise
Guenter, Sebastian, can I pursuade you to take a (second) look at the
patches [1] I sent 4 months ago that implement the same thing, except
that they also give a .config and a boot-cmdline way to define what
"fast enough" means - which is necessary in many cases where it's simply
not realistic to have userspace up-and-running before the dog is hungry.
[1] https://lkml.org/lkml/2017/1/9/408
I'm of course happy to rebase and retest those on top of current master,
but the implementation and semantics should be reviewable as-is.
Rasmus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2] watchdog: core: add option to avoid early handling of watchdog
2017-05-15 22:08 ` Rasmus Villemoes
@ 2017-05-20 16:31 ` Sebastian Reichel
0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2017-05-20 16:31 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: linux-watchdog, linux-kernel, Guenter Roeck, Wim Van Sebroeck
[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]
Hi,
On Tue, May 16, 2017 at 12:08:32AM +0200, Rasmus Villemoes wrote:
> On Sun, May 14 2017, Guenter Roeck wrote:
>
> > On 05/12/2017 05:05 AM, Sebastian Reichel wrote:
> >> On some systems its desirable to have watchdog reboot the system
> >> when it does not come up fast enough. This adds a kernel parameter
> >> to disable the auto-update of watchdog before userspace takes over
> >> and a kernel option to set the default. The info messages were
> >> added to shorten error searching on misconfigured systems.
> >>
> >> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> >
> > Minor nitpicks below (which I fixed up in my watchdog-next branch).
> > Otherwise
>
> Guenter, Sebastian, can I pursuade you to take a (second) look at the
> patches [1] I sent 4 months ago that implement the same thing, except
> that they also give a .config and a boot-cmdline way to define what
> "fast enough" means - which is necessary in many cases where it's simply
> not realistic to have userspace up-and-running before the dog is hungry.
>
> [1] https://lkml.org/lkml/2017/1/9/408
>
> I'm of course happy to rebase and retest those on top of current master,
> but the implementation and semantics should be reviewable as-is.
I wasn't aware of your work. For the hardware I work with at
Collabora my patch is enough, since "fast enough" is defined by
correct watchdog configuration (done by the bootloader).
I guess something like your patch is required for systems, which do
not reach userspace in < 60 seconds (minus some safety margin). This
is not needed by us. OTOH your patches should also work for our use case.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-20 16:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-11 12:52 [PATCH] watchdog: core: add option to avoid early handling of watchdog Sebastian Reichel
2017-05-11 13:47 ` Guenter Roeck
2017-05-12 12:05 ` [PATCHv2] " Sebastian Reichel
2017-05-14 14:47 ` Guenter Roeck
2017-05-15 22:08 ` Rasmus Villemoes
2017-05-20 16:31 ` Sebastian Reichel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).