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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 07, 2014 at 12:17:28PM -0300, Ezequiel Garcia wrote:
> On Fri, Feb 07, 2014 at 05:38:09AM -0800, Guenter Roeck wrote:
> > On 02/07/2014 02:40 AM, Ezequiel Garcia wrote:
> > > On Thu, Feb 06, 2014 at 06:02:56PM -0800, Guenter Roeck wrote:
> > >> On 02/06/2014 09:20 AM, Ezequiel Garcia wrote:
> > >>> 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@xxxxxxxxxxxx>
> > >>> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@xxxxxxxxx>
> > >>> Tested-by: Willy Tarreau <w@xxxxxx>
> > >>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@xxxxxxxxxxxxxxxxxx>
> > >>> ---
> > >>>    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?

> > >
> > > Hm... I see.
> > >
> > >> 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.

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

thx,

Jason.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [CentOS ARM]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]     [Photos]