| Message ID | 1414060846-17306-1-git-send-email-timo.kokkonen@offcode.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 10/23/14 13:40, Timo Kokkonen wrote: > By default the driver will start a kernel timer which keeps on kicking > the watchdog HW until user space has opened the watchdog > device. Usually this is desirable as the watchdog HW is running by > default and the user space may not have any watchdog daemon running at > all. > > However, on production systems it may be mandatory that also early > crashes and lockups will lead to a watchdog reset, even if they happen > before the user space has opened the watchdog device. > > To resolve the issue, add a new device tree property > "atmel,no-early-timer" which will prevent the kernel timer from > pinging the watchdog HW on behalf of user space. The default is still > to use kernel timer, but more strict behavior can be enabled via the > device tree property. > Any thoughts on this one? Should I resend this with some more people or lists to get someone to comment or review this patch? Thanks, -Timo > Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi> > --- > Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++ > drivers/watchdog/at91sam9_wdt.c | 6 +++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt > index f90e294..1b9289e 100644 > --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt > +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt > @@ -28,6 +28,10 @@ Optional properties: > entering idle state. > - atmel,dbg-halt : Should be present if you want to stop the watchdog when > entering debug state. > +- atmel,no-early-timer : Should be present if you want to let the > + watchdog timer to expire even before user space has opened the > + device. If not set, a kernel timer will keep on pinging the > + watchdog until it is opened. > > Example: > watchdog@fffffd40 { > diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c > index 489729b..8cac712 100644 > --- a/drivers/watchdog/at91sam9_wdt.c > +++ b/drivers/watchdog/at91sam9_wdt.c > @@ -89,6 +89,7 @@ struct at91wdt { > u32 mr_mask; > unsigned long heartbeat; /* WDT heartbeat in jiffies */ > bool nowayout; > + bool no_early_timer; > unsigned int irq; > }; > > @@ -122,7 +123,7 @@ static void at91_ping(unsigned long data) > { > struct at91wdt *wdt = (struct at91wdt *)data; > if (time_before(jiffies, wdt->next_heartbeat) || > - !watchdog_active(&wdt->wdd)) { > + (!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) { > at91_wdt_reset(wdt); > mod_timer(&wdt->timer, jiffies + wdt->heartbeat); > } else { > @@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt) > > wdt->mr |= max | ((max - min) << 16); > > + if (of_property_read_bool(np, "atmel,no-early-timer")) > + wdt->no_early_timer = 1; > + > return 0; > } > #else >
On 12/11/2014 09:20, Timo Kokkonen : > Hi, > > On 10/23/14 13:40, Timo Kokkonen wrote: >> By default the driver will start a kernel timer which keeps on kicking >> the watchdog HW until user space has opened the watchdog >> device. Usually this is desirable as the watchdog HW is running by >> default and the user space may not have any watchdog daemon running at >> all. >> >> However, on production systems it may be mandatory that also early >> crashes and lockups will lead to a watchdog reset, even if they happen >> before the user space has opened the watchdog device. >> >> To resolve the issue, add a new device tree property >> "atmel,no-early-timer" which will prevent the kernel timer from >> pinging the watchdog HW on behalf of user space. The default is still >> to use kernel timer, but more strict behavior can be enabled via the >> device tree property. >> > > Any thoughts on this one? Should I resend this with some more people or > lists to get someone to comment or review this patch? I would like to know if this property added to the device-tree is already defined by another watchdog timer. Indeed, it seems a pretty generic behavior and I am surprised it doesn't have an already existing implementation. ... >> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi> >> --- >> Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++ >> drivers/watchdog/at91sam9_wdt.c | 6 +++++- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt >> index f90e294..1b9289e 100644 >> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt >> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt >> @@ -28,6 +28,10 @@ Optional properties: >> entering idle state. >> - atmel,dbg-halt : Should be present if you want to stop the watchdog when >> entering debug state. >> +- atmel,no-early-timer : Should be present if you want to let the >> + watchdog timer to expire even before user space has opened the >> + device. If not set, a kernel timer will keep on pinging the >> + watchdog until it is opened. >> >> Example: >> watchdog@fffffd40 { >> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c >> index 489729b..8cac712 100644 >> --- a/drivers/watchdog/at91sam9_wdt.c >> +++ b/drivers/watchdog/at91sam9_wdt.c >> @@ -89,6 +89,7 @@ struct at91wdt { >> u32 mr_mask; >> unsigned long heartbeat; /* WDT heartbeat in jiffies */ >> bool nowayout; >> + bool no_early_timer; >> unsigned int irq; >> }; >> >> @@ -122,7 +123,7 @@ static void at91_ping(unsigned long data) >> { >> struct at91wdt *wdt = (struct at91wdt *)data; >> if (time_before(jiffies, wdt->next_heartbeat) || >> - !watchdog_active(&wdt->wdd)) { >> + (!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) { ... Nitpicking: there seems to be a indentation mismatch here: use tab + spaces to align to previous line... >> at91_wdt_reset(wdt); >> mod_timer(&wdt->timer, jiffies + wdt->heartbeat); >> } else { >> @@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt) >> >> wdt->mr |= max | ((max - min) << 16); >> >> + if (of_property_read_bool(np, "atmel,no-early-timer")) >> + wdt->no_early_timer = 1; >> + >> return 0; >> } >> #else Otherwise, it seems clean. Can you please re-send a version with the Device tree maintainer in CC? Best regards,
Hi, On 11/13/14 11:12, Nicolas Ferre wrote: > On 12/11/2014 09:20, Timo Kokkonen : >> Hi, >> >> >> Any thoughts on this one? Should I resend this with some more people or >> lists to get someone to comment or review this patch? > > I would like to know if this property added to the device-tree is > already defined by another watchdog timer. As far as I know, no. And I didn't even think about it. But after taking a quick look at Documentation/devicetree/bindings/watchdog directory, it seems there are only a few bindings that are commong among the watchdog drivers.. > Indeed, it seems a pretty generic behavior and I am surprised it doesn't > have an already existing implementation. Yeah, I think it would be a good idea to use a generic property name for it. I don't think it makes sense to make any changes to the watchdog core driver for this, it is quite hardware specific thing after all. But the policy is generic and could be supported with other drivers too. There are other products out there that have this kind of requirement already and I guess they all just hack the driver to keep the watchdog running to make sure the device gets reset if user space did not open the device for some reason. If we are about to go with a generic property name, I think it needs to be something more descriptive with respect to the desired policy. So, how about a property name "enable-early-reset". If set to 1, the watchdog HW is left running and reset will happen unless user space does something about it. If left undefined or has value 0, act just like now? How about that? -Timo > > ... > >>> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi> >>> --- >>> Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++ >>> drivers/watchdog/at91sam9_wdt.c | 6 +++++- >>> 2 files changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt >>> index f90e294..1b9289e 100644 >>> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt >>> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt >>> @@ -28,6 +28,10 @@ Optional properties: >>> entering idle state. >>> - atmel,dbg-halt : Should be present if you want to stop the watchdog when >>> entering debug state. >>> +- atmel,no-early-timer : Should be present if you want to let the >>> + watchdog timer to expire even before user space has opened the >>> + device. If not set, a kernel timer will keep on pinging the >>> + watchdog until it is opened. >>> >>> Example: >>> watchdog@fffffd40 { >>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c >>> index 489729b..8cac712 100644 >>> --- a/drivers/watchdog/at91sam9_wdt.c >>> +++ b/drivers/watchdog/at91sam9_wdt.c >>> @@ -89,6 +89,7 @@ struct at91wdt { >>> u32 mr_mask; >>> unsigned long heartbeat; /* WDT heartbeat in jiffies */ >>> bool nowayout; >>> + bool no_early_timer; >>> unsigned int irq; >>> }; >>> >>> @@ -122,7 +123,7 @@ static void at91_ping(unsigned long data) >>> { >>> struct at91wdt *wdt = (struct at91wdt *)data; >>> if (time_before(jiffies, wdt->next_heartbeat) || >>> - !watchdog_active(&wdt->wdd)) { >>> + (!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) { > > ... Nitpicking: there seems to be a indentation mismatch here: use tab + > spaces to align to previous line... > >>> at91_wdt_reset(wdt); >>> mod_timer(&wdt->timer, jiffies + wdt->heartbeat); >>> } else { >>> @@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt) >>> >>> wdt->mr |= max | ((max - min) << 16); >>> >>> + if (of_property_read_bool(np, "atmel,no-early-timer")) >>> + wdt->no_early_timer = 1; >>> + >>> return 0; >>> } >>> #else > > Otherwise, it seems clean. Can you please re-send a version with the > Device tree maintainer in CC? > > Best regards, >
diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt index f90e294..1b9289e 100644 --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt @@ -28,6 +28,10 @@ Optional properties: entering idle state. - atmel,dbg-halt : Should be present if you want to stop the watchdog when entering debug state. +- atmel,no-early-timer : Should be present if you want to let the + watchdog timer to expire even before user space has opened the + device. If not set, a kernel timer will keep on pinging the + watchdog until it is opened. Example: watchdog@fffffd40 { diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c index 489729b..8cac712 100644 --- a/drivers/watchdog/at91sam9_wdt.c +++ b/drivers/watchdog/at91sam9_wdt.c @@ -89,6 +89,7 @@ struct at91wdt { u32 mr_mask; unsigned long heartbeat; /* WDT heartbeat in jiffies */ bool nowayout; + bool no_early_timer; unsigned int irq; }; @@ -122,7 +123,7 @@ static void at91_ping(unsigned long data) { struct at91wdt *wdt = (struct at91wdt *)data; if (time_before(jiffies, wdt->next_heartbeat) || - !watchdog_active(&wdt->wdd)) { + (!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) { at91_wdt_reset(wdt); mod_timer(&wdt->timer, jiffies + wdt->heartbeat); } else { @@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt) wdt->mr |= max | ((max - min) << 16); + if (of_property_read_bool(np, "atmel,no-early-timer")) + wdt->no_early_timer = 1; + return 0; } #else
By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all. However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device. To resolve the issue, add a new device tree property "atmel,no-early-timer" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property. Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi> --- Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++ drivers/watchdog/at91sam9_wdt.c | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-)