| Message ID | 20200612155942.246816-1-woodylin@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] softdog: Add options 'soft_reboot_target' and 'soft_active_on_boot' | expand |
Hi Woody, On 6/12/20 8:59 AM, Woody Lin wrote: > Add module parameters 'soft_reboot_target' and 'soft_active_on_boot' for > customizing softdog configuration; config reboot target by assigning > soft_reboot_target, and set soft_active_on_boot to start up softdog > timer at module initialization stage. > The code should include an explanation for the need for the double indirection (ie why schedule another worker instead of calling kthread_run directly). Also, there should be a comment explaining how it works, especially if kthread_run() fails to run a thread, and why kthread_run() is needed in the first place. > Signed-off-by: Woody Lin <woodylin@google.com> > --- > drivers/watchdog/softdog.c | 41 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c > index 3e4885c1545e..d267af37e066 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_target; > +module_param(soft_reboot_target, charp, 0000); > +MODULE_PARM_DESC(soft_reboot_target, > + "Softdog action, set reboot target (default=emergency)"); > + This isn't really a "reboot target". It is just a string passed to kernel_restart(). For the most part that string doesn't really do anything useful. What is it supposed to accomplish in your case, other than causing a reboot with the configured reboot_mode (which is a global variable) ? If the underlying idea is to cause an orderly restart following reboot_mode, a boolean would accomplish pretty much the same. Am I missing something ? > +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_target); > + 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,16 @@ 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) { > + static DECLARE_WORK(reboot_work, reboot_work_fn); > + > + 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 +179,19 @@ 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); > + } Strictly speaking you should call the ping function here to start the timer. Otherwise the watchdog won't really start if watchdog_register_device() fails. Guenter > + > 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_target=%s soft_active_on_boot=%d\n", > + soft_reboot_target, soft_active_on_boot); > > return 0; > } >
Hi Guenter, > The code should include an explanation for the need for the double > indirection (ie why schedule another worker instead of calling > kthread_run directly). Also, there should be a comment explaining > how it works, especially if kthread_run() fails to run a thread, > and why kthread_run() is needed in the first place. No problem, I am adding this to the next version. > This isn't really a "reboot target". It is just a string passed > to kernel_restart(). For the most part that string doesn't really > do anything useful. What is it supposed to accomplish in your case, > other than causing a reboot with the configured reboot_mode > (which is a global variable) ? > > If the underlying idea is to cause an orderly restart following > reboot_mode, a boolean would accomplish pretty much the same. > > Am I missing something ? This argument is next sent to registered reboot notifiers and machine_restart, according to implementation of 'kernel_restart'. So for platforms which implemented different handlers for each 'cmd', the timeout can be handled with desired behavior. For example, some platforms might want to reboot to rescue mode when timeout. void kernel_restart(char *cmd) kernel_restart_prepare(cmd); -> blocking_notifier_call_chain(&reboot_notifier_list, SYS_RESTART, cmd); ... machine_restart(cmd); Btw, according to the signature of kernel_restart, I should name this argument 'soft_reboot_cmd', instead of '*_target'. Sorry for causing the confusion. > > + if (soft_active_on_boot) { > > + set_bit(WDOG_HW_RUNNING, &softdog_dev.status); > > + set_bit(WDOG_ACTIVE, &softdog_dev.status); > > + } > > Strictly speaking you should call the ping function here to start the timer. > Otherwise the watchdog won't really start if watchdog_register_device() > fails. Thanks for catching this, I will revise this in the next version. Woody
On Fri, Jun 19, 2020 at 06:27:04PM +0800, Woody Lin wrote: > Hi Guenter, > > > The code should include an explanation for the need for the double > > indirection (ie why schedule another worker instead of calling > > kthread_run directly). Also, there should be a comment explaining > > how it works, especially if kthread_run() fails to run a thread, > > and why kthread_run() is needed in the first place. > > No problem, I am adding this to the next version. > > > This isn't really a "reboot target". It is just a string passed > > to kernel_restart(). For the most part that string doesn't really > > do anything useful. What is it supposed to accomplish in your case, > > other than causing a reboot with the configured reboot_mode > > (which is a global variable) ? > > > > If the underlying idea is to cause an orderly restart following > > reboot_mode, a boolean would accomplish pretty much the same. > > > > Am I missing something ? > > This argument is next sent to registered reboot notifiers and machine_restart, > according to implementation of 'kernel_restart'. So for platforms which > implemented different handlers for each 'cmd', the timeout can be handled with > desired behavior. For example, some platforms might want to reboot to rescue > mode when timeout. > > void kernel_restart(char *cmd) > kernel_restart_prepare(cmd); > -> blocking_notifier_call_chain(&reboot_notifier_list, SYS_RESTART, cmd); > ... > machine_restart(cmd); > > Btw, according to the signature of kernel_restart, I should name this argument > 'soft_reboot_cmd', instead of '*_target'. Sorry for causing the confusion. > Yes, that would make more sense. Thanks, Guenter > > > + if (soft_active_on_boot) { > > > + set_bit(WDOG_HW_RUNNING, &softdog_dev.status); > > > + set_bit(WDOG_ACTIVE, &softdog_dev.status); > > > + } > > > > Strictly speaking you should call the ping function here to start the timer. > > Otherwise the watchdog won't really start if watchdog_register_device() > > fails. > > Thanks for catching this, I will revise this in the next version. > > Woody
diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c index 3e4885c1545e..d267af37e066 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_target; +module_param(soft_reboot_target, charp, 0000); +MODULE_PARM_DESC(soft_reboot_target, + "Softdog action, set reboot target (default=emergency)"); + +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_target); + 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,16 @@ 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) { + static DECLARE_WORK(reboot_work, reboot_work_fn); + + 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 +179,19 @@ 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); + } + 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_target=%s soft_active_on_boot=%d\n", + soft_reboot_target, soft_active_on_boot); return 0; }
Add module parameters 'soft_reboot_target' and 'soft_active_on_boot' for customizing softdog configuration; config reboot target by assigning soft_reboot_target, 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 | 41 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)