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@xxxxxxxxxx>
> ---
> 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;
> }
>
