From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932613AbaEGLxw (ORCPT ); Wed, 7 May 2014 07:53:52 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:39720 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755574AbaEGLxv (ORCPT ); Wed, 7 May 2014 07:53:51 -0400 Message-ID: <1399463549.4536.10.camel@weser.hi.pengutronix.de> Subject: Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots From: Lucas Stach To: Guenter Roeck Cc: Maxime Ripard , Russell King , linux-watchdog@vger.kernel.org, Arnd Bergmann , Catalin Marinas , Will Deacon , linux-kernel@vger.kernel.org, Jonas Jensen , Wim Van Sebroeck , linux-arm-kernel@lists.infradead.org Date: Wed, 07 May 2014 13:52:29 +0200 In-Reply-To: <20140503042925.GB5882@roeck-us.net> References: <1398958893-30049-1-git-send-email-linux@roeck-us.net> <1398958893-30049-2-git-send-email-linux@roeck-us.net> <20140503012243.GG4090@lukather> <20140503042925.GB5882@roeck-us.net> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.8.5-2+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:6f8:1178:2:fa0f:41ff:fe58:4010 X-SA-Exim-Mail-From: l.stach@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > --- > > > 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. Regards, Lucas -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |