Re: [PATCH 2/2] watchdog: gpio-wdt: Add panic notifier

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hello,

On Mon, Nov 09, 2015 at 07:19:09AM -0800, Guenter Roeck wrote:
> On 11/09/2015 01:55 AM, Alexander Stein wrote:
> >This notifier is required when the watchdog is configured as always running
> >because in this case the watchdog will be triggered when the kernel panics
> >at boot before any application could open the device, e.g. because the
> >rootfs is broken. This should result in a resetting system. Thus we
> >register a panic notifier which stops triggering the watchdog.
> >
> 
> Shouldn't the timer be stopped instead ?

What do you mean saying "timer"? The hardware? This might or might not
be possible.

I think this depends on policy what you want. There are people that just
want to calm the watchdog such that it doesn't interfere with the
system. For these it might be right to stop the timer. If however the
watchdog is responsible to bring a non-responding system back into
operation it sounds right to stop petting the watchdog and let it reset
the machine. (There are a few things that might complicate the logic,
i.e. with panic=5 on the kernel command line it might make sense to keep
the timer until the 5 seconds after panic are over.)

> >diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> >index c7b8a06..aaa0815 100644
> >--- a/drivers/watchdog/gpio_wdt.c
> >+++ b/drivers/watchdog/gpio_wdt.c
> >@@ -233,11 +245,19 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto error_unregister;
> >
> >+	priv->panic_notifier.notifier_call = gpio_wdt_notify_panic;
> >+	ret = atomic_notifier_chain_register(&panic_notifier_list,
> >+					     &priv->panic_notifier);
> >+	if (ret)
> >+		goto error_unregister_notify;
> >+
> >  	if (priv->always_running)
> >  		gpio_wdt_start_impl(priv);
> >
> >  	return 0;
> >
> >+error_unregister_notify:
> >+	unregister_reboot_notifier(&priv->reboot_notifier);

The logic is wrong here. If atomic_notifier_chain_register failed you
shouldn't call unregister_reboot_notifier.

Best regards
Uwe

> >  error_unregister:
> >  	watchdog_unregister_device(&priv->wdd);
> >  	return ret;

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |