* [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
@ 2020-07-01 11:03 Woody Lin
2020-07-01 14:10 ` Guenter Roeck
2020-07-07 4:03 ` Guenter Roeck
0 siblings, 2 replies; 8+ messages in thread
From: Woody Lin @ 2020-07-01 11:03 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck; +Cc: linux-watchdog, Woody Lin
Add module parameters 'soft_reboot_cmd' and 'soft_active_on_boot' for
customizing softdog configuration; config reboot command by assigning
soft_reboot_cmd, and set soft_active_on_boot to start up softdog
timer at module initialization stage.
Signed-off-by: Woody Lin <woodylin@google.com>
---
drivers/watchdog/softdog.c | 56 ++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
index 3e4885c1545e..8c8d214b6aa7 100644
--- a/drivers/watchdog/softdog.c
+++ b/drivers/watchdog/softdog.c
@@ -20,11 +20,13 @@
#include <linux/hrtimer.h>
#include <linux/init.h>
#include <linux/kernel.h>
+#include <linux/kthread.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/reboot.h>
#include <linux/types.h>
#include <linux/watchdog.h>
+#include <linux/workqueue.h>
#define TIMER_MARGIN 60 /* Default is 60 seconds */
static unsigned int soft_margin = TIMER_MARGIN; /* in seconds */
@@ -49,11 +51,33 @@ module_param(soft_panic, int, 0);
MODULE_PARM_DESC(soft_panic,
"Softdog action, set to 1 to panic, 0 to reboot (default=0)");
+static char *soft_reboot_cmd;
+module_param(soft_reboot_cmd, charp, 0000);
+MODULE_PARM_DESC(soft_reboot_cmd,
+ "Set reboot command. Emergency reboot takes place if unset");
+
+static bool soft_active_on_boot;
+module_param(soft_active_on_boot, bool, 0000);
+MODULE_PARM_DESC(soft_active_on_boot,
+ "Set to true to active Softdog on boot (default=false)");
+
static struct hrtimer softdog_ticktock;
static struct hrtimer softdog_preticktock;
+static int reboot_kthread_fn(void *data)
+{
+ kernel_restart(soft_reboot_cmd);
+ return -EPERM; /* Should not reach here */
+}
+
+static void reboot_work_fn(struct work_struct *unused)
+{
+ kthread_run(reboot_kthread_fn, NULL, "softdog_reboot");
+}
+
static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
{
+ static bool soft_reboot_fired;
module_put(THIS_MODULE);
if (soft_noboot) {
pr_crit("Triggered - Reboot ignored\n");
@@ -62,6 +86,33 @@ static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
panic("Software Watchdog Timer expired");
} else {
pr_crit("Initiating system reboot\n");
+ if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
+ static DECLARE_WORK(reboot_work, reboot_work_fn);
+ /*
+ * The 'kernel_restart' is a 'might-sleep' operation.
+ * Also, executing it in system-wide workqueues blocks
+ * any driver from using the same workqueue in its
+ * shutdown callback function. Thus, we should execute
+ * the 'kernel_restart' in a standalone kernel thread.
+ * But since starting a kernel thread is also a
+ * 'might-sleep' operation, so the 'reboot_work' is
+ * required as a launcher of the kernel thread.
+ *
+ * After request the reboot, restart the timer to
+ * schedule an 'emergency_restart' reboot after
+ * 'TIMER_MARGIN' seconds. It's because if the softdog
+ * hangs, it might be because of scheduling issues. And
+ * if that is the case, both 'schedule_work' and
+ * 'kernel_restart' may possibly be malfunctional at the
+ * same time.
+ */
+ soft_reboot_fired = true;
+ schedule_work(&reboot_work);
+ hrtimer_add_expires_ns(timer,
+ (u64)TIMER_MARGIN * NSEC_PER_SEC);
+
+ return HRTIMER_RESTART;
+ }
emergency_restart();
pr_crit("Reboot didn't ?????\n");
}
@@ -145,12 +196,17 @@ static int __init softdog_init(void)
softdog_preticktock.function = softdog_pretimeout;
}
+ if (soft_active_on_boot)
+ softdog_ping(&softdog_dev);
+
ret = watchdog_register_device(&softdog_dev);
if (ret)
return ret;
pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
+ pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
+ soft_reboot_cmd, soft_active_on_boot);
return 0;
}
--
2.27.0.212.ge8ba1cc988-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
2020-07-01 11:03 [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot' Woody Lin
@ 2020-07-01 14:10 ` Guenter Roeck
2020-07-01 14:32 ` Woody Lin
2020-07-07 4:03 ` Guenter Roeck
1 sibling, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2020-07-01 14:10 UTC (permalink / raw)
To: Woody Lin, Wim Van Sebroeck; +Cc: linux-watchdog
On 7/1/20 4:03 AM, Woody Lin wrote:
> Add module parameters 'soft_reboot_cmd' and 'soft_active_on_boot' for
> customizing softdog configuration; config reboot command by assigning
> soft_reboot_cmd, and set soft_active_on_boot to start up softdog
> timer at module initialization stage.
>
> Signed-off-by: Woody Lin <woodylin@google.com>
Sigh. Now I'll have to look up old versions and compare to figure out
changes from v2. Why do people always believe that kernel maintainers
have endless amounts of time available ?
[ Sorry, yes, I know, we are supposed to remain friendly all the time.
It is just _so_ frustrating to have to deal with this all the time. ]
Guenter
> ---
> drivers/watchdog/softdog.c | 56 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index 3e4885c1545e..8c8d214b6aa7 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -20,11 +20,13 @@
> #include <linux/hrtimer.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> +#include <linux/kthread.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/reboot.h>
> #include <linux/types.h>
> #include <linux/watchdog.h>
> +#include <linux/workqueue.h>
>
> #define TIMER_MARGIN 60 /* Default is 60 seconds */
> static unsigned int soft_margin = TIMER_MARGIN; /* in seconds */
> @@ -49,11 +51,33 @@ module_param(soft_panic, int, 0);
> MODULE_PARM_DESC(soft_panic,
> "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
>
> +static char *soft_reboot_cmd;
> +module_param(soft_reboot_cmd, charp, 0000);
> +MODULE_PARM_DESC(soft_reboot_cmd,
> + "Set reboot command. Emergency reboot takes place if unset");
> +
> +static bool soft_active_on_boot;
> +module_param(soft_active_on_boot, bool, 0000);
> +MODULE_PARM_DESC(soft_active_on_boot,
> + "Set to true to active Softdog on boot (default=false)");
> +
> static struct hrtimer softdog_ticktock;
> static struct hrtimer softdog_preticktock;
>
> +static int reboot_kthread_fn(void *data)
> +{
> + kernel_restart(soft_reboot_cmd);
> + return -EPERM; /* Should not reach here */
> +}
> +
> +static void reboot_work_fn(struct work_struct *unused)
> +{
> + kthread_run(reboot_kthread_fn, NULL, "softdog_reboot");
> +}
> +
> static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> {
> + static bool soft_reboot_fired;
> module_put(THIS_MODULE);
> if (soft_noboot) {
> pr_crit("Triggered - Reboot ignored\n");
> @@ -62,6 +86,33 @@ static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> panic("Software Watchdog Timer expired");
> } else {
> pr_crit("Initiating system reboot\n");
> + if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
> + static DECLARE_WORK(reboot_work, reboot_work_fn);
> + /*
> + * The 'kernel_restart' is a 'might-sleep' operation.
> + * Also, executing it in system-wide workqueues blocks
> + * any driver from using the same workqueue in its
> + * shutdown callback function. Thus, we should execute
> + * the 'kernel_restart' in a standalone kernel thread.
> + * But since starting a kernel thread is also a
> + * 'might-sleep' operation, so the 'reboot_work' is
> + * required as a launcher of the kernel thread.
> + *
> + * After request the reboot, restart the timer to
> + * schedule an 'emergency_restart' reboot after
> + * 'TIMER_MARGIN' seconds. It's because if the softdog
> + * hangs, it might be because of scheduling issues. And
> + * if that is the case, both 'schedule_work' and
> + * 'kernel_restart' may possibly be malfunctional at the
> + * same time.
> + */
> + soft_reboot_fired = true;
> + schedule_work(&reboot_work);
> + hrtimer_add_expires_ns(timer,
> + (u64)TIMER_MARGIN * NSEC_PER_SEC);
> +
> + return HRTIMER_RESTART;
> + }
> emergency_restart();
> pr_crit("Reboot didn't ?????\n");
> }
> @@ -145,12 +196,17 @@ static int __init softdog_init(void)
> softdog_preticktock.function = softdog_pretimeout;
> }
>
> + if (soft_active_on_boot)
> + softdog_ping(&softdog_dev);
> +
> ret = watchdog_register_device(&softdog_dev);
> if (ret)
> return ret;
>
> pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
> soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
> + pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
> + soft_reboot_cmd, soft_active_on_boot);
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
2020-07-01 14:10 ` Guenter Roeck
@ 2020-07-01 14:32 ` Woody Lin
2020-07-02 15:07 ` Guenter Roeck
0 siblings, 1 reply; 8+ messages in thread
From: Woody Lin @ 2020-07-01 14:32 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog
Attach diff from v2 to v3 here for you to refer (Or any advice about
what next time I can do when uploading the v#N patch for reviewers to
figure out the diff between patch sets more easily? I'd love to follow
if it improves the review process)
Woody
---- 8< ----
diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
index d267af37e066..8c8d214b6aa7 100644
--- a/drivers/watchdog/softdog.c
+++ b/drivers/watchdog/softdog.c
@@ -51,10 +51,10 @@ module_param(soft_panic, int, 0);
MODULE_PARM_DESC(soft_panic,
"Softdog action, set to 1 to panic, 0 to reboot (default=0)");
-static char *soft_reboot_target;
-module_param(soft_reboot_target, charp, 0000);
-MODULE_PARM_DESC(soft_reboot_target,
- "Softdog action, set reboot target (default=emergency)");
+static char *soft_reboot_cmd;
+module_param(soft_reboot_cmd, charp, 0000);
+MODULE_PARM_DESC(soft_reboot_cmd,
+ "Set reboot command. Emergency reboot takes place if unset");
static bool soft_active_on_boot;
module_param(soft_active_on_boot, bool, 0000);
@@ -66,7 +66,7 @@ static struct hrtimer softdog_preticktock;
static int reboot_kthread_fn(void *data)
{
- kernel_restart(soft_reboot_target);
+ kernel_restart(soft_reboot_cmd);
return -EPERM; /* Should not reach here */
}
@@ -86,9 +86,26 @@ static enum hrtimer_restart softdog_fire(struct
hrtimer *timer)
panic("Software Watchdog Timer expired");
} else {
pr_crit("Initiating system reboot\n");
- if (!soft_reboot_fired && soft_reboot_target != NULL) {
+ if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
static DECLARE_WORK(reboot_work, reboot_work_fn);
-
+ /*
+ * The 'kernel_restart' is a 'might-sleep' operation.
+ * Also, executing it in system-wide workqueues blocks
+ * any driver from using the same workqueue in its
+ * shutdown callback function. Thus, we should execute
+ * the 'kernel_restart' in a standalone kernel thread.
+ * But since starting a kernel thread is also a
+ * 'might-sleep' operation, so the 'reboot_work' is
+ * required as a launcher of the kernel thread.
+ *
+ * After request the reboot, restart the timer to
+ * schedule an 'emergency_restart' reboot after
+ * 'TIMER_MARGIN' seconds. It's because if the softdog
+ * hangs, it might be because of scheduling issues. And
+ * if that is the case, both 'schedule_work' and
+ * 'kernel_restart' may possibly be malfunctional at the
+ * same time.
+ */
soft_reboot_fired = true;
schedule_work(&reboot_work);
hrtimer_add_expires_ns(timer,
@@ -179,10 +196,8 @@ static int __init softdog_init(void)
softdog_preticktock.function = softdog_pretimeout;
}
- if (soft_active_on_boot) {
- set_bit(WDOG_HW_RUNNING, &softdog_dev.status);
- set_bit(WDOG_ACTIVE, &softdog_dev.status);
- }
+ if (soft_active_on_boot)
+ softdog_ping(&softdog_dev);
ret = watchdog_register_device(&softdog_dev);
if (ret)
@@ -190,8 +205,8 @@ static int __init softdog_init(void)
pr_info("initialized. soft_noboot=%d soft_margin=%d sec
soft_panic=%d (nowayout=%d)\n",
soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
- pr_info(" soft_reboot_target=%s soft_active_on_boot=%d\n",
- soft_reboot_target, soft_active_on_boot);
+ pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
+ soft_reboot_cmd, soft_active_on_boot);
return 0;
}
---- >8 ----
On Wed, Jul 1, 2020 at 10:10 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/1/20 4:03 AM, Woody Lin wrote:
> > Add module parameters 'soft_reboot_cmd' and 'soft_active_on_boot' for
> > customizing softdog configuration; config reboot command by assigning
> > soft_reboot_cmd, and set soft_active_on_boot to start up softdog
> > timer at module initialization stage.
> >
> > Signed-off-by: Woody Lin <woodylin@google.com>
>
> Sigh. Now I'll have to look up old versions and compare to figure out
> changes from v2. Why do people always believe that kernel maintainers
> have endless amounts of time available ?
>
> [ Sorry, yes, I know, we are supposed to remain friendly all the time.
> It is just _so_ frustrating to have to deal with this all the time. ]
>
> Guenter
>
> > ---
> > drivers/watchdog/softdog.c | 56 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> > index 3e4885c1545e..8c8d214b6aa7 100644
> > --- a/drivers/watchdog/softdog.c
> > +++ b/drivers/watchdog/softdog.c
> > @@ -20,11 +20,13 @@
> > #include <linux/hrtimer.h>
> > #include <linux/init.h>
> > #include <linux/kernel.h>
> > +#include <linux/kthread.h>
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/reboot.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > +#include <linux/workqueue.h>
> >
> > #define TIMER_MARGIN 60 /* Default is 60 seconds */
> > static unsigned int soft_margin = TIMER_MARGIN; /* in seconds */
> > @@ -49,11 +51,33 @@ module_param(soft_panic, int, 0);
> > MODULE_PARM_DESC(soft_panic,
> > "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
> >
> > +static char *soft_reboot_cmd;
> > +module_param(soft_reboot_cmd, charp, 0000);
> > +MODULE_PARM_DESC(soft_reboot_cmd,
> > + "Set reboot command. Emergency reboot takes place if unset");
> > +
> > +static bool soft_active_on_boot;
> > +module_param(soft_active_on_boot, bool, 0000);
> > +MODULE_PARM_DESC(soft_active_on_boot,
> > + "Set to true to active Softdog on boot (default=false)");
> > +
> > static struct hrtimer softdog_ticktock;
> > static struct hrtimer softdog_preticktock;
> >
> > +static int reboot_kthread_fn(void *data)
> > +{
> > + kernel_restart(soft_reboot_cmd);
> > + return -EPERM; /* Should not reach here */
> > +}
> > +
> > +static void reboot_work_fn(struct work_struct *unused)
> > +{
> > + kthread_run(reboot_kthread_fn, NULL, "softdog_reboot");
> > +}
> > +
> > static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> > {
> > + static bool soft_reboot_fired;
> > module_put(THIS_MODULE);
> > if (soft_noboot) {
> > pr_crit("Triggered - Reboot ignored\n");
> > @@ -62,6 +86,33 @@ static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> > panic("Software Watchdog Timer expired");
> > } else {
> > pr_crit("Initiating system reboot\n");
> > + if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
> > + static DECLARE_WORK(reboot_work, reboot_work_fn);
> > + /*
> > + * The 'kernel_restart' is a 'might-sleep' operation.
> > + * Also, executing it in system-wide workqueues blocks
> > + * any driver from using the same workqueue in its
> > + * shutdown callback function. Thus, we should execute
> > + * the 'kernel_restart' in a standalone kernel thread.
> > + * But since starting a kernel thread is also a
> > + * 'might-sleep' operation, so the 'reboot_work' is
> > + * required as a launcher of the kernel thread.
> > + *
> > + * After request the reboot, restart the timer to
> > + * schedule an 'emergency_restart' reboot after
> > + * 'TIMER_MARGIN' seconds. It's because if the softdog
> > + * hangs, it might be because of scheduling issues. And
> > + * if that is the case, both 'schedule_work' and
> > + * 'kernel_restart' may possibly be malfunctional at the
> > + * same time.
> > + */
> > + soft_reboot_fired = true;
> > + schedule_work(&reboot_work);
> > + hrtimer_add_expires_ns(timer,
> > + (u64)TIMER_MARGIN * NSEC_PER_SEC);
> > +
> > + return HRTIMER_RESTART;
> > + }
> > emergency_restart();
> > pr_crit("Reboot didn't ?????\n");
> > }
> > @@ -145,12 +196,17 @@ static int __init softdog_init(void)
> > softdog_preticktock.function = softdog_pretimeout;
> > }
> >
> > + if (soft_active_on_boot)
> > + softdog_ping(&softdog_dev);
> > +
> > ret = watchdog_register_device(&softdog_dev);
> > if (ret)
> > return ret;
> >
> > pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
> > soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
> > + pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
> > + soft_reboot_cmd, soft_active_on_boot);
> >
> > return 0;
> > }
> >
>
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
2020-07-01 14:32 ` Woody Lin
@ 2020-07-02 15:07 ` Guenter Roeck
0 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2020-07-02 15:07 UTC (permalink / raw)
To: Woody Lin; +Cc: Wim Van Sebroeck, linux-watchdog
On Wed, Jul 01, 2020 at 10:32:16PM +0800, Woody Lin wrote:
> Attach diff from v2 to v3 here for you to refer (Or any advice about
> what next time I can do when uploading the v#N patch for reviewers to
> figure out the diff between patch sets more easily? I'd love to follow
> if it improves the review process)
>
I would have expected something like
---
v3: Renamed soft_reboot_target to soft_reboot_cmd
Explain reason for introducing a worker
<other changes made in v3>
v2: <changes made in v2>
Guenter
> Woody
>
> ---- 8< ----
>
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index d267af37e066..8c8d214b6aa7 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -51,10 +51,10 @@ module_param(soft_panic, int, 0);
> MODULE_PARM_DESC(soft_panic,
> "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
>
> -static char *soft_reboot_target;
> -module_param(soft_reboot_target, charp, 0000);
> -MODULE_PARM_DESC(soft_reboot_target,
> - "Softdog action, set reboot target (default=emergency)");
> +static char *soft_reboot_cmd;
> +module_param(soft_reboot_cmd, charp, 0000);
> +MODULE_PARM_DESC(soft_reboot_cmd,
> + "Set reboot command. Emergency reboot takes place if unset");
>
> static bool soft_active_on_boot;
> module_param(soft_active_on_boot, bool, 0000);
> @@ -66,7 +66,7 @@ static struct hrtimer softdog_preticktock;
>
> static int reboot_kthread_fn(void *data)
> {
> - kernel_restart(soft_reboot_target);
> + kernel_restart(soft_reboot_cmd);
> return -EPERM; /* Should not reach here */
> }
>
> @@ -86,9 +86,26 @@ static enum hrtimer_restart softdog_fire(struct
> hrtimer *timer)
> panic("Software Watchdog Timer expired");
> } else {
> pr_crit("Initiating system reboot\n");
> - if (!soft_reboot_fired && soft_reboot_target != NULL) {
> + if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
> static DECLARE_WORK(reboot_work, reboot_work_fn);
> -
> + /*
> + * The 'kernel_restart' is a 'might-sleep' operation.
> + * Also, executing it in system-wide workqueues blocks
> + * any driver from using the same workqueue in its
> + * shutdown callback function. Thus, we should execute
> + * the 'kernel_restart' in a standalone kernel thread.
> + * But since starting a kernel thread is also a
> + * 'might-sleep' operation, so the 'reboot_work' is
> + * required as a launcher of the kernel thread.
> + *
> + * After request the reboot, restart the timer to
> + * schedule an 'emergency_restart' reboot after
> + * 'TIMER_MARGIN' seconds. It's because if the softdog
> + * hangs, it might be because of scheduling issues. And
> + * if that is the case, both 'schedule_work' and
> + * 'kernel_restart' may possibly be malfunctional at the
> + * same time.
> + */
> soft_reboot_fired = true;
> schedule_work(&reboot_work);
> hrtimer_add_expires_ns(timer,
> @@ -179,10 +196,8 @@ static int __init softdog_init(void)
> softdog_preticktock.function = softdog_pretimeout;
> }
>
> - if (soft_active_on_boot) {
> - set_bit(WDOG_HW_RUNNING, &softdog_dev.status);
> - set_bit(WDOG_ACTIVE, &softdog_dev.status);
> - }
> + if (soft_active_on_boot)
> + softdog_ping(&softdog_dev);
>
> ret = watchdog_register_device(&softdog_dev);
> if (ret)
> @@ -190,8 +205,8 @@ static int __init softdog_init(void)
>
> pr_info("initialized. soft_noboot=%d soft_margin=%d sec
> soft_panic=%d (nowayout=%d)\n",
> soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
> - pr_info(" soft_reboot_target=%s soft_active_on_boot=%d\n",
> - soft_reboot_target, soft_active_on_boot);
> + pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
> + soft_reboot_cmd, soft_active_on_boot);
>
> return 0;
> }
> ---- >8 ----
>
> On Wed, Jul 1, 2020 at 10:10 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 7/1/20 4:03 AM, Woody Lin wrote:
> > > Add module parameters 'soft_reboot_cmd' and 'soft_active_on_boot' for
> > > customizing softdog configuration; config reboot command by assigning
> > > soft_reboot_cmd, and set soft_active_on_boot to start up softdog
> > > timer at module initialization stage.
> > >
> > > Signed-off-by: Woody Lin <woodylin@google.com>
> >
> > Sigh. Now I'll have to look up old versions and compare to figure out
> > changes from v2. Why do people always believe that kernel maintainers
> > have endless amounts of time available ?
> >
> > [ Sorry, yes, I know, we are supposed to remain friendly all the time.
> > It is just _so_ frustrating to have to deal with this all the time. ]
> >
> > Guenter
> >
> > > ---
> > > drivers/watchdog/softdog.c | 56 ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> > > index 3e4885c1545e..8c8d214b6aa7 100644
> > > --- a/drivers/watchdog/softdog.c
> > > +++ b/drivers/watchdog/softdog.c
> > > @@ -20,11 +20,13 @@
> > > #include <linux/hrtimer.h>
> > > #include <linux/init.h>
> > > #include <linux/kernel.h>
> > > +#include <linux/kthread.h>
> > > #include <linux/module.h>
> > > #include <linux/moduleparam.h>
> > > #include <linux/reboot.h>
> > > #include <linux/types.h>
> > > #include <linux/watchdog.h>
> > > +#include <linux/workqueue.h>
> > >
> > > #define TIMER_MARGIN 60 /* Default is 60 seconds */
> > > static unsigned int soft_margin = TIMER_MARGIN; /* in seconds */
> > > @@ -49,11 +51,33 @@ module_param(soft_panic, int, 0);
> > > MODULE_PARM_DESC(soft_panic,
> > > "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
> > >
> > > +static char *soft_reboot_cmd;
> > > +module_param(soft_reboot_cmd, charp, 0000);
> > > +MODULE_PARM_DESC(soft_reboot_cmd,
> > > + "Set reboot command. Emergency reboot takes place if unset");
> > > +
> > > +static bool soft_active_on_boot;
> > > +module_param(soft_active_on_boot, bool, 0000);
> > > +MODULE_PARM_DESC(soft_active_on_boot,
> > > + "Set to true to active Softdog on boot (default=false)");
> > > +
> > > static struct hrtimer softdog_ticktock;
> > > static struct hrtimer softdog_preticktock;
> > >
> > > +static int reboot_kthread_fn(void *data)
> > > +{
> > > + kernel_restart(soft_reboot_cmd);
> > > + return -EPERM; /* Should not reach here */
> > > +}
> > > +
> > > +static void reboot_work_fn(struct work_struct *unused)
> > > +{
> > > + kthread_run(reboot_kthread_fn, NULL, "softdog_reboot");
> > > +}
> > > +
> > > static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> > > {
> > > + static bool soft_reboot_fired;
> > > module_put(THIS_MODULE);
> > > if (soft_noboot) {
> > > pr_crit("Triggered - Reboot ignored\n");
> > > @@ -62,6 +86,33 @@ static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> > > panic("Software Watchdog Timer expired");
> > > } else {
> > > pr_crit("Initiating system reboot\n");
> > > + if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
> > > + static DECLARE_WORK(reboot_work, reboot_work_fn);
> > > + /*
> > > + * The 'kernel_restart' is a 'might-sleep' operation.
> > > + * Also, executing it in system-wide workqueues blocks
> > > + * any driver from using the same workqueue in its
> > > + * shutdown callback function. Thus, we should execute
> > > + * the 'kernel_restart' in a standalone kernel thread.
> > > + * But since starting a kernel thread is also a
> > > + * 'might-sleep' operation, so the 'reboot_work' is
> > > + * required as a launcher of the kernel thread.
> > > + *
> > > + * After request the reboot, restart the timer to
> > > + * schedule an 'emergency_restart' reboot after
> > > + * 'TIMER_MARGIN' seconds. It's because if the softdog
> > > + * hangs, it might be because of scheduling issues. And
> > > + * if that is the case, both 'schedule_work' and
> > > + * 'kernel_restart' may possibly be malfunctional at the
> > > + * same time.
> > > + */
> > > + soft_reboot_fired = true;
> > > + schedule_work(&reboot_work);
> > > + hrtimer_add_expires_ns(timer,
> > > + (u64)TIMER_MARGIN * NSEC_PER_SEC);
> > > +
> > > + return HRTIMER_RESTART;
> > > + }
> > > emergency_restart();
> > > pr_crit("Reboot didn't ?????\n");
> > > }
> > > @@ -145,12 +196,17 @@ static int __init softdog_init(void)
> > > softdog_preticktock.function = softdog_pretimeout;
> > > }
> > >
> > > + if (soft_active_on_boot)
> > > + softdog_ping(&softdog_dev);
> > > +
> > > ret = watchdog_register_device(&softdog_dev);
> > > if (ret)
> > > return ret;
> > >
> > > pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
> > > soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
> > > + pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
> > > + soft_reboot_cmd, soft_active_on_boot);
> > >
> > > return 0;
> > > }
> > >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
2020-07-01 11:03 [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot' Woody Lin
2020-07-01 14:10 ` Guenter Roeck
@ 2020-07-07 4:03 ` Guenter Roeck
2020-07-07 8:46 ` Woody Lin
1 sibling, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2020-07-07 4:03 UTC (permalink / raw)
To: Woody Lin; +Cc: Wim Van Sebroeck, linux-watchdog
On Wed, Jul 01, 2020 at 07:03:40PM +0800, Woody Lin wrote:
> Add module parameters 'soft_reboot_cmd' and 'soft_active_on_boot' for
> customizing softdog configuration; config reboot command by assigning
> soft_reboot_cmd, and set soft_active_on_boot to start up softdog
> timer at module initialization stage.
>
> Signed-off-by: Woody Lin <woodylin@google.com>
> ---
> drivers/watchdog/softdog.c | 56 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index 3e4885c1545e..8c8d214b6aa7 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -20,11 +20,13 @@
> #include <linux/hrtimer.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> +#include <linux/kthread.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/reboot.h>
> #include <linux/types.h>
> #include <linux/watchdog.h>
> +#include <linux/workqueue.h>
>
> #define TIMER_MARGIN 60 /* Default is 60 seconds */
> static unsigned int soft_margin = TIMER_MARGIN; /* in seconds */
> @@ -49,11 +51,33 @@ module_param(soft_panic, int, 0);
> MODULE_PARM_DESC(soft_panic,
> "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
>
> +static char *soft_reboot_cmd;
> +module_param(soft_reboot_cmd, charp, 0000);
> +MODULE_PARM_DESC(soft_reboot_cmd,
> + "Set reboot command. Emergency reboot takes place if unset");
> +
> +static bool soft_active_on_boot;
> +module_param(soft_active_on_boot, bool, 0000);
> +MODULE_PARM_DESC(soft_active_on_boot,
> + "Set to true to active Softdog on boot (default=false)");
> +
> static struct hrtimer softdog_ticktock;
> static struct hrtimer softdog_preticktock;
>
> +static int reboot_kthread_fn(void *data)
> +{
> + kernel_restart(soft_reboot_cmd);
> + return -EPERM; /* Should not reach here */
> +}
> +
> +static void reboot_work_fn(struct work_struct *unused)
> +{
> + kthread_run(reboot_kthread_fn, NULL, "softdog_reboot");
> +}
> +
> static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> {
> + static bool soft_reboot_fired;
Per coding style there should be an empty line here.
> module_put(THIS_MODULE);
> if (soft_noboot) {
> pr_crit("Triggered - Reboot ignored\n");
> @@ -62,6 +86,33 @@ static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> panic("Software Watchdog Timer expired");
> } else {
> pr_crit("Initiating system reboot\n");
> + if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
> + static DECLARE_WORK(reboot_work, reboot_work_fn);
> + /*
> + * The 'kernel_restart' is a 'might-sleep' operation.
> + * Also, executing it in system-wide workqueues blocks
> + * any driver from using the same workqueue in its
> + * shutdown callback function. Thus, we should execute
> + * the 'kernel_restart' in a standalone kernel thread.
> + * But since starting a kernel thread is also a
> + * 'might-sleep' operation, so the 'reboot_work' is
> + * required as a launcher of the kernel thread.
> + *
> + * After request the reboot, restart the timer to
> + * schedule an 'emergency_restart' reboot after
> + * 'TIMER_MARGIN' seconds. It's because if the softdog
> + * hangs, it might be because of scheduling issues. And
> + * if that is the case, both 'schedule_work' and
> + * 'kernel_restart' may possibly be malfunctional at the
> + * same time.
> + */
> + soft_reboot_fired = true;
> + schedule_work(&reboot_work);
> + hrtimer_add_expires_ns(timer,
> + (u64)TIMER_MARGIN * NSEC_PER_SEC);
> +
> + return HRTIMER_RESTART;
> + }
> emergency_restart();
> pr_crit("Reboot didn't ?????\n");
> }
> @@ -145,12 +196,17 @@ static int __init softdog_init(void)
> softdog_preticktock.function = softdog_pretimeout;
> }
>
> + if (soft_active_on_boot)
> + softdog_ping(&softdog_dev);
> +
> ret = watchdog_register_device(&softdog_dev);
> if (ret)
> return ret;
>
> pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
> soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
> + pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
> + soft_reboot_cmd, soft_active_on_boot);
soft_reboot_cmd can be NULL, which makes the output a bit awkward.
>
> return 0;
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
2020-07-07 4:03 ` Guenter Roeck
@ 2020-07-07 8:46 ` Woody Lin
2020-07-07 11:47 ` Guenter Roeck
0 siblings, 1 reply; 8+ messages in thread
From: Woody Lin @ 2020-07-07 8:46 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog
On Tue, Jul 7, 2020 at 12:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Jul 01, 2020 at 07:03:40PM +0800, Woody Lin wrote:
> > Add module parameters 'soft_reboot_cmd' and 'soft_active_on_boot' for
> > customizing softdog configuration; config reboot command by assigning
> > soft_reboot_cmd, and set soft_active_on_boot to start up softdog
> > timer at module initialization stage.
> >
> > Signed-off-by: Woody Lin <woodylin@google.com>
> > ---
> > drivers/watchdog/softdog.c | 56 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> > index 3e4885c1545e..8c8d214b6aa7 100644
> > --- a/drivers/watchdog/softdog.c
> > +++ b/drivers/watchdog/softdog.c
> > @@ -20,11 +20,13 @@
> > #include <linux/hrtimer.h>
> > #include <linux/init.h>
> > #include <linux/kernel.h>
> > +#include <linux/kthread.h>
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/reboot.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > +#include <linux/workqueue.h>
> >
> > #define TIMER_MARGIN 60 /* Default is 60 seconds */
> > static unsigned int soft_margin = TIMER_MARGIN; /* in seconds */
> > @@ -49,11 +51,33 @@ module_param(soft_panic, int, 0);
> > MODULE_PARM_DESC(soft_panic,
> > "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
> >
> > +static char *soft_reboot_cmd;
> > +module_param(soft_reboot_cmd, charp, 0000);
> > +MODULE_PARM_DESC(soft_reboot_cmd,
> > + "Set reboot command. Emergency reboot takes place if unset");
> > +
> > +static bool soft_active_on_boot;
> > +module_param(soft_active_on_boot, bool, 0000);
> > +MODULE_PARM_DESC(soft_active_on_boot,
> > + "Set to true to active Softdog on boot (default=false)");
> > +
> > static struct hrtimer softdog_ticktock;
> > static struct hrtimer softdog_preticktock;
> >
> > +static int reboot_kthread_fn(void *data)
> > +{
> > + kernel_restart(soft_reboot_cmd);
> > + return -EPERM; /* Should not reach here */
> > +}
> > +
> > +static void reboot_work_fn(struct work_struct *unused)
> > +{
> > + kthread_run(reboot_kthread_fn, NULL, "softdog_reboot");
> > +}
> > +
> > static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> > {
> > + static bool soft_reboot_fired;
>
> Per coding style there should be an empty line here.
Ack.
>
> > module_put(THIS_MODULE);
> > if (soft_noboot) {
> > pr_crit("Triggered - Reboot ignored\n");
> > @@ -62,6 +86,33 @@ static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
> > panic("Software Watchdog Timer expired");
> > } else {
> > pr_crit("Initiating system reboot\n");
> > + if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
> > + static DECLARE_WORK(reboot_work, reboot_work_fn);
> > + /*
> > + * The 'kernel_restart' is a 'might-sleep' operation.
> > + * Also, executing it in system-wide workqueues blocks
> > + * any driver from using the same workqueue in its
> > + * shutdown callback function. Thus, we should execute
> > + * the 'kernel_restart' in a standalone kernel thread.
> > + * But since starting a kernel thread is also a
> > + * 'might-sleep' operation, so the 'reboot_work' is
> > + * required as a launcher of the kernel thread.
> > + *
> > + * After request the reboot, restart the timer to
> > + * schedule an 'emergency_restart' reboot after
> > + * 'TIMER_MARGIN' seconds. It's because if the softdog
> > + * hangs, it might be because of scheduling issues. And
> > + * if that is the case, both 'schedule_work' and
> > + * 'kernel_restart' may possibly be malfunctional at the
> > + * same time.
> > + */
> > + soft_reboot_fired = true;
> > + schedule_work(&reboot_work);
> > + hrtimer_add_expires_ns(timer,
> > + (u64)TIMER_MARGIN * NSEC_PER_SEC);
> > +
> > + return HRTIMER_RESTART;
> > + }
> > emergency_restart();
> > pr_crit("Reboot didn't ?????\n");
> > }
> > @@ -145,12 +196,17 @@ static int __init softdog_init(void)
> > softdog_preticktock.function = softdog_pretimeout;
> > }
> >
> > + if (soft_active_on_boot)
> > + softdog_ping(&softdog_dev);
> > +
> > ret = watchdog_register_device(&softdog_dev);
> > if (ret)
> > return ret;
> >
> > pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
> > soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
> > + pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
> > + soft_reboot_cmd, soft_active_on_boot);
>
> soft_reboot_cmd can be NULL, which makes the output a bit awkward.
Then how about change it to something like this:
"soft_reboot_cmd=%s", soft_reboot_cmd ?: "<null> (emergency reboot)"
Then we will see "soft_reboot_cmd=<null> (emergency reboot)" when it's NULL.
>
> >
> > return 0;
> > }
Woody
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
2020-07-07 8:46 ` Woody Lin
@ 2020-07-07 11:47 ` Guenter Roeck
2020-07-08 7:17 ` Woody Lin
0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2020-07-07 11:47 UTC (permalink / raw)
To: Woody Lin; +Cc: Wim Van Sebroeck, linux-watchdog
On 7/7/20 1:46 AM, Woody Lin wrote:
> On Tue, Jul 7, 2020 at 12:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Wed, Jul 01, 2020 at 07:03:40PM +0800, Woody Lin wrote:
>>> Add module parameters 'soft_reboot_cmd' and 'soft_active_on_boot' for
>>> customizing softdog configuration; config reboot command by assigning
>>> soft_reboot_cmd, and set soft_active_on_boot to start up softdog
>>> timer at module initialization stage.
>>>
>>> Signed-off-by: Woody Lin <woodylin@google.com>
>>> ---
>>> drivers/watchdog/softdog.c | 56 ++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
>>> index 3e4885c1545e..8c8d214b6aa7 100644
>>> --- a/drivers/watchdog/softdog.c
>>> +++ b/drivers/watchdog/softdog.c
>>> @@ -20,11 +20,13 @@
>>> #include <linux/hrtimer.h>
>>> #include <linux/init.h>
>>> #include <linux/kernel.h>
>>> +#include <linux/kthread.h>
>>> #include <linux/module.h>
>>> #include <linux/moduleparam.h>
>>> #include <linux/reboot.h>
>>> #include <linux/types.h>
>>> #include <linux/watchdog.h>
>>> +#include <linux/workqueue.h>
>>>
>>> #define TIMER_MARGIN 60 /* Default is 60 seconds */
>>> static unsigned int soft_margin = TIMER_MARGIN; /* in seconds */
>>> @@ -49,11 +51,33 @@ module_param(soft_panic, int, 0);
>>> MODULE_PARM_DESC(soft_panic,
>>> "Softdog action, set to 1 to panic, 0 to reboot (default=0)");
>>>
>>> +static char *soft_reboot_cmd;
>>> +module_param(soft_reboot_cmd, charp, 0000);
>>> +MODULE_PARM_DESC(soft_reboot_cmd,
>>> + "Set reboot command. Emergency reboot takes place if unset");
>>> +
>>> +static bool soft_active_on_boot;
>>> +module_param(soft_active_on_boot, bool, 0000);
>>> +MODULE_PARM_DESC(soft_active_on_boot,
>>> + "Set to true to active Softdog on boot (default=false)");
>>> +
>>> static struct hrtimer softdog_ticktock;
>>> static struct hrtimer softdog_preticktock;
>>>
>>> +static int reboot_kthread_fn(void *data)
>>> +{
>>> + kernel_restart(soft_reboot_cmd);
>>> + return -EPERM; /* Should not reach here */
>>> +}
>>> +
>>> +static void reboot_work_fn(struct work_struct *unused)
>>> +{
>>> + kthread_run(reboot_kthread_fn, NULL, "softdog_reboot");
>>> +}
>>> +
>>> static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
>>> {
>>> + static bool soft_reboot_fired;
>>
>> Per coding style there should be an empty line here.
>
> Ack.
>
>>
>>> module_put(THIS_MODULE);
>>> if (soft_noboot) {
>>> pr_crit("Triggered - Reboot ignored\n");
>>> @@ -62,6 +86,33 @@ static enum hrtimer_restart softdog_fire(struct hrtimer *timer)
>>> panic("Software Watchdog Timer expired");
>>> } else {
>>> pr_crit("Initiating system reboot\n");
>>> + if (!soft_reboot_fired && soft_reboot_cmd != NULL) {
>>> + static DECLARE_WORK(reboot_work, reboot_work_fn);
>>> + /*
>>> + * The 'kernel_restart' is a 'might-sleep' operation.
>>> + * Also, executing it in system-wide workqueues blocks
>>> + * any driver from using the same workqueue in its
>>> + * shutdown callback function. Thus, we should execute
>>> + * the 'kernel_restart' in a standalone kernel thread.
>>> + * But since starting a kernel thread is also a
>>> + * 'might-sleep' operation, so the 'reboot_work' is
>>> + * required as a launcher of the kernel thread.
>>> + *
>>> + * After request the reboot, restart the timer to
>>> + * schedule an 'emergency_restart' reboot after
>>> + * 'TIMER_MARGIN' seconds. It's because if the softdog
>>> + * hangs, it might be because of scheduling issues. And
>>> + * if that is the case, both 'schedule_work' and
>>> + * 'kernel_restart' may possibly be malfunctional at the
>>> + * same time.
>>> + */
>>> + soft_reboot_fired = true;
>>> + schedule_work(&reboot_work);
>>> + hrtimer_add_expires_ns(timer,
>>> + (u64)TIMER_MARGIN * NSEC_PER_SEC);
>>> +
>>> + return HRTIMER_RESTART;
>>> + }
>>> emergency_restart();
>>> pr_crit("Reboot didn't ?????\n");
>>> }
>>> @@ -145,12 +196,17 @@ static int __init softdog_init(void)
>>> softdog_preticktock.function = softdog_pretimeout;
>>> }
>>>
>>> + if (soft_active_on_boot)
>>> + softdog_ping(&softdog_dev);
>>> +
>>> ret = watchdog_register_device(&softdog_dev);
>>> if (ret)
>>> return ret;
>>>
>>> pr_info("initialized. soft_noboot=%d soft_margin=%d sec soft_panic=%d (nowayout=%d)\n",
>>> soft_noboot, softdog_dev.timeout, soft_panic, nowayout);
>>> + pr_info(" soft_reboot_cmd=%s soft_active_on_boot=%d\n",
>>> + soft_reboot_cmd, soft_active_on_boot);
>>
>> soft_reboot_cmd can be NULL, which makes the output a bit awkward.
>
> Then how about change it to something like this:
> "soft_reboot_cmd=%s", soft_reboot_cmd ?: "<null> (emergency reboot)"
> Then we will see "soft_reboot_cmd=<null> (emergency reboot)" when it's NULL.
I'd rather see something like "<not set>". "<null>" looks like an error.
Also, it isn't correct to assume emergency reboot; that is only correct
if neither soft_noboot nor soft_panic is set.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot'
2020-07-07 11:47 ` Guenter Roeck
@ 2020-07-08 7:17 ` Woody Lin
0 siblings, 0 replies; 8+ messages in thread
From: Woody Lin @ 2020-07-08 7:17 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog
On Tue, Jul 7, 2020 at 7:47 PM Guenter Roeck <linux@roeck-us.net> wrote:
> I'd rather see something like "<not set>". "<null>" looks like an error.
> Also, it isn't correct to assume emergency reboot; that is only correct
> if neither soft_noboot nor soft_panic is set.
"<not set>" sounds good to me, and thanks for correcting my usage
description on "(emergency reboot)".
I'm uploading the next version.
Woody
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-08 7:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-01 11:03 [PATCH v3] softdog: Add options 'soft_reboot_cmd' and 'soft_active_on_boot' Woody Lin
2020-07-01 14:10 ` Guenter Roeck
2020-07-01 14:32 ` Woody Lin
2020-07-02 15:07 ` Guenter Roeck
2020-07-07 4:03 ` Guenter Roeck
2020-07-07 8:46 ` Woody Lin
2020-07-07 11:47 ` Guenter Roeck
2020-07-08 7:17 ` Woody Lin
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).