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
