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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>>> ---
>>> 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
>>>
>