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

[Guenter Roeck <linux@roeck-us.net>]

Hi Uwe,

On 11/09/2015 11:02 AM, Uwe Kleine-König wrote:
> 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 meant the timer referenced with the variable 'timer' in struct gpio_wdt_priv,
and "stop timer' would translate to somoething like 'mod_timer(&priv->timer, 0);'.
Sorry for not being more specific.

> 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.)
>
For my part I don't really want or suggest anything. I copied you on the patch
since you were involved in improving the driver, and I thought you might have
valuable input.

Having said that, I agree - since this is dealing with a panic, it may well
be that manipulating it may not be possible. So maybe it is best left alone
at this point.

Overall this might be a generic problem, not specifically related to the gpio
watchdog - what should be done with a watchdog if the system panics ?
Should the watchdog be disabled, or should it time out and force-reboot the
system ?

Up to now, the watchdog is left running, and will cause a hard reset after
it times out. This will be the first exception to this rule. As a result,
if the soft reboot caused by the panic() fails to reboot the system, the
system may be left in an unusable state. Is this ok / acceptable ?

>>> 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.
>

I tend to agree - all but to calls to register a panic notifier don't
check the return value from atomic_notifier_chain_register().

Thanks,
Guenter