Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640418 > unrolled thread

[PATCHv2] watchdog: core: add option to avoid early handling of watchdog

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCHv2] watchdog: core: add option to avoid early handling of watchdog Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-05-12 14:10 +0200
    Re: [PATCHv2] watchdog: core: add option to avoid early handling of  watchdog Guenter Roeck <linux@roeck-us.net> - 2017-05-14 16:50 +0200
      Re: [PATCHv2] watchdog: core: add option to avoid early handling of watchdog Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2017-05-16 00:10 +0200
        Re: [PATCHv2] watchdog: core: add option to avoid early handling of  watchdog Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-05-20 18:40 +0200

#1640418 — [PATCHv2] watchdog: core: add option to avoid early handling of watchdog

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

[toc] | [next] | [standalone]

#1641103 — Re: [PATCHv2] watchdog: core: add option to avoid early handling of watchdog

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)) ")");
>

[toc] | [prev] | [next] | [standalone]

#1642092

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

[toc] | [prev] | [next] | [standalone]

#1646150 — Re: [PATCHv2] watchdog: core: add option to avoid early handling of watchdog

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web