| Message ID | 20200804051743.19115-1-mark.tomlinson@alliedtelesis.co.nz |
|---|---|
| State | Rejected |
| Headers | show |
| Series | RTC: Implement pretimeout watchdog for DS1307 | expand |
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 >
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 >> >
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 >>> >
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);
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(-)