Hello Uwe.
Uwe Kleine-König writes:
> Hello,
>
> On Wed, Oct 07, 2015 at 12:19:11PM +0000, Harald Geyer wrote:
> > This allows the system to actually halt even if userspace forgot to
> > disable the watchdog first. Old behaviour was that the watchdog forced
> > the system to boot again.
> >
> > Signed-off-by: Harald Geyer <[email protected]>
> > ---
> > Changes since v2:
> > * split this out as a separate patch
> >
> > drivers/watchdog/stmp3xxx_rtc_wdt.c | 28 ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c
> > b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> > index 3ee6128..e09a01f 100644
> > --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
> > +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> > @@ -14,6 +14,8 @@
> > #include <linux/watchdog.h>
> > #include <linux/platform_device.h>
> > #include <linux/stmp3xxx_rtc_wdt.h>
> > +#include <linux/notifier.h>
> > +#include <linux/reboot.h>
> >
> > #define WDOG_TICK_RATE 1000 /* 1 kHz clock */
> > #define STMP3XXX_DEFAULT_TIMEOUT 19
> > @@ -69,6 +71,28 @@ static struct watchdog_device stmp3xxx_wdd = {
> > .status = WATCHDOG_NOWAYOUT_INIT_STATUS,
> > };
> >
> > +static int wdt_notify_sys(struct notifier_block *nb, unsigned long code,
> > + void *unused)
> > +{
> > + struct device *dev = watchdog_get_drvdata(&stmp3xxx_wdd);
> > + struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(dev);
> > +
> > + switch (code) {
> > + case SYS_DOWN: /* keep enabled, system might crash while going down */
> > + break;
> > + case SYS_HALT: /* allow the system to actually halt */
>
> <nitpick>
> You used a tab before the comment "keep enabled ...", but spaces for
> "allow the system ...".
> </nitpick>
Ok
> > + case SYS_POWER_OFF:
> > + wdt_stop(&stmp3xxx_wdd);
> > + break;
> > + }
> > +
> > + return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block wdt_notifier = {
> > + .notifier_call = wdt_notify_sys,
>
> s/ / /
Ok
> > +};
> > +
> > static int stmp3xxx_wdt_probe(struct platform_device *pdev)
> > {
> > int ret;
> > @@ -84,6 +108,9 @@ static int stmp3xxx_wdt_probe(struct platform_device
> > *pdev)
> > return ret;
> > }
> >
> > + if (register_reboot_notifier(&wdt_notifier))
> > + dev_warn(&pdev->dev, "cannot register reboot notifier\n");
>
> OK, you don't fail when registering the notifier fails, but ...
>
> > dev_info(&pdev->dev, "initialized watchdog with heartbeat %ds\n",
> > stmp3xxx_wdd.timeout);
> > return 0;
> > @@ -91,6 +118,7 @@ static int stmp3xxx_wdt_probe(struct platform_device
> > *pdev)
> >
> > static int stmp3xxx_wdt_remove(struct platform_device *pdev)
> > {
> > + unregister_reboot_notifier(&wdt_notifier);
>
> ... I think it's not ok to unconditionally unregister it then, is it?
> (Looking at how unregister_reboot_notifier is implemented it might work
> as intended, but if that's only by chance I wouldn't rely on it.)
I'm not sure what you refer to by "how unregister_reboot_notifier is
implemented". The documentation for unregister_reboot_notifier says:
/**
* unregister_reboot_notifier - Unregister previously registered reboot
notifier
* @nb: Hook to be unregistered
*
* Unregisters a previously registered reboot
* notifier function.
*
* Returns zero on success, or %-ENOENT on failure.
*/
If register_reboot_notifier failed, then unregister_reboot_notifier will
simply return -ENOENT, I don't see a problem here. As this is documented
API I think we can rely on it.
Can you also have a look at the following patch (next in this series,
unfortunately I failed a formatting the subject prefix properly):
http://www.spinics.net/lists/linux-watchdog/msg07483.html
That one is particularly messy, as I don't have all the HW documentation
I wish I had, so I'd appreciate if somebody, who has experience with
the hardware, looked at it.
Thanks,
Harald
> Best regards
> Uwe
>
> > watchdog_unregister_device(&stmp3xxx_wdd);
> > return 0;
> > }
> > --
> > 1.7.10.4
> >
> >
> >
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
--
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
