Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Lucas Stach]

Am Mittwoch, den 07.05.2014, 06:01 -0700 schrieb Guenter Roeck:
> 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@xxxxxxxxxxxx>
> >>>> ---
> >>>>   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.

That sounds rather fragile to me. I would like to have a more explicit
property to control this behavior.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/