* Re: [PATCH] RTC: Implement pretimeout watchdog for DS1307 [not found] <20200804051743.19115-1-mark.tomlinson@alliedtelesis.co.nz> @ 2020-08-04 15:20 ` Alexandre Belloni 2020-08-04 16:14 ` Guenter Roeck 0 siblings, 1 reply; 3+ messages in thread From: Alexandre Belloni @ 2020-08-04 15:20 UTC (permalink / raw) To: Mark Tomlinson Cc: a.zummo, linux-rtc, linux-kernel, Guenter Roeck, linux-watchdog Hi, The subject prefix is not correct, it should be rtc: ds1307: Also, shouldn't that kind of software timeout which doesn't actually depend on the hardware better be handled in the watchdog core? Then this will benefit all the watchdog and will certainly avoid a lot of code duplication. On 04/08/2020 17:17:43+1200, Mark Tomlinson wrote: > If the hardware watchdog in the clock chip simply pulls the reset line > of the CPU, then there is no chance to write a stack trace to help > determine what may have been blocking the CPU. > > This patch adds a pretimeout to the watchdog, which, if enabled, sets > a timer to go off before the hardware watchdog kicks in, and calls > the standard pretimeout function, which can (for example) call panic. > > Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz> > --- > drivers/rtc/rtc-ds1307.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c > index 49702942bb08..647f8659d0bd 100644 > --- a/drivers/rtc/rtc-ds1307.c > +++ b/drivers/rtc/rtc-ds1307.c > @@ -23,6 +23,7 @@ > #include <linux/clk-provider.h> > #include <linux/regmap.h> > #include <linux/watchdog.h> > +#include <linux/timer.h> > > /* > * We can't determine type by probing, but if we expect pre-Linux code > @@ -174,6 +175,10 @@ struct ds1307 { > #ifdef CONFIG_COMMON_CLK > struct clk_hw clks[2]; > #endif > +#ifdef CONFIG_WATCHDOG_CORE > + struct timer_list soft_timer; > + struct watchdog_device *wdt; > +#endif > }; > > struct chip_desc { > @@ -863,12 +868,34 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset) > } > > #ifdef CONFIG_WATCHDOG_CORE > +static void ds1388_soft_wdt_expire(struct timer_list *soft_timer) > +{ > + struct ds1307 *ds1307 = container_of(soft_timer, struct ds1307, soft_timer); > + > + watchdog_notify_pretimeout(ds1307->wdt); > +} > + > +static void ds1388_soft_timer_set(struct watchdog_device *wdt_dev) > +{ > + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); > + int soft_timeout; > + > + if (wdt_dev->pretimeout > 0) { > + soft_timeout = wdt_dev->timeout - wdt_dev->pretimeout; > + mod_timer(&ds1307->soft_timer, jiffies + soft_timeout * HZ); > + } else { > + del_timer(&ds1307->soft_timer); > + } > +} > + > static int ds1388_wdt_start(struct watchdog_device *wdt_dev) > { > struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); > u8 regs[2]; > int ret; > > + ds1388_soft_timer_set(wdt_dev); > + > ret = regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG, > DS1388_BIT_WF, 0); > if (ret) > @@ -900,6 +927,7 @@ static int ds1388_wdt_stop(struct watchdog_device *wdt_dev) > { > struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); > > + del_timer(&ds1307->soft_timer); > return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL, > DS1388_BIT_WDE | DS1388_BIT_RST, 0); > } > @@ -909,6 +937,7 @@ static int ds1388_wdt_ping(struct watchdog_device *wdt_dev) > struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); > u8 regs[2]; > > + ds1388_soft_timer_set(wdt_dev); > return regmap_bulk_read(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs, > sizeof(regs)); > } > @@ -923,6 +952,7 @@ static int ds1388_wdt_set_timeout(struct watchdog_device *wdt_dev, > regs[0] = 0; > regs[1] = bin2bcd(wdt_dev->timeout); > > + ds1388_soft_timer_set(wdt_dev); > return regmap_bulk_write(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs, > sizeof(regs)); > } > @@ -1652,7 +1682,8 @@ static void ds1307_clks_register(struct ds1307 *ds1307) > > #ifdef CONFIG_WATCHDOG_CORE > static const struct watchdog_info ds1388_wdt_info = { > - .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | > + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT, > .identity = "DS1388 watchdog", > }; > > @@ -1681,6 +1712,8 @@ static void ds1307_wdt_register(struct ds1307 *ds1307) > wdt->timeout = 99; > wdt->max_timeout = 99; > wdt->min_timeout = 1; > + ds1307->wdt = wdt; > + timer_setup(&ds1307->soft_timer, ds1388_soft_wdt_expire, 0); > > watchdog_init_timeout(wdt, 0, ds1307->dev); > watchdog_set_drvdata(wdt, ds1307); > -- > 2.28.0 > -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] RTC: Implement pretimeout watchdog for DS1307 2020-08-04 15:20 ` [PATCH] RTC: Implement pretimeout watchdog for DS1307 Alexandre Belloni @ 2020-08-04 16:14 ` Guenter Roeck 2020-08-04 21:38 ` Chris Packham 0 siblings, 1 reply; 3+ messages in thread From: Guenter Roeck @ 2020-08-04 16:14 UTC (permalink / raw) To: Alexandre Belloni, Mark Tomlinson Cc: a.zummo, linux-rtc, linux-kernel, linux-watchdog On 8/4/20 8:20 AM, Alexandre Belloni wrote: > Hi, > > The subject prefix is not correct, it should be rtc: ds1307: > > Also, shouldn't that kind of software timeout which doesn't actually > depend on the hardware better be handled in the watchdog core? Then this > will benefit all the watchdog and will certainly avoid a lot of code > duplication. > Good point. I absolutely agree. We'd have to hash out some details, such as how and when to enable it, but this definitely belongs into the watchdog core if it is considered useful. On the other side, the softdog already implements this. Why not just instantiate the softdog as second watchdog and use it for the purpose of handling pretimeouts ? Thanks, Guenter > On 04/08/2020 17:17:43+1200, Mark Tomlinson wrote: >> If the hardware watchdog in the clock chip simply pulls the reset line >> of the CPU, then there is no chance to write a stack trace to help >> determine what may have been blocking the CPU. >> >> This patch adds a pretimeout to the watchdog, which, if enabled, sets >> a timer to go off before the hardware watchdog kicks in, and calls >> the standard pretimeout function, which can (for example) call panic. >> >> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz> >> --- >> drivers/rtc/rtc-ds1307.c | 35 ++++++++++++++++++++++++++++++++++- >> 1 file changed, 34 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c >> index 49702942bb08..647f8659d0bd 100644 >> --- a/drivers/rtc/rtc-ds1307.c >> +++ b/drivers/rtc/rtc-ds1307.c >> @@ -23,6 +23,7 @@ >> #include <linux/clk-provider.h> >> #include <linux/regmap.h> >> #include <linux/watchdog.h> >> +#include <linux/timer.h> >> >> /* >> * We can't determine type by probing, but if we expect pre-Linux code >> @@ -174,6 +175,10 @@ struct ds1307 { >> #ifdef CONFIG_COMMON_CLK >> struct clk_hw clks[2]; >> #endif >> +#ifdef CONFIG_WATCHDOG_CORE >> + struct timer_list soft_timer; >> + struct watchdog_device *wdt; >> +#endif >> }; >> >> struct chip_desc { >> @@ -863,12 +868,34 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset) >> } >> >> #ifdef CONFIG_WATCHDOG_CORE >> +static void ds1388_soft_wdt_expire(struct timer_list *soft_timer) >> +{ >> + struct ds1307 *ds1307 = container_of(soft_timer, struct ds1307, soft_timer); >> + >> + watchdog_notify_pretimeout(ds1307->wdt); >> +} >> + >> +static void ds1388_soft_timer_set(struct watchdog_device *wdt_dev) >> +{ >> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >> + int soft_timeout; >> + >> + if (wdt_dev->pretimeout > 0) { >> + soft_timeout = wdt_dev->timeout - wdt_dev->pretimeout; >> + mod_timer(&ds1307->soft_timer, jiffies + soft_timeout * HZ); >> + } else { >> + del_timer(&ds1307->soft_timer); >> + } >> +} >> + >> static int ds1388_wdt_start(struct watchdog_device *wdt_dev) >> { >> struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >> u8 regs[2]; >> int ret; >> >> + ds1388_soft_timer_set(wdt_dev); >> + >> ret = regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG, >> DS1388_BIT_WF, 0); >> if (ret) >> @@ -900,6 +927,7 @@ static int ds1388_wdt_stop(struct watchdog_device *wdt_dev) >> { >> struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >> >> + del_timer(&ds1307->soft_timer); >> return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL, >> DS1388_BIT_WDE | DS1388_BIT_RST, 0); >> } >> @@ -909,6 +937,7 @@ static int ds1388_wdt_ping(struct watchdog_device *wdt_dev) >> struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >> u8 regs[2]; >> >> + ds1388_soft_timer_set(wdt_dev); >> return regmap_bulk_read(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs, >> sizeof(regs)); >> } >> @@ -923,6 +952,7 @@ static int ds1388_wdt_set_timeout(struct watchdog_device *wdt_dev, >> regs[0] = 0; >> regs[1] = bin2bcd(wdt_dev->timeout); >> >> + ds1388_soft_timer_set(wdt_dev); >> return regmap_bulk_write(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs, >> sizeof(regs)); >> } >> @@ -1652,7 +1682,8 @@ static void ds1307_clks_register(struct ds1307 *ds1307) >> >> #ifdef CONFIG_WATCHDOG_CORE >> static const struct watchdog_info ds1388_wdt_info = { >> - .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | >> + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT, >> .identity = "DS1388 watchdog", >> }; >> >> @@ -1681,6 +1712,8 @@ static void ds1307_wdt_register(struct ds1307 *ds1307) >> wdt->timeout = 99; >> wdt->max_timeout = 99; >> wdt->min_timeout = 1; >> + ds1307->wdt = wdt; >> + timer_setup(&ds1307->soft_timer, ds1388_soft_wdt_expire, 0); >> >> watchdog_init_timeout(wdt, 0, ds1307->dev); >> watchdog_set_drvdata(wdt, ds1307); >> -- >> 2.28.0 >> > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Re: [PATCH] RTC: Implement pretimeout watchdog for DS1307 2020-08-04 16:14 ` Guenter Roeck @ 2020-08-04 21:38 ` Chris Packham 0 siblings, 0 replies; 3+ messages in thread From: Chris Packham @ 2020-08-04 21:38 UTC (permalink / raw) To: Guenter Roeck, Alexandre Belloni, Mark Tomlinson Cc: a.zummo, linux-rtc, linux-kernel, linux-watchdog On 5/08/20 4:14 am, Guenter Roeck wrote: > On 8/4/20 8:20 AM, Alexandre Belloni wrote: >> Hi, >> >> The subject prefix is not correct, it should be rtc: ds1307: >> >> Also, shouldn't that kind of software timeout which doesn't actually >> depend on the hardware better be handled in the watchdog core? Then this >> will benefit all the watchdog and will certainly avoid a lot of code >> duplication. >> > Good point. I absolutely agree. We'd have to hash out some details, > such as how and when to enable it, but this definitely belongs into > the watchdog core if it is considered useful. > > On the other side, the softdog already implements this. Why not just > instantiate the softdog as second watchdog and use it for the purpose > of handling pretimeouts ? > > Thanks, > Guenter One advantage of this kind of approach (implemented more generically) is that the timers are linked. They get serviced together and any updates to the timeout happen at the same time. One possible implementation path would be to have it kick in in the watchdog core if the flags supplied by the driver don't already contain WDIOF_PRETIMEOUT (probably tucked behind a config flag at least initially). Talking with Mark I think we probably will go with the softdog approach and deal with keeping them in sync in our monitoring application. > >> On 04/08/2020 17:17:43+1200, Mark Tomlinson wrote: >>> If the hardware watchdog in the clock chip simply pulls the reset line >>> of the CPU, then there is no chance to write a stack trace to help >>> determine what may have been blocking the CPU. >>> >>> This patch adds a pretimeout to the watchdog, which, if enabled, sets >>> a timer to go off before the hardware watchdog kicks in, and calls >>> the standard pretimeout function, which can (for example) call panic. >>> >>> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz> >>> --- >>> drivers/rtc/rtc-ds1307.c | 35 ++++++++++++++++++++++++++++++++++- >>> 1 file changed, 34 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c >>> index 49702942bb08..647f8659d0bd 100644 >>> --- a/drivers/rtc/rtc-ds1307.c >>> +++ b/drivers/rtc/rtc-ds1307.c >>> @@ -23,6 +23,7 @@ >>> #include <linux/clk-provider.h> >>> #include <linux/regmap.h> >>> #include <linux/watchdog.h> >>> +#include <linux/timer.h> >>> >>> /* >>> * We can't determine type by probing, but if we expect pre-Linux code >>> @@ -174,6 +175,10 @@ struct ds1307 { >>> #ifdef CONFIG_COMMON_CLK >>> struct clk_hw clks[2]; >>> #endif >>> +#ifdef CONFIG_WATCHDOG_CORE >>> + struct timer_list soft_timer; >>> + struct watchdog_device *wdt; >>> +#endif >>> }; >>> >>> struct chip_desc { >>> @@ -863,12 +868,34 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset) >>> } >>> >>> #ifdef CONFIG_WATCHDOG_CORE >>> +static void ds1388_soft_wdt_expire(struct timer_list *soft_timer) >>> +{ >>> + struct ds1307 *ds1307 = container_of(soft_timer, struct ds1307, soft_timer); >>> + >>> + watchdog_notify_pretimeout(ds1307->wdt); >>> +} >>> + >>> +static void ds1388_soft_timer_set(struct watchdog_device *wdt_dev) >>> +{ >>> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >>> + int soft_timeout; >>> + >>> + if (wdt_dev->pretimeout > 0) { >>> + soft_timeout = wdt_dev->timeout - wdt_dev->pretimeout; >>> + mod_timer(&ds1307->soft_timer, jiffies + soft_timeout * HZ); >>> + } else { >>> + del_timer(&ds1307->soft_timer); >>> + } >>> +} >>> + >>> static int ds1388_wdt_start(struct watchdog_device *wdt_dev) >>> { >>> struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >>> u8 regs[2]; >>> int ret; >>> >>> + ds1388_soft_timer_set(wdt_dev); >>> + >>> ret = regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG, >>> DS1388_BIT_WF, 0); >>> if (ret) >>> @@ -900,6 +927,7 @@ static int ds1388_wdt_stop(struct watchdog_device *wdt_dev) >>> { >>> struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >>> >>> + del_timer(&ds1307->soft_timer); >>> return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL, >>> DS1388_BIT_WDE | DS1388_BIT_RST, 0); >>> } >>> @@ -909,6 +937,7 @@ static int ds1388_wdt_ping(struct watchdog_device *wdt_dev) >>> struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev); >>> u8 regs[2]; >>> >>> + ds1388_soft_timer_set(wdt_dev); >>> return regmap_bulk_read(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs, >>> sizeof(regs)); >>> } >>> @@ -923,6 +952,7 @@ static int ds1388_wdt_set_timeout(struct watchdog_device *wdt_dev, >>> regs[0] = 0; >>> regs[1] = bin2bcd(wdt_dev->timeout); >>> >>> + ds1388_soft_timer_set(wdt_dev); >>> return regmap_bulk_write(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs, >>> sizeof(regs)); >>> } >>> @@ -1652,7 +1682,8 @@ static void ds1307_clks_register(struct ds1307 *ds1307) >>> >>> #ifdef CONFIG_WATCHDOG_CORE >>> static const struct watchdog_info ds1388_wdt_info = { >>> - .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, >>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | >>> + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT, >>> .identity = "DS1388 watchdog", >>> }; >>> >>> @@ -1681,6 +1712,8 @@ static void ds1307_wdt_register(struct ds1307 *ds1307) >>> wdt->timeout = 99; >>> wdt->max_timeout = 99; >>> wdt->min_timeout = 1; >>> + ds1307->wdt = wdt; >>> + timer_setup(&ds1307->soft_timer, ds1388_soft_wdt_expire, 0); >>> >>> watchdog_init_timeout(wdt, 0, ds1307->dev); >>> watchdog_set_drvdata(wdt, ds1307); >>> -- >>> 2.28.0 >>> > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-04 21:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200804051743.19115-1-mark.tomlinson@alliedtelesis.co.nz>
2020-08-04 15:20 ` [PATCH] RTC: Implement pretimeout watchdog for DS1307 Alexandre Belloni
2020-08-04 16:14 ` Guenter Roeck
2020-08-04 21:38 ` Chris Packham
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).