| Message ID | 1398958893-30049-2-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Guenter, This looks pretty sensible to me (and the arm/arm64 bits look fine too), but I have one question below... On Thu, May 01, 2014 at 04:41:29PM +0100, 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); > + } > +} What reboot_mode values would you expect a watchdog to support other than REBOOT_HARD? Also, is the cmd even useful here? Will
On 05/02/2014 03:01 AM, Will Deacon wrote: > Hi Guenter, > > This looks pretty sensible to me (and the arm/arm64 bits look fine too), but > I have one question below... > > On Thu, May 01, 2014 at 04:41:29PM +0100, 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); >> + } >> +} > > What reboot_mode values would you expect a watchdog to support other than > REBOOT_HARD? Also, is the cmd even useful here? > Answer is "I don't know" in both cases. I thought about dropping the parameters, but then I thought it does not hurt to have them around either. I am open to suggestions and to dropping the parameters if everyone agrees that they are useless. Guenter
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 Or maybe we can just use the start callback, with the min timeout already registered, and prevent the user to kick the watchdog. Maxime
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. > Or maybe we can just use the start callback, with the min timeout already > registered, and prevent the user to kick the watchdog. > Doesn't always work, unfortunately, even now. The moxart driver causes an explicit and immediate reset. Also, some watchdogs don't reset the system directly but get an interrupt, which then calls the reset handler. Which, in our case, would call the start callback again, and you would have an endless loop. Guenter
On Fri, May 02, 2014 at 09:29:25PM -0700, Guenter Roeck wrote: > > > + 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. The A31 we were discussing about in the other thread that doesn't have a watchdog driver yet has four, identical, watchogs. I'm not really concerned about the mentionned issue, since they will never be removed, but the situation might happen with an on-SoC watchdog and an i2c one (if that even exists). But yes, right, that can be postponed. > > Or maybe we can just use the start callback, with the min timeout already > > registered, and prevent the user to kick the watchdog. > > > Doesn't always work, unfortunately, even now. The moxart driver causes > an explicit and immediate reset. Also, some watchdogs don't reset the system > directly but get an interrupt, which then calls the reset handler. Which, > in our case, would call the start callback again, and you would have an endless > loop. Ok. You have my Acked-by then. Thanks! Maxime
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) you can decrease one level of indentation: if (wdd_reboot_dev && wdd_reboot_dev->ops->reboot) also, shouldn't you test if 'ops' is valid too ? In that case: if (wdd_reboot_dev && wdd_reboot_dev->ops && wdd_reboot_dev->ops->reboot)
On Mon, May 05, 2014 at 01:36:06PM -0500, Felipe Balbi wrote: > 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) > > you can decrease one level of indentation: > > if (wdd_reboot_dev && wdd_reboot_dev->ops->reboot) > > also, shouldn't you test if 'ops' is valid too ? In that case: > > if (wdd_reboot_dev && wdd_reboot_dev->ops && > wdd_reboot_dev->ops->reboot) > Hmmm ... makes me think. If ops->reboot is not set, wdd_reboot_dev should be NULL to start with, since it is only initialized if ops->reboot is not NULL. It is not common in the Linux kernel to re-check pointers if the condition can not occur, so I don't think it should be done here. In other words, I should probably remove the ops check as well. Guenter
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. Regards, Lucas
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
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@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. That sounds rather fragile to me. I would like to have a more explicit property to control this behavior. Regards, Lucas
On Wed, May 07, 2014 at 06:01:31AM -0700, Guenter Roeck wrote: > >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. It doesn't work, because the DT nodes are usually ordered by physical address. Maxime
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; + return 0; } EXPORT_SYMBOL_GPL(watchdog_register_device); @@ -181,6 +195,9 @@ void watchdog_unregister_device(struct watchdog_device *wdd) if (wdd == NULL) return; + if (wdd == wdd_reboot_dev) + wdd_reboot_dev = NULL; + devno = wdd->cdev.dev; ret = watchdog_dev_unregister(wdd); if (ret) diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index 2a3038e..0d204af 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/reboot.h> #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -23,6 +24,7 @@ struct watchdog_device; * @start: The routine for starting the watchdog device. * @stop: The routine for stopping the watchdog device. * @ping: The routine that sends a keepalive ping to the watchdog device. + * @reboot: The routine for rebooting the system * @status: The routine that shows the status of the watchdog device. * @set_timeout:The routine for setting the watchdog devices timeout value. * @get_timeleft:The routine that get's the time that's left before a reset. @@ -42,6 +44,8 @@ struct watchdog_ops { int (*stop)(struct watchdog_device *); /* optional operations */ int (*ping)(struct watchdog_device *); + void (*reboot)(struct watchdog_device *, enum reboot_mode, + const char *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); @@ -142,4 +146,11 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd, extern int watchdog_register_device(struct watchdog_device *); extern void watchdog_unregister_device(struct watchdog_device *); +#ifdef CONFIG_WATCHDOG_CORE +extern void watchdog_do_reboot(enum reboot_mode mode, const char *cmd); +#else +static inline void watchdog_do_reboot(enum reboot_mode mode, + const char *cmd) { } +#endif + #endif /* ifndef _LINUX_WATCHDOG_H */
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(+)