[PATCH v6 05/19] watchdog: orion: Make sure the watchdog is initially stopped

On Fri, Feb 07, 2014 at 10:44:53AM -0500, Jason Cooper wrote:

On Fri, Feb 07, 2014 at 12:17:28PM -0300, Ezequiel Garcia wrote:

quoted

On Fri, Feb 07, 2014 at 05:38:09AM -0800, Guenter Roeck wrote:

quoted

On 02/07/2014 02:40 AM, Ezequiel Garcia wrote:

quoted

On Thu, Feb 06, 2014 at 06:02:56PM -0800, Guenter Roeck wrote:

quoted

On 02/06/2014 09:20 AM, Ezequiel Garcia wrote:

quoted

Having the watchdog initially fully stopped is important to avoid
any spurious watchdog triggers, in case the registers are not in
its reset state.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Tested-by: Willy Tarreau <w@1wt.eu>
Signed-off-by: Ezequiel Garcia <redacted>
---
   drivers/watchdog/orion_wdt.c | 3 +++
   1 file changed, 3 insertions(+)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 6746033..2dbeee9 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c

@@ -142,6 +142,9 @@ static int orion_wdt_probe(struct platform_device *pdev)
   	orion_wdt.max_timeout = wdt_max_duration;
   	watchdog_init_timeout(&orion_wdt, heartbeat, &pdev->dev);

+	/* Let's make sure the watchdog is fully stopped */
+	orion_wdt_stop(&orion_wdt);
+

Actually we just had that in another driver, and I stumbled over it there.

Problem with stopping the watchdog in probe unconditionally is that you can
use it to defeat nowayout: unload the module, then load it again,
and the watchdog is stopped even if nowayout is true.

How often would a user legitimately want to unload/load the watchdog
module?

Not sure what you are saying here. If you don't think the nowayout option
should be supported, drop it. Don't claim it is supported when it isn't.

quoted

quoted

quoted

Hm... I see.

quoted

Is this really what you want ? Or, in other words, what is the problem
you are trying to solve ?

Well, this is related to the discussion about the bootloader not
reseting the watchdog properly, provoking spurious watchdog triggering.

Jason Gunthorpe explained [1] that we needed a particular sequence:

  1. Disable WDT
  2. Clear bridge
  3. Enable WDT

We added the irq handling to satisfy (2), and the watchdog stop for (1).

The watchdog stop was agreed specifically [2].

Ideas?

Other drivers assume that if the watchdog is running, it is supposed
to be running. The more common approach in such cases is to ping the
watchdog once to give userspace more time to get ready, but leave
it enabled. So you could check if the watchdog is enabled, and if
it was enabled re-enable it after initialization is complete
(and maybe log a message stating that the watchdog is enabled).

If you don't want to do that, and if you are defeating nowayout
on purpose to fix a problem with a broken bootloader,
you should at least put in comment describing the problem you are
trying to solve, and that you accept breaking nowayout with your fix.

Yes, this should be commented.

quoted

I'm not fond of not having "nowayout" option on our driver, given I'm sure
it's a watchdog feature for a good reason.

On the other side, I can't see how can we distinguish a previously
and explicitly enabled watchdog, from a spurious enable by broken bootloader.

How about we just don't define module_exit() and leave a comment as
such?  It's not unprecedented, a couple of the atm drivers are
explicitly setup like this (uPD98402.c, zatm.c, eni.c).

Or don't support tristate in the first place. There was some argument from
others earlier that a watchdog should never be optional. Or drop the nowayout
option from the driver.

There is one problem, though, if you don't support a pre-enabled watchdog:
If the system dies before the watchdog application starts running, it will
hang. This is the reason why the watchdog is enabled on purpose by some
boot loaders.

Guenter