[PATCH] watchdog: force to stop the watchdog when unregister

[PATCH] watchdog: force to stop the watchdog when unregister

[roy.qing.li]

From: Li RongQing <roy.qing.li@gmail.com>

force to stop the watchdog when unregister, otherwise the watchdog maybe
be suspended and be run after the module is unloaded, like the below:
   $modprobe softdog
   $echo 1 >/dev/watchdog
   $modprobe -r softdog

   CPU 20 Unable to handle kernel paging request at virtual address
   Oops[#1]:
   CPU: 20 PID: 0 Comm: swapper/20 Not tainted 4.1.13-WR8.0.0.0_standard #6
   ...
   Modules linked in: [last unloaded: softdog]
   	....
   Call Trace:
   [<ffffffff801e142c>] cascade+0x34/0xb0
   [<ffffffff801e1964>] run_timer_softirq+0x30c/0x368
   [<ffffffff80181044>] __do_softirq+0x1ec/0x418
   [<ffffffff801815d0>] irq_exit+0x90/0x98
   [<ffffffff8010749c>] plat_irq_dispatch+0xa4/0x140
   [<ffffffff80152740>] ret_from_irq+0x0/0x4
   [<ffffffff801529e0>] __r4k_wait+0x20/0x40
   [<ffffffff801c2278>] cpu_startup_entry+0x2a0/0x368
   [<ffffffff8015fa64>] start_secondary+0x444/0x4d8

Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 drivers/watchdog/watchdog_dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 56a649e..2f3139b 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -569,6 +569,8 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 
 int watchdog_dev_unregister(struct watchdog_device *wdd)
 {
+	watchdog_stop(wdd);
+
 	mutex_lock(&wdd->lock);
 	set_bit(WDOG_UNREGISTERED, &wdd->status);
 	mutex_unlock(&wdd->lock);
-- 
2.1.4

Re: [PATCH] watchdog: force to stop the watchdog when unregister

[Guenter Roeck]

On 12/17/2015 12:57 AM, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
>
> force to stop the watchdog when unregister, otherwise the watchdog maybe
> be suspended and be run after the module is unloaded, like the below:
>     $modprobe softdog
>     $echo 1 >/dev/watchdog
>     $modprobe -r softdog
>
>     CPU 20 Unable to handle kernel paging request at virtual address
>     Oops[#1]:
>     CPU: 20 PID: 0 Comm: swapper/20 Not tainted 4.1.13-WR8.0.0.0_standard #6
>     ...
>     Modules linked in: [last unloaded: softdog]
>     	....
>     Call Trace:
>     [<ffffffff801e142c>] cascade+0x34/0xb0
>     [<ffffffff801e1964>] run_timer_softirq+0x30c/0x368
>     [<ffffffff80181044>] __do_softirq+0x1ec/0x418
>     [<ffffffff801815d0>] irq_exit+0x90/0x98
>     [<ffffffff8010749c>] plat_irq_dispatch+0xa4/0x140
>     [<ffffffff80152740>] ret_from_irq+0x0/0x4
>     [<ffffffff801529e0>] __r4k_wait+0x20/0x40
>     [<ffffffff801c2278>] cpu_startup_entry+0x2a0/0x368
>     [<ffffffff8015fa64>] start_secondary+0x444/0x4d8
>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>

Real problem, wrong solution. We can not unconditionally stop all watchdogs
in the watchdog core when unregistering a watchdog.

Do you get an error message telling you that the watchdog isn't stopped
after the echo command ? There should have been a "watchdog did not stop!"
message on the console. This is per design.

The problem here is that the softdog has static variables which are accessed
if its timer is still running after the driver is unloaded. This is a problem
in the softdog driver, not in the watchdog core.

The softdog driver should refuse to unload while still running. Anything
else would defeat the purpose of MAGICCLOSE and the requirement to write
the magic character before closing the watchdog.

Thanks,
Guenter

> ---
>   drivers/watchdog/watchdog_dev.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 56a649e..2f3139b 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -569,6 +569,8 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>
>   int watchdog_dev_unregister(struct watchdog_device *wdd)
>   {
> +	watchdog_stop(wdd);
> +
>   	mutex_lock(&wdd->lock);
>   	set_bit(WDOG_UNREGISTERED, &wdd->status);
>   	mutex_unlock(&wdd->lock);
>

Re: [PATCH] watchdog: force to stop the watchdog when unregister

[Li RongQing]

On Thu, Dec 17, 2015 at 7:04 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>
>
> Real problem, wrong solution. We can not unconditionally stop all watchdogs
> in the watchdog core when unregistering a watchdog.
>
> Do you get an error message telling you that the watchdog isn't stopped
> after the echo command ? There should have been a "watchdog did not stop!"
> message on the console. This is per design.

Yes, I get  "watchdog did not stop!"

>
> The problem here is that the softdog has static variables which are accessed
> if its timer is still running after the driver is unloaded. This is a
> problem
> in the softdog driver, not in the watchdog core.
>
> The softdog driver should refuse to unload while still running. Anything
> else would defeat the purpose of MAGICCLOSE and the requirement to write
> the magic character before closing the watchdog.

Ok, I will resend a patch as you suggested

thanks

-Roy

>
> Thanks,
> Guenter