Messages in this thread | | | Date | Wed, 07 May 2014 06:01:31 -0700 | From | Guenter Roeck <> | Subject | Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots |
| |
On 05/07/2014 04:52 AM, Lucas Stach wrote: > Hi Guenter, > > Am Freitag, den 02.05.2014, 21:29 -0700 schrieb Guenter Roeck: >> On Fri, May 02, 2014 at 06:22:43PM -0700, Maxime Ripard wrote: >>> Hi Guenter, >>> >>> On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote: >>>> Some hardware implements reboot through its watchdog hardware, >>>> for example by triggering a watchdog timeout. Platform specific >>>> code starts to spread into watchdog drivers, typically by setting >>>> pointers to a callback functions which is then called from the >>>> platform reset handler. >>>> >>>> To simplify code and provide a unified API to trigger reboots by >>>> watchdog drivers, provide a single API to trigger such reboots >>>> through the watchdog subsystem. >>>> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>>> drivers/watchdog/watchdog_core.c | 17 +++++++++++++++++ >>>> include/linux/watchdog.h | 11 +++++++++++ >>>> 2 files changed, 28 insertions(+) >>>> >>>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c >>>> index cec9b55..4ec6e2f 100644 >>>> --- a/drivers/watchdog/watchdog_core.c >>>> +++ b/drivers/watchdog/watchdog_core.c >>>> @@ -43,6 +43,17 @@ >>>> static DEFINE_IDA(watchdog_ida); >>>> static struct class *watchdog_class; >>>> >>>> +static struct watchdog_device *wdd_reboot_dev; >>>> + >>>> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd) >>>> +{ >>>> + if (wdd_reboot_dev) { >>>> + if (wdd_reboot_dev->ops->reboot) >>>> + wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd); >>>> + } >>>> +} >>>> +EXPORT_SYMBOL(watchdog_do_reboot); >>>> + >>>> static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) >>>> { >>>> /* >>>> @@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd) >>>> return ret; >>>> } >>>> >>>> + if (wdd->ops->reboot) >>>> + wdd_reboot_dev = wdd; >>>> + >>> >>> Overall, it looks really great, but I guess we can make it a >>> list. Otherwise, we might end up in a situation where we could not >>> reboot anymore, like this one for example: >>> - a first watchdog is probed, registers a reboot function >>> - a second watchdog is probed, registers a reboot function that >>> overwrites the first one. >>> - then, the second watchdog disappears for some reason, and the >>> reboot is set to NULL >>> >> I thought about that, but how likely (or unlikely) is that to ever happen ? >> So I figured it is not worth the effort, and would just add complexity without >> real gain. We could always add the list later if we ever encounter a situation >> where two watchdogs in the same system provide a reboot callback. >> > > While this is not directly related to the issue you are fixing with this > series, I would like to have it considered when talking about a watchdog > system reboot API. > > On i.MX we have the same situation where we have to reboot through the > SoC watchdog. This works, but may leave the external components of the > system (those not integrated in the SoC) in an undefined state. So if we > have a PMIC with integrated watchdog we would rather like to this one to > reboot the system, as it the reset is then much more closer to a > power-on-reset. > > This means we could have multiple watchdogs in the system, where we > really want a specific one (maybe designated through a DT property) to > do the reset. This isn't compatible with the "last watchdog that > registers a handler wins the system reset" logic in your patch. >
Wouldn't the order in which watchdogs are configured in dt define that ? The last one wins.
Thanks, Guenter
| |