* [PATCHv8 00/10] watchdog: Extend kernel API and add early_timeout_sec feature
@ 2015-05-19 8:25 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:25 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
The watchdog kernel API is quite limited. It has support for providing
generic device handling, but it doesn't really know anything about the
watchdog hardware or its constraints. The watchdog drivers come with a
lot of diversity and their own set of quirks and constraints. Some of
their limitations are not nice for the user space, so the drivers work
around them with all sorts of ad hoc implementations.
One common pattern is to use kernel timers or work queues to allow
longer timeout parameters than the actual hardware supports. To solve
this problem, this patch set extends the kernel watchdog API with a
few parameters that let the core know more about the watchdog HW and
take care about the timeout extending. This also lets core emulate
unstoppable hardware by pinging the watchdog from the worker while the
watchdog is expected to be stopped.
A generic watchdog policy is also introduced in order to choose what
to do with the watchdog on boot up. It can be stopped, like before, or
it can be started or left untouched. Starting a watchdog on the boot
up may require that kernel keeps the watchdog alive for some time
until the user space takes over it. For this "early timeout" value is
introduced. It can be given a default value from Kconfig or
"early_timeout_sec" device tree property can be used to override the
built in default.
The changes in this series are designed to be taken in use one driver
at time. Old drivers continue to work exactly the same as before.
This patch set converts at91sam9_wdt, imx2_wdt and omap_wdt to use the
new API extensions. The patches have been tested on three ARM boards:
ADG42 (at91 sama5d3), beaglebone and wandboard dual. These three
different hardware tests fully configurable watchdog (omap_wdt),
configurable but unstoppable watcdog (imx2_wdt) and fully unstoppable
and runtime unconfigurable watchdog (at91sama5d3_wdt). The combination
of these three watchdogs gives fairly good coverage of possible use
cases.
Please review and give feedback.
Patch revision history:
-v8: hw_heartbeat and hw_max_timeout values are converted in msec
resolution instead of jiffies. watchdog_active() function is renamed
to watchdog_hw_active() in order to state more explicitly what it is
testing. This series now talks more about how it implements new
watchdog policy instead of talking about early-timeout-sec property,
which not the whole story of it. The default policy can also be
selected with Kconfig option. Watchdog documentation is also updated
in Documentation directory and in watchdog.h header file.
-v7: Convert also omap_wdt and imx2_wdt drivers in addition to
at91sam9_wdt to use the new core API. This allowed me to test a lot
more use cases and find many bugs I had left in previous
version. The max_timeout parameter is no longer needed for new
drivers as watchdog core can extend the timeout and support
unlimited timeout values on behalf of the HW.
-v6: Fixed some issued based on feedback from Wenyou Yang. The logic
in watchdog_worker() function is now significantly easier to
read. Several errors with stopping and starting the worker are also
now fixed.
-v5: Re-think the approach to be fully generic. The early_timeout_sec
handling is no longer in the driver but in the watchdog core. As a
result the core needed to gain knowledge about the watchdog
hardware. Appropriate handling is added in the core. The side effect
for this is that drivers using the new extensions can be simplified
a lot and different kinds of watchdog hardware can be made to
behave the same for the user space.
-v4: Binding documentation is now separated completely from the driver
patch. The documentation no longer makes any assumptions about how
the actual implementation is made, it just describes the actual
behavior the driver should implement in order to satisfy the
requirement.
- v3: Rename the property to "early-timeout-sec" and use it as a
timeout value that stops the timer in the atmel driver after the
timeout expires. A watchdog.txt is also introduced for documenting
the common watchdog properties, including now this one and
"timeout-sec" property.
- v2: Rename the property to "enable-early-reset" as the behavior
itself is not atmel specific. This way other drivers are free to
implement same behavior with the same property name.
- v1: Propose property name "atmle,no-early-timer" for disabling the
timer that keeps the atmel watchdog running until user space opens
the device.
Timo Kokkonen (10):
watchdog: Rename watchdog_active to watchdog_hw_active
watchdog: core: Ping watchdog on behalf of user space when needed
watchdog: core: Introduce default watchdog policy
watchdog: core: Allow extending early timeout
Documentation/watchdog: Document watchdog core API changes
devicetree: Document generic watchdog properties
Documentation/watchdog: watchdog-test.c: Add support for changing
timeout
watchdog: at91sam9_wdt: Convert to use new watchdog core extensions
watchdog: imx2_wdt: Convert to use new core extensions
watchdog: omap_wdt: Convert to use new core extensions
.../devicetree/bindings/watchdog/watchdog.txt | 20 +++
.../watchdog/convert_drivers_to_kernel_api.txt | 26 ++++
Documentation/watchdog/src/watchdog-test.c | 6 +
Documentation/watchdog/watchdog-kernel-api.txt | 71 +++++----
drivers/watchdog/Kconfig | 72 +++++++++
drivers/watchdog/at91sam9_wdt.c | 73 ++-------
drivers/watchdog/iTCO_wdt.c | 2 +-
drivers/watchdog/imx2_wdt.c | 51 ++-----
drivers/watchdog/omap_wdt.c | 26 +++-
drivers/watchdog/sp805_wdt.c | 4 +-
drivers/watchdog/stmp3xxx_rtc_wdt.c | 4 +-
drivers/watchdog/tegra_wdt.c | 6 +-
drivers/watchdog/twl4030_wdt.c | 4 +-
drivers/watchdog/ux500_wdt.c | 4 +-
drivers/watchdog/via_wdt.c | 2 +-
drivers/watchdog/watchdog_core.c | 170 ++++++++++++++++++++-
drivers/watchdog/watchdog_dev.c | 116 +++++++++++---
include/linux/watchdog.h | 69 ++++++++-
18 files changed, 554 insertions(+), 172 deletions(-)
create mode 100644 Documentation/devicetree/bindings/watchdog/watchdog.txt
--
2.1.0
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 00/10] watchdog: Extend kernel API and add early_timeout_sec feature
@ 2015-05-19 8:25 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:25 UTC (permalink / raw)
To: linux-arm-kernel
The watchdog kernel API is quite limited. It has support for providing
generic device handling, but it doesn't really know anything about the
watchdog hardware or its constraints. The watchdog drivers come with a
lot of diversity and their own set of quirks and constraints. Some of
their limitations are not nice for the user space, so the drivers work
around them with all sorts of ad hoc implementations.
One common pattern is to use kernel timers or work queues to allow
longer timeout parameters than the actual hardware supports. To solve
this problem, this patch set extends the kernel watchdog API with a
few parameters that let the core know more about the watchdog HW and
take care about the timeout extending. This also lets core emulate
unstoppable hardware by pinging the watchdog from the worker while the
watchdog is expected to be stopped.
A generic watchdog policy is also introduced in order to choose what
to do with the watchdog on boot up. It can be stopped, like before, or
it can be started or left untouched. Starting a watchdog on the boot
up may require that kernel keeps the watchdog alive for some time
until the user space takes over it. For this "early timeout" value is
introduced. It can be given a default value from Kconfig or
"early_timeout_sec" device tree property can be used to override the
built in default.
The changes in this series are designed to be taken in use one driver
at time. Old drivers continue to work exactly the same as before.
This patch set converts at91sam9_wdt, imx2_wdt and omap_wdt to use the
new API extensions. The patches have been tested on three ARM boards:
ADG42 (at91 sama5d3), beaglebone and wandboard dual. These three
different hardware tests fully configurable watchdog (omap_wdt),
configurable but unstoppable watcdog (imx2_wdt) and fully unstoppable
and runtime unconfigurable watchdog (at91sama5d3_wdt). The combination
of these three watchdogs gives fairly good coverage of possible use
cases.
Please review and give feedback.
Patch revision history:
-v8: hw_heartbeat and hw_max_timeout values are converted in msec
resolution instead of jiffies. watchdog_active() function is renamed
to watchdog_hw_active() in order to state more explicitly what it is
testing. This series now talks more about how it implements new
watchdog policy instead of talking about early-timeout-sec property,
which not the whole story of it. The default policy can also be
selected with Kconfig option. Watchdog documentation is also updated
in Documentation directory and in watchdog.h header file.
-v7: Convert also omap_wdt and imx2_wdt drivers in addition to
at91sam9_wdt to use the new core API. This allowed me to test a lot
more use cases and find many bugs I had left in previous
version. The max_timeout parameter is no longer needed for new
drivers as watchdog core can extend the timeout and support
unlimited timeout values on behalf of the HW.
-v6: Fixed some issued based on feedback from Wenyou Yang. The logic
in watchdog_worker() function is now significantly easier to
read. Several errors with stopping and starting the worker are also
now fixed.
-v5: Re-think the approach to be fully generic. The early_timeout_sec
handling is no longer in the driver but in the watchdog core. As a
result the core needed to gain knowledge about the watchdog
hardware. Appropriate handling is added in the core. The side effect
for this is that drivers using the new extensions can be simplified
a lot and different kinds of watchdog hardware can be made to
behave the same for the user space.
-v4: Binding documentation is now separated completely from the driver
patch. The documentation no longer makes any assumptions about how
the actual implementation is made, it just describes the actual
behavior the driver should implement in order to satisfy the
requirement.
- v3: Rename the property to "early-timeout-sec" and use it as a
timeout value that stops the timer in the atmel driver after the
timeout expires. A watchdog.txt is also introduced for documenting
the common watchdog properties, including now this one and
"timeout-sec" property.
- v2: Rename the property to "enable-early-reset" as the behavior
itself is not atmel specific. This way other drivers are free to
implement same behavior with the same property name.
- v1: Propose property name "atmle,no-early-timer" for disabling the
timer that keeps the atmel watchdog running until user space opens
the device.
Timo Kokkonen (10):
watchdog: Rename watchdog_active to watchdog_hw_active
watchdog: core: Ping watchdog on behalf of user space when needed
watchdog: core: Introduce default watchdog policy
watchdog: core: Allow extending early timeout
Documentation/watchdog: Document watchdog core API changes
devicetree: Document generic watchdog properties
Documentation/watchdog: watchdog-test.c: Add support for changing
timeout
watchdog: at91sam9_wdt: Convert to use new watchdog core extensions
watchdog: imx2_wdt: Convert to use new core extensions
watchdog: omap_wdt: Convert to use new core extensions
.../devicetree/bindings/watchdog/watchdog.txt | 20 +++
.../watchdog/convert_drivers_to_kernel_api.txt | 26 ++++
Documentation/watchdog/src/watchdog-test.c | 6 +
Documentation/watchdog/watchdog-kernel-api.txt | 71 +++++----
drivers/watchdog/Kconfig | 72 +++++++++
drivers/watchdog/at91sam9_wdt.c | 73 ++-------
drivers/watchdog/iTCO_wdt.c | 2 +-
drivers/watchdog/imx2_wdt.c | 51 ++-----
drivers/watchdog/omap_wdt.c | 26 +++-
drivers/watchdog/sp805_wdt.c | 4 +-
drivers/watchdog/stmp3xxx_rtc_wdt.c | 4 +-
drivers/watchdog/tegra_wdt.c | 6 +-
drivers/watchdog/twl4030_wdt.c | 4 +-
drivers/watchdog/ux500_wdt.c | 4 +-
drivers/watchdog/via_wdt.c | 2 +-
drivers/watchdog/watchdog_core.c | 170 ++++++++++++++++++++-
drivers/watchdog/watchdog_dev.c | 116 +++++++++++---
include/linux/watchdog.h | 69 ++++++++-
18 files changed, 554 insertions(+), 172 deletions(-)
create mode 100644 Documentation/devicetree/bindings/watchdog/watchdog.txt
--
2.1.0
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
Before extending the watchdog core midlayer, it is useful to rename
the watchdog_active function so that it states explicitly what it
really does. That is, "active" watchdog means really that the watchdog
hardware is running and needs pinging to prevent a watchdog reset
taking place in near future.
This is different to "watchdog open" state, which simply states that
kernel is expecting the user space to keep the watchdog alive. These
states might become different mainly because some hardware have
limitations that prevent them from being stopped at will.
The actual change to the code was made with the following script:
find -name *.c | xargs sed -i -e s.watchdog_active.watchdog_hw_active.g
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/at91sam9_wdt.c | 2 +-
drivers/watchdog/iTCO_wdt.c | 2 +-
drivers/watchdog/imx2_wdt.c | 6 +++---
drivers/watchdog/sp805_wdt.c | 4 ++--
drivers/watchdog/stmp3xxx_rtc_wdt.c | 4 ++--
drivers/watchdog/tegra_wdt.c | 6 +++---
drivers/watchdog/twl4030_wdt.c | 4 ++--
drivers/watchdog/ux500_wdt.c | 4 ++--
drivers/watchdog/via_wdt.c | 2 +-
drivers/watchdog/watchdog_dev.c | 6 +++---
include/linux/watchdog.h | 4 ++--
11 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 1443b3c..411b7a6 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -122,7 +122,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_hw_active(&wdt->wdd)) {
at91_wdt_reset(wdt);
mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
} else {
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3c3fd41..75d7534 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -595,7 +595,7 @@ static int iTCO_wdt_suspend_noirq(struct device *dev)
int ret = 0;
iTCO_wdt_private.suspended = false;
- if (watchdog_active(&iTCO_wdt_watchdog_dev) && need_suspend()) {
+ if (watchdog_hw_active(&iTCO_wdt_watchdog_dev) && need_suspend()) {
ret = iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
if (!ret)
iTCO_wdt_private.suspended = true;
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 5e6d808..48fcce8 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -348,7 +348,7 @@ static int imx2_wdt_suspend(struct device *dev)
imx2_wdt_ping(wdog);
/* The watchdog is not active */
- if (!watchdog_active(wdog))
+ if (!watchdog_hw_active(wdog))
del_timer_sync(&wdev->timer);
}
@@ -365,7 +365,7 @@ static int imx2_wdt_resume(struct device *dev)
clk_prepare_enable(wdev->clk);
- if (watchdog_active(wdog) && !imx2_wdt_is_running(wdev)) {
+ if (watchdog_hw_active(wdog) && !imx2_wdt_is_running(wdev)) {
/*
* If the watchdog is still active and resumes
* from deep sleep state, need to restart the
@@ -382,7 +382,7 @@ static int imx2_wdt_resume(struct device *dev)
* But the watchdog is not active, then start
* the timer again.
*/
- if (!watchdog_active(wdog))
+ if (!watchdog_hw_active(wdog))
mod_timer(&wdev->timer,
jiffies + wdog->timeout * HZ / 2);
}
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index c1b03f4..0b0f1c406 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -262,7 +262,7 @@ static int __maybe_unused sp805_wdt_suspend(struct device *dev)
{
struct sp805_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
return wdt_disable(&wdt->wdd);
return 0;
@@ -272,7 +272,7 @@ static int __maybe_unused sp805_wdt_resume(struct device *dev)
{
struct sp805_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
return wdt_enable(&wdt->wdd);
return 0;
diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
index e7f0d5b..1137262 100644
--- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
+++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
@@ -98,7 +98,7 @@ static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev)
{
struct watchdog_device *wdd = &stmp3xxx_wdd;
- if (watchdog_active(wdd))
+ if (watchdog_hw_active(wdd))
return wdt_stop(wdd);
return 0;
@@ -108,7 +108,7 @@ static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev)
{
struct watchdog_device *wdd = &stmp3xxx_wdd;
- if (watchdog_active(wdd))
+ if (watchdog_hw_active(wdd))
return wdt_start(wdd);
return 0;
diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
index 30451ea..40d4691 100644
--- a/drivers/watchdog/tegra_wdt.c
+++ b/drivers/watchdog/tegra_wdt.c
@@ -140,7 +140,7 @@ static int tegra_wdt_set_timeout(struct watchdog_device *wdd,
{
wdd->timeout = timeout;
- if (watchdog_active(wdd))
+ if (watchdog_hw_active(wdd))
return tegra_wdt_start(wdd);
return 0;
@@ -257,7 +257,7 @@ static int tegra_wdt_runtime_suspend(struct device *dev)
{
struct tegra_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
tegra_wdt_stop(&wdt->wdd);
return 0;
@@ -267,7 +267,7 @@ static int tegra_wdt_runtime_resume(struct device *dev)
{
struct tegra_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
tegra_wdt_start(&wdt->wdd);
return 0;
diff --git a/drivers/watchdog/twl4030_wdt.c b/drivers/watchdog/twl4030_wdt.c
index 2c1db6f..118b361 100644
--- a/drivers/watchdog/twl4030_wdt.c
+++ b/drivers/watchdog/twl4030_wdt.c
@@ -109,7 +109,7 @@ static int twl4030_wdt_remove(struct platform_device *pdev)
static int twl4030_wdt_suspend(struct platform_device *pdev, pm_message_t state)
{
struct watchdog_device *wdt = platform_get_drvdata(pdev);
- if (watchdog_active(wdt))
+ if (watchdog_hw_active(wdt))
return twl4030_wdt_stop(wdt);
return 0;
@@ -118,7 +118,7 @@ static int twl4030_wdt_suspend(struct platform_device *pdev, pm_message_t state)
static int twl4030_wdt_resume(struct platform_device *pdev)
{
struct watchdog_device *wdt = platform_get_drvdata(pdev);
- if (watchdog_active(wdt))
+ if (watchdog_hw_active(wdt))
return twl4030_wdt_start(wdt);
return 0;
diff --git a/drivers/watchdog/ux500_wdt.c b/drivers/watchdog/ux500_wdt.c
index 9de09ab..6349bfa 100644
--- a/drivers/watchdog/ux500_wdt.c
+++ b/drivers/watchdog/ux500_wdt.c
@@ -124,7 +124,7 @@ static int ux500_wdt_remove(struct platform_device *dev)
static int ux500_wdt_suspend(struct platform_device *pdev,
pm_message_t state)
{
- if (watchdog_active(&ux500_wdt)) {
+ if (watchdog_hw_active(&ux500_wdt)) {
ux500_wdt_stop(&ux500_wdt);
prcmu_config_a9wdog(PRCMU_WDOG_CPU1, true);
@@ -136,7 +136,7 @@ static int ux500_wdt_suspend(struct platform_device *pdev,
static int ux500_wdt_resume(struct platform_device *pdev)
{
- if (watchdog_active(&ux500_wdt)) {
+ if (watchdog_hw_active(&ux500_wdt)) {
ux500_wdt_stop(&ux500_wdt);
prcmu_config_a9wdog(PRCMU_WDOG_CPU1, false);
diff --git a/drivers/watchdog/via_wdt.c b/drivers/watchdog/via_wdt.c
index 56369c4..b1c095c 100644
--- a/drivers/watchdog/via_wdt.c
+++ b/drivers/watchdog/via_wdt.c
@@ -91,7 +91,7 @@ static inline void wdt_reset(void)
static void wdt_timer_tick(unsigned long data)
{
if (time_before(jiffies, next_heartbeat) ||
- (!watchdog_active(&wdt_dev))) {
+ (!watchdog_hw_active(&wdt_dev))) {
wdt_reset();
mod_timer(&timer, jiffies + WDT_HEARTBEAT);
} else
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6aaefba..2c4d3f1 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -70,7 +70,7 @@ static int watchdog_ping(struct watchdog_device *wddev)
goto out_ping;
}
- if (!watchdog_active(wddev))
+ if (!watchdog_hw_active(wddev))
goto out_ping;
if (wddev->ops->ping)
@@ -103,7 +103,7 @@ static int watchdog_start(struct watchdog_device *wddev)
goto out_start;
}
- if (watchdog_active(wddev))
+ if (watchdog_hw_active(wddev))
goto out_start;
err = wddev->ops->start(wddev);
@@ -136,7 +136,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
goto out_stop;
}
- if (!watchdog_active(wddev))
+ if (!watchdog_hw_active(wddev))
goto out_stop;
if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index a746bf5..5b84578 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -90,7 +90,7 @@ struct watchdog_device {
struct mutex lock;
unsigned long status;
/* Bit numbers for status flags */
-#define WDOG_ACTIVE 0 /* Is the watchdog running/active */
+#define WDOG_ACTIVE 0 /* Is the watchdog hw running/active */
#define WDOG_DEV_OPEN 1 /* Opened via /dev/watchdog ? */
#define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
#define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
@@ -101,7 +101,7 @@ struct watchdog_device {
#define WATCHDOG_NOWAYOUT_INIT_STATUS (WATCHDOG_NOWAYOUT << WDOG_NO_WAY_OUT)
/* Use the following function to check whether or not the watchdog is active */
-static inline bool watchdog_active(struct watchdog_device *wdd)
+static inline bool watchdog_hw_active(struct watchdog_device *wdd)
{
return test_bit(WDOG_ACTIVE, &wdd->status);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Before extending the watchdog core midlayer, it is useful to rename
the watchdog_active function so that it states explicitly what it
really does. That is, "active" watchdog means really that the watchdog
hardware is running and needs pinging to prevent a watchdog reset
taking place in near future.
This is different to "watchdog open" state, which simply states that
kernel is expecting the user space to keep the watchdog alive. These
states might become different mainly because some hardware have
limitations that prevent them from being stopped at will.
The actual change to the code was made with the following script:
find -name *.c | xargs sed -i -e s.watchdog_active.watchdog_hw_active.g
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/at91sam9_wdt.c | 2 +-
drivers/watchdog/iTCO_wdt.c | 2 +-
drivers/watchdog/imx2_wdt.c | 6 +++---
drivers/watchdog/sp805_wdt.c | 4 ++--
drivers/watchdog/stmp3xxx_rtc_wdt.c | 4 ++--
drivers/watchdog/tegra_wdt.c | 6 +++---
drivers/watchdog/twl4030_wdt.c | 4 ++--
drivers/watchdog/ux500_wdt.c | 4 ++--
drivers/watchdog/via_wdt.c | 2 +-
drivers/watchdog/watchdog_dev.c | 6 +++---
include/linux/watchdog.h | 4 ++--
11 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 1443b3c..411b7a6 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -122,7 +122,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_hw_active(&wdt->wdd)) {
at91_wdt_reset(wdt);
mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
} else {
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3c3fd41..75d7534 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -595,7 +595,7 @@ static int iTCO_wdt_suspend_noirq(struct device *dev)
int ret = 0;
iTCO_wdt_private.suspended = false;
- if (watchdog_active(&iTCO_wdt_watchdog_dev) && need_suspend()) {
+ if (watchdog_hw_active(&iTCO_wdt_watchdog_dev) && need_suspend()) {
ret = iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
if (!ret)
iTCO_wdt_private.suspended = true;
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 5e6d808..48fcce8 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -348,7 +348,7 @@ static int imx2_wdt_suspend(struct device *dev)
imx2_wdt_ping(wdog);
/* The watchdog is not active */
- if (!watchdog_active(wdog))
+ if (!watchdog_hw_active(wdog))
del_timer_sync(&wdev->timer);
}
@@ -365,7 +365,7 @@ static int imx2_wdt_resume(struct device *dev)
clk_prepare_enable(wdev->clk);
- if (watchdog_active(wdog) && !imx2_wdt_is_running(wdev)) {
+ if (watchdog_hw_active(wdog) && !imx2_wdt_is_running(wdev)) {
/*
* If the watchdog is still active and resumes
* from deep sleep state, need to restart the
@@ -382,7 +382,7 @@ static int imx2_wdt_resume(struct device *dev)
* But the watchdog is not active, then start
* the timer again.
*/
- if (!watchdog_active(wdog))
+ if (!watchdog_hw_active(wdog))
mod_timer(&wdev->timer,
jiffies + wdog->timeout * HZ / 2);
}
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index c1b03f4..0b0f1c406 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -262,7 +262,7 @@ static int __maybe_unused sp805_wdt_suspend(struct device *dev)
{
struct sp805_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
return wdt_disable(&wdt->wdd);
return 0;
@@ -272,7 +272,7 @@ static int __maybe_unused sp805_wdt_resume(struct device *dev)
{
struct sp805_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
return wdt_enable(&wdt->wdd);
return 0;
diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
index e7f0d5b..1137262 100644
--- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
+++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
@@ -98,7 +98,7 @@ static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev)
{
struct watchdog_device *wdd = &stmp3xxx_wdd;
- if (watchdog_active(wdd))
+ if (watchdog_hw_active(wdd))
return wdt_stop(wdd);
return 0;
@@ -108,7 +108,7 @@ static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev)
{
struct watchdog_device *wdd = &stmp3xxx_wdd;
- if (watchdog_active(wdd))
+ if (watchdog_hw_active(wdd))
return wdt_start(wdd);
return 0;
diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
index 30451ea..40d4691 100644
--- a/drivers/watchdog/tegra_wdt.c
+++ b/drivers/watchdog/tegra_wdt.c
@@ -140,7 +140,7 @@ static int tegra_wdt_set_timeout(struct watchdog_device *wdd,
{
wdd->timeout = timeout;
- if (watchdog_active(wdd))
+ if (watchdog_hw_active(wdd))
return tegra_wdt_start(wdd);
return 0;
@@ -257,7 +257,7 @@ static int tegra_wdt_runtime_suspend(struct device *dev)
{
struct tegra_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
tegra_wdt_stop(&wdt->wdd);
return 0;
@@ -267,7 +267,7 @@ static int tegra_wdt_runtime_resume(struct device *dev)
{
struct tegra_wdt *wdt = dev_get_drvdata(dev);
- if (watchdog_active(&wdt->wdd))
+ if (watchdog_hw_active(&wdt->wdd))
tegra_wdt_start(&wdt->wdd);
return 0;
diff --git a/drivers/watchdog/twl4030_wdt.c b/drivers/watchdog/twl4030_wdt.c
index 2c1db6f..118b361 100644
--- a/drivers/watchdog/twl4030_wdt.c
+++ b/drivers/watchdog/twl4030_wdt.c
@@ -109,7 +109,7 @@ static int twl4030_wdt_remove(struct platform_device *pdev)
static int twl4030_wdt_suspend(struct platform_device *pdev, pm_message_t state)
{
struct watchdog_device *wdt = platform_get_drvdata(pdev);
- if (watchdog_active(wdt))
+ if (watchdog_hw_active(wdt))
return twl4030_wdt_stop(wdt);
return 0;
@@ -118,7 +118,7 @@ static int twl4030_wdt_suspend(struct platform_device *pdev, pm_message_t state)
static int twl4030_wdt_resume(struct platform_device *pdev)
{
struct watchdog_device *wdt = platform_get_drvdata(pdev);
- if (watchdog_active(wdt))
+ if (watchdog_hw_active(wdt))
return twl4030_wdt_start(wdt);
return 0;
diff --git a/drivers/watchdog/ux500_wdt.c b/drivers/watchdog/ux500_wdt.c
index 9de09ab..6349bfa 100644
--- a/drivers/watchdog/ux500_wdt.c
+++ b/drivers/watchdog/ux500_wdt.c
@@ -124,7 +124,7 @@ static int ux500_wdt_remove(struct platform_device *dev)
static int ux500_wdt_suspend(struct platform_device *pdev,
pm_message_t state)
{
- if (watchdog_active(&ux500_wdt)) {
+ if (watchdog_hw_active(&ux500_wdt)) {
ux500_wdt_stop(&ux500_wdt);
prcmu_config_a9wdog(PRCMU_WDOG_CPU1, true);
@@ -136,7 +136,7 @@ static int ux500_wdt_suspend(struct platform_device *pdev,
static int ux500_wdt_resume(struct platform_device *pdev)
{
- if (watchdog_active(&ux500_wdt)) {
+ if (watchdog_hw_active(&ux500_wdt)) {
ux500_wdt_stop(&ux500_wdt);
prcmu_config_a9wdog(PRCMU_WDOG_CPU1, false);
diff --git a/drivers/watchdog/via_wdt.c b/drivers/watchdog/via_wdt.c
index 56369c4..b1c095c 100644
--- a/drivers/watchdog/via_wdt.c
+++ b/drivers/watchdog/via_wdt.c
@@ -91,7 +91,7 @@ static inline void wdt_reset(void)
static void wdt_timer_tick(unsigned long data)
{
if (time_before(jiffies, next_heartbeat) ||
- (!watchdog_active(&wdt_dev))) {
+ (!watchdog_hw_active(&wdt_dev))) {
wdt_reset();
mod_timer(&timer, jiffies + WDT_HEARTBEAT);
} else
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6aaefba..2c4d3f1 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -70,7 +70,7 @@ static int watchdog_ping(struct watchdog_device *wddev)
goto out_ping;
}
- if (!watchdog_active(wddev))
+ if (!watchdog_hw_active(wddev))
goto out_ping;
if (wddev->ops->ping)
@@ -103,7 +103,7 @@ static int watchdog_start(struct watchdog_device *wddev)
goto out_start;
}
- if (watchdog_active(wddev))
+ if (watchdog_hw_active(wddev))
goto out_start;
err = wddev->ops->start(wddev);
@@ -136,7 +136,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
goto out_stop;
}
- if (!watchdog_active(wddev))
+ if (!watchdog_hw_active(wddev))
goto out_stop;
if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index a746bf5..5b84578 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -90,7 +90,7 @@ struct watchdog_device {
struct mutex lock;
unsigned long status;
/* Bit numbers for status flags */
-#define WDOG_ACTIVE 0 /* Is the watchdog running/active */
+#define WDOG_ACTIVE 0 /* Is the watchdog hw running/active */
#define WDOG_DEV_OPEN 1 /* Opened via /dev/watchdog ? */
#define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
#define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
@@ -101,7 +101,7 @@ struct watchdog_device {
#define WATCHDOG_NOWAYOUT_INIT_STATUS (WATCHDOG_NOWAYOUT << WDOG_NO_WAY_OUT)
/* Use the following function to check whether or not the watchdog is active */
-static inline bool watchdog_active(struct watchdog_device *wdd)
+static inline bool watchdog_hw_active(struct watchdog_device *wdd)
{
return test_bit(WDOG_ACTIVE, &wdd->status);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 02/10] watchdog: core: Ping watchdog on behalf of user space when needed
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
Many watchdogs are not stoppable on the hardware level at all. Some
others have a very short maximum timeout value. Both of these limits
are problematic to the userspace and watchdog drivers have been
traditionally implementing workarounds of their own in order to
overcome the limitations. This obviously results in duplicate code in
the driver level and makes it harder to implement generic hardware
independent features.
Extend the watchdog core by allowing it do the most common tasks on
behalf of the driver. For this to work the watchdog core needs two new
values from the driver, hw_max_timeout and hw_heartbeat. The first one
tells the core what is the maximum supported timeout value in the
hardware and the second one tells the preferred heartbeat value for
the hardware. Both values are in milliseconds.
The driver needs to also call watchdog_init_params() in order to let
watchdog core fill in default watchdog paramters and let the core
check the validity of the values.
The watchdog core api changes slightly because of this change. Most
importantly, the watchdog core now stands out as a clear midlayer
between the driver and the user space. That is, the hw_max_timeout and
hw_heartbeat values are meant to be filled in by the driver for the
core. They will not be visible to user space any way. The timeout
variable however is part of user space API. The comments in watchdog.h
are changed also to reflect more clearly the difference. The
max_timeout also becomes obsolete as the worker can support arbitrary
timeout values.
As long as we have still old drivers that don't tell the core about
their hw capabilities, we need to support the old driver handling
too. Add watchdog_needs_legacy_handling() function to watchdog.h so we
can implement easily the old driver behavior and it becomes clear from
the code which parts can be removed once all existing drivers supply
the new parameters to watchdog core.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/watchdog_core.c | 124 ++++++++++++++++++++++++++++++++++++++-
drivers/watchdog/watchdog_dev.c | 105 ++++++++++++++++++++++++++-------
include/linux/watchdog.h | 64 ++++++++++++++++++--
3 files changed, 265 insertions(+), 28 deletions(-)
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index cec9b55..16e10e0 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -45,6 +45,9 @@ static struct class *watchdog_class;
static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
{
+ /* Newer drivers don't need this check any more */
+ if (!watchdog_needs_legacy_handling(wdd))
+ return;
/*
* Check that we have valid min and max timeout values, if
* not reset them both to 0 (=not used or unknown)
@@ -98,6 +101,110 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
}
EXPORT_SYMBOL_GPL(watchdog_init_timeout);
+static void watchdog_set_default_timeout(struct watchdog_device *wdd,
+ struct device *dev)
+{
+ int ret;
+ /*
+ * If driver has already set up a timeout, eg. from a module
+ * parameter, no need to do anything here
+ */
+ if (!watchdog_timeout_invalid(wdd, wdd->timeout))
+ return;
+
+ /* Query device tree */
+ if (dev && dev->of_node) {
+ ret = of_property_read_u32(dev->of_node, "timeout-sec",
+ &wdd->timeout);
+ if (!ret && !watchdog_timeout_invalid(wdd, wdd->timeout))
+ return;
+ }
+
+ /*
+ * If no other preference is given, use 60 seconds as the
+ * default timeout value
+ */
+ wdd->timeout = 60;
+}
+
+/**
+ * watchdog_init_parms() - initialize generic watchdog parameters
+ * @wdd: Watchdog device to operate
+ * @dev: Device that stores the device tree properties
+ *
+ * Initialize the generic watchdog parameters. At least hw_max_timeout
+ * must be set prior calling this function. Other unset parameters are
+ * set based on information gathered from different sources (kernel
+ * config, device tree, ...) or set up with a reasonable defaults.
+ *
+ * A zero is returned on success and -EINVAL for failure.
+ */
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
+{
+ int ret = 0;
+
+ watchdog_set_default_timeout(wdd, dev);
+
+ if (wdd->max_timeout)
+ dev_err(dev, "Driver setting deprecated max_timeout, please fix\n");
+
+ if (!wdd->hw_max_timeout)
+ return -EINVAL;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(watchdog_init_params);
+
+static void watchdog_worker(struct work_struct *work)
+{
+ struct watchdog_device *wdd = container_of(to_delayed_work(work),
+ struct watchdog_device, work);
+ bool stopped_keepalive;
+ bool extend_keepalive;
+
+ mutex_lock(&wdd->lock);
+
+ /* Watchdog device is not open but HW does not support stopping */
+ stopped_keepalive = watchdog_hw_active(wdd) &&
+ !watchdog_dev_open(wdd);
+
+ /* Keepalive time is longer than what hardware is capable */
+ extend_keepalive = watchdog_dev_open(wdd) &&
+ msecs_to_jiffies(wdd->hw_max_timeout) < wdd->timeout * HZ;
+
+ if (time_after(jiffies, wdd->expires) && watchdog_dev_open(wdd)) {
+ /*
+ * Userspace has exceeded its timeout, the watchdog is
+ * going to trigger soon.
+ */
+ mutex_unlock(&wdd->lock);
+ return;
+ }
+
+ if (stopped_keepalive || extend_keepalive) {
+ unsigned long expires = msecs_to_jiffies(wdd->hw_heartbeat);
+
+ _watchdog_ping(wdd);
+ schedule_delayed_work(&wdd->work, expires);
+ }
+
+ mutex_unlock(&wdd->lock);
+}
+
+static int prepare_watchdog(struct watchdog_device *wdd)
+{
+ if (watchdog_needs_legacy_handling(wdd)) {
+ pr_info("Incomplete watchdog driver implementation, please report or fix\n");
+ return 0;
+ }
+
+ if (!watchdog_hw_active(wdd))
+ return 0;
+
+ /* Stop the watchdog now until user space opens the device */
+ return watchdog_stop(wdd);
+}
+
/**
* watchdog_register_device() - register a watchdog device
* @wdd: watchdog device
@@ -156,13 +263,23 @@ int watchdog_register_device(struct watchdog_device *wdd)
wdd->dev = device_create(watchdog_class, wdd->parent, devno,
NULL, "watchdog%d", wdd->id);
if (IS_ERR(wdd->dev)) {
- watchdog_dev_unregister(wdd);
- ida_simple_remove(&watchdog_ida, id);
ret = PTR_ERR(wdd->dev);
- return ret;
+ goto dev_create_fail;
}
+ INIT_DELAYED_WORK(&wdd->work, watchdog_worker);
+
+ ret = prepare_watchdog(wdd);
+ if (ret)
+ goto dev_prepare_fail;
+
return 0;
+
+dev_prepare_fail:
+dev_create_fail:
+ watchdog_dev_unregister(wdd);
+ ida_simple_remove(&watchdog_ida, id);
+ return ret;
}
EXPORT_SYMBOL_GPL(watchdog_register_device);
@@ -181,6 +298,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
if (wdd == NULL)
return;
+ cancel_delayed_work_sync(&wdd->work);
devno = wdd->cdev.dev;
ret = watchdog_dev_unregister(wdd);
if (ret)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 2c4d3f1..ac17668 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -73,10 +73,13 @@ static int watchdog_ping(struct watchdog_device *wddev)
if (!watchdog_hw_active(wddev))
goto out_ping;
- if (wddev->ops->ping)
- err = wddev->ops->ping(wddev); /* ping the watchdog */
- else
- err = wddev->ops->start(wddev); /* restart watchdog */
+ err = _watchdog_ping(wddev);
+
+ if (wddev->hw_max_timeout &&
+ wddev->timeout * 1000 > wddev->hw_max_timeout) {
+ wddev->expires = jiffies + wddev->timeout * HZ;
+ schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+ }
out_ping:
mutex_unlock(&wddev->lock);
@@ -92,7 +95,7 @@ out_ping:
* failure.
*/
-static int watchdog_start(struct watchdog_device *wddev)
+int watchdog_start(struct watchdog_device *wddev)
{
int err = 0;
@@ -100,17 +103,26 @@ static int watchdog_start(struct watchdog_device *wddev)
if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
err = -ENODEV;
- goto out_start;
+ goto out_no_start;
}
+ if (wddev->hw_max_timeout &&
+ wddev->timeout * HZ > msecs_to_jiffies(wddev->hw_max_timeout)) {
+ wddev->expires = jiffies + wddev->timeout * HZ;
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
+ } else
+ cancel_delayed_work(&wddev->work);
+
if (watchdog_hw_active(wddev))
- goto out_start;
+ goto out_started;
err = wddev->ops->start(wddev);
if (err == 0)
set_bit(WDOG_ACTIVE, &wddev->status);
-out_start:
+out_started:
+out_no_start:
mutex_unlock(&wddev->lock);
return err;
}
@@ -125,7 +137,7 @@ out_start:
* If the 'nowayout' feature was set, the watchdog cannot be stopped.
*/
-static int watchdog_stop(struct watchdog_device *wddev)
+int watchdog_stop(struct watchdog_device *wddev)
{
int err = 0;
@@ -146,9 +158,29 @@ static int watchdog_stop(struct watchdog_device *wddev)
}
err = wddev->ops->stop(wddev);
- if (err == 0)
- clear_bit(WDOG_ACTIVE, &wddev->status);
+ if (err == -ENOTSUPP) {
+ /*
+ * Unstoppable watchdogs need the worker to keep them
+ * alive. Ping it once as we don't know how much time
+ * there is left in the watchdog timer and it will
+ * take a while until the worker pings it.
+ */
+ err = _watchdog_ping(wddev);
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
+ goto out_stop;
+ } else {
+ /*
+ * in case there was some other failure to stop the
+ * watchdog, return error and be prepared for a
+ * watchdog reset..
+ */
+ goto out_err_stop;
+ }
+ clear_bit(WDOG_ACTIVE, &wddev->status);
+
+out_err_stop:
out_stop:
mutex_unlock(&wddev->lock);
return err;
@@ -194,14 +226,8 @@ out_status:
static int watchdog_set_timeout(struct watchdog_device *wddev,
unsigned int timeout)
{
- int err;
-
- if ((wddev->ops->set_timeout == NULL) ||
- !(wddev->info->options & WDIOF_SETTIMEOUT))
- return -EOPNOTSUPP;
-
- if (watchdog_timeout_invalid(wddev, timeout))
- return -EINVAL;
+ int hw_timeout_sec;
+ int err = 0;
mutex_lock(&wddev->lock);
@@ -210,7 +236,46 @@ static int watchdog_set_timeout(struct watchdog_device *wddev,
goto out_timeout;
}
- err = wddev->ops->set_timeout(wddev, timeout);
+ if (watchdog_needs_legacy_handling(wddev)) {
+ if ((wddev->ops->set_timeout == NULL) ||
+ !(wddev->info->options & WDIOF_SETTIMEOUT)) {
+ err = -EOPNOTSUPP;
+ goto out_timeout;
+ }
+
+ if (watchdog_timeout_invalid(wddev, timeout)) {
+ err = -EINVAL;
+ goto out_timeout;
+ }
+
+ err = wddev->ops->set_timeout(wddev, timeout);
+ goto out_timeout;
+ }
+
+ if (timeout < wddev->min_timeout)
+ return -EINVAL;
+
+ hw_timeout_sec = min(timeout, wddev->hw_max_timeout / 1000);
+ if (wddev->info->options & WDIOF_SETTIMEOUT)
+ err = wddev->ops->set_timeout(wddev, hw_timeout_sec);
+
+ if (timeout * 1000 > wddev->hw_max_timeout) {
+ /*
+ * Worker heartbeat period might have changed, restart
+ * the worker to ensure the next keepalive ping
+ * happens at the right moment.
+ */
+ cancel_delayed_work(&wddev->work);
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
+ }
+
+ /*
+ * The watchdog core emulates all timeouts that are longer
+ * than what the HW is capable of.
+ */
+ if (hw_timeout_sec < timeout)
+ wddev->timeout = timeout;
out_timeout:
mutex_unlock(&wddev->lock);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 5b84578..03f0553 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -12,6 +12,7 @@
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/cdev.h>
+#include <linux/workqueue.h>
#include <uapi/linux/watchdog.h>
struct watchdog_ops;
@@ -59,11 +60,14 @@ struct watchdog_ops {
* @info: Pointer to a watchdog_info structure.
* @ops: Pointer to the list of watchdog operations.
* @bootstatus: Status of the watchdog device at boot.
- * @timeout: The watchdog devices timeout value.
+ * @timeout: The watchdog devices timeout value for userspace, in seconds.
* @min_timeout:The watchdog devices minimum timeout value.
- * @max_timeout:The watchdog devices maximum timeout value.
+ * @max_timeout:The watchdog devices maximum timeout value. DEPRECATED!
+ * @hw_max_timeout:The watchdog hardware maximum timeout value.
+ * @hw_heartbeat:Time interval in HW between timer pings.
* @driver-data:Pointer to the drivers private data.
* @lock: Lock for watchdog core internal use only.
+ * @work: Worker used to provide longer timeouts than hardware supports.
* @status: Field that contains the devices internal status bits.
*
* The watchdog_device structure contains all information about a
@@ -85,9 +89,13 @@ struct watchdog_device {
unsigned int bootstatus;
unsigned int timeout;
unsigned int min_timeout;
- unsigned int max_timeout;
+ unsigned int max_timeout; /* DEPRECATED, do not use */
+ unsigned int hw_max_timeout; /* in msec */
+ unsigned int hw_heartbeat; /* in msec */
void *driver_data;
struct mutex lock;
+ struct delayed_work work; /* keepalive worker */
+ unsigned long int expires; /* for keepalive worker */
unsigned long status;
/* Bit numbers for status flags */
#define WDOG_ACTIVE 0 /* Is the watchdog hw running/active */
@@ -106,6 +114,22 @@ static inline bool watchdog_hw_active(struct watchdog_device *wdd)
return test_bit(WDOG_ACTIVE, &wdd->status);
}
+/* Use to test whether userspace has /dev/watchdog open */
+static inline bool watchdog_dev_open(struct watchdog_device *wdd)
+{
+ return test_bit(WDOG_DEV_OPEN, &wdd->status);
+}
+
+/*
+ * Test whether driver still expects legacy behavior from core. Once
+ * all drivers are converted, remove this function and all code
+ * depending on it.
+ */
+static inline bool watchdog_needs_legacy_handling(struct watchdog_device *wdd)
+{
+ return !wdd->hw_max_timeout;
+}
+
/* Use the following function to set the nowayout feature */
static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
{
@@ -113,11 +137,30 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
set_bit(WDOG_NO_WAY_OUT, &wdd->status);
}
+/* Use for setting active flag */
+static inline void watchdog_set_hw_active(struct watchdog_device *wdd,
+ bool active)
+{
+ if (active)
+ set_bit(WDOG_ACTIVE, &wdd->status);
+ else
+ clear_bit(WDOG_ACTIVE, &wdd->status);
+}
+
/* Use the following function to check if a timeout value is invalid */
static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
{
- return ((wdd->max_timeout != 0) &&
- (t < wdd->min_timeout || t > wdd->max_timeout));
+ if (!wdd->hw_max_timeout) {
+ /*
+ * Legacy drivers still have meaningful
+ * max_timeout. Newer drivers don't have maximum as
+ * core can extend the timeout indefinitely
+ */
+ return ((wdd->max_timeout != 0) &&
+ (t < wdd->min_timeout || t > wdd->max_timeout));
+ }
+
+ return t > wdd->min_timeout;
}
/* Use the following functions to manipulate watchdog driver specific data */
@@ -131,9 +174,20 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
return wdd->driver_data;
}
+static inline int _watchdog_ping(struct watchdog_device *wddev)
+{
+ if (wddev->ops->ping)
+ return wddev->ops->ping(wddev); /* ping the watchdog */
+ else
+ return wddev->ops->start(wddev); /* restart watchdog */
+}
+
/* drivers/watchdog/watchdog_core.c */
extern int watchdog_init_timeout(struct watchdog_device *wdd,
unsigned int timeout_parm, struct device *dev);
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev);
+int watchdog_start(struct watchdog_device *wddev);
+int watchdog_stop(struct watchdog_device *wddev);
extern int watchdog_register_device(struct watchdog_device *);
extern void watchdog_unregister_device(struct watchdog_device *);
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 02/10] watchdog: core: Ping watchdog on behalf of user space when needed
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Many watchdogs are not stoppable on the hardware level at all. Some
others have a very short maximum timeout value. Both of these limits
are problematic to the userspace and watchdog drivers have been
traditionally implementing workarounds of their own in order to
overcome the limitations. This obviously results in duplicate code in
the driver level and makes it harder to implement generic hardware
independent features.
Extend the watchdog core by allowing it do the most common tasks on
behalf of the driver. For this to work the watchdog core needs two new
values from the driver, hw_max_timeout and hw_heartbeat. The first one
tells the core what is the maximum supported timeout value in the
hardware and the second one tells the preferred heartbeat value for
the hardware. Both values are in milliseconds.
The driver needs to also call watchdog_init_params() in order to let
watchdog core fill in default watchdog paramters and let the core
check the validity of the values.
The watchdog core api changes slightly because of this change. Most
importantly, the watchdog core now stands out as a clear midlayer
between the driver and the user space. That is, the hw_max_timeout and
hw_heartbeat values are meant to be filled in by the driver for the
core. They will not be visible to user space any way. The timeout
variable however is part of user space API. The comments in watchdog.h
are changed also to reflect more clearly the difference. The
max_timeout also becomes obsolete as the worker can support arbitrary
timeout values.
As long as we have still old drivers that don't tell the core about
their hw capabilities, we need to support the old driver handling
too. Add watchdog_needs_legacy_handling() function to watchdog.h so we
can implement easily the old driver behavior and it becomes clear from
the code which parts can be removed once all existing drivers supply
the new parameters to watchdog core.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/watchdog_core.c | 124 ++++++++++++++++++++++++++++++++++++++-
drivers/watchdog/watchdog_dev.c | 105 ++++++++++++++++++++++++++-------
include/linux/watchdog.h | 64 ++++++++++++++++++--
3 files changed, 265 insertions(+), 28 deletions(-)
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index cec9b55..16e10e0 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -45,6 +45,9 @@ static struct class *watchdog_class;
static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
{
+ /* Newer drivers don't need this check any more */
+ if (!watchdog_needs_legacy_handling(wdd))
+ return;
/*
* Check that we have valid min and max timeout values, if
* not reset them both to 0 (=not used or unknown)
@@ -98,6 +101,110 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
}
EXPORT_SYMBOL_GPL(watchdog_init_timeout);
+static void watchdog_set_default_timeout(struct watchdog_device *wdd,
+ struct device *dev)
+{
+ int ret;
+ /*
+ * If driver has already set up a timeout, eg. from a module
+ * parameter, no need to do anything here
+ */
+ if (!watchdog_timeout_invalid(wdd, wdd->timeout))
+ return;
+
+ /* Query device tree */
+ if (dev && dev->of_node) {
+ ret = of_property_read_u32(dev->of_node, "timeout-sec",
+ &wdd->timeout);
+ if (!ret && !watchdog_timeout_invalid(wdd, wdd->timeout))
+ return;
+ }
+
+ /*
+ * If no other preference is given, use 60 seconds as the
+ * default timeout value
+ */
+ wdd->timeout = 60;
+}
+
+/**
+ * watchdog_init_parms() - initialize generic watchdog parameters
+ * @wdd: Watchdog device to operate
+ * @dev: Device that stores the device tree properties
+ *
+ * Initialize the generic watchdog parameters. At least hw_max_timeout
+ * must be set prior calling this function. Other unset parameters are
+ * set based on information gathered from different sources (kernel
+ * config, device tree, ...) or set up with a reasonable defaults.
+ *
+ * A zero is returned on success and -EINVAL for failure.
+ */
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
+{
+ int ret = 0;
+
+ watchdog_set_default_timeout(wdd, dev);
+
+ if (wdd->max_timeout)
+ dev_err(dev, "Driver setting deprecated max_timeout, please fix\n");
+
+ if (!wdd->hw_max_timeout)
+ return -EINVAL;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(watchdog_init_params);
+
+static void watchdog_worker(struct work_struct *work)
+{
+ struct watchdog_device *wdd = container_of(to_delayed_work(work),
+ struct watchdog_device, work);
+ bool stopped_keepalive;
+ bool extend_keepalive;
+
+ mutex_lock(&wdd->lock);
+
+ /* Watchdog device is not open but HW does not support stopping */
+ stopped_keepalive = watchdog_hw_active(wdd) &&
+ !watchdog_dev_open(wdd);
+
+ /* Keepalive time is longer than what hardware is capable */
+ extend_keepalive = watchdog_dev_open(wdd) &&
+ msecs_to_jiffies(wdd->hw_max_timeout) < wdd->timeout * HZ;
+
+ if (time_after(jiffies, wdd->expires) && watchdog_dev_open(wdd)) {
+ /*
+ * Userspace has exceeded its timeout, the watchdog is
+ * going to trigger soon.
+ */
+ mutex_unlock(&wdd->lock);
+ return;
+ }
+
+ if (stopped_keepalive || extend_keepalive) {
+ unsigned long expires = msecs_to_jiffies(wdd->hw_heartbeat);
+
+ _watchdog_ping(wdd);
+ schedule_delayed_work(&wdd->work, expires);
+ }
+
+ mutex_unlock(&wdd->lock);
+}
+
+static int prepare_watchdog(struct watchdog_device *wdd)
+{
+ if (watchdog_needs_legacy_handling(wdd)) {
+ pr_info("Incomplete watchdog driver implementation, please report or fix\n");
+ return 0;
+ }
+
+ if (!watchdog_hw_active(wdd))
+ return 0;
+
+ /* Stop the watchdog now until user space opens the device */
+ return watchdog_stop(wdd);
+}
+
/**
* watchdog_register_device() - register a watchdog device
* @wdd: watchdog device
@@ -156,13 +263,23 @@ int watchdog_register_device(struct watchdog_device *wdd)
wdd->dev = device_create(watchdog_class, wdd->parent, devno,
NULL, "watchdog%d", wdd->id);
if (IS_ERR(wdd->dev)) {
- watchdog_dev_unregister(wdd);
- ida_simple_remove(&watchdog_ida, id);
ret = PTR_ERR(wdd->dev);
- return ret;
+ goto dev_create_fail;
}
+ INIT_DELAYED_WORK(&wdd->work, watchdog_worker);
+
+ ret = prepare_watchdog(wdd);
+ if (ret)
+ goto dev_prepare_fail;
+
return 0;
+
+dev_prepare_fail:
+dev_create_fail:
+ watchdog_dev_unregister(wdd);
+ ida_simple_remove(&watchdog_ida, id);
+ return ret;
}
EXPORT_SYMBOL_GPL(watchdog_register_device);
@@ -181,6 +298,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
if (wdd == NULL)
return;
+ cancel_delayed_work_sync(&wdd->work);
devno = wdd->cdev.dev;
ret = watchdog_dev_unregister(wdd);
if (ret)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 2c4d3f1..ac17668 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -73,10 +73,13 @@ static int watchdog_ping(struct watchdog_device *wddev)
if (!watchdog_hw_active(wddev))
goto out_ping;
- if (wddev->ops->ping)
- err = wddev->ops->ping(wddev); /* ping the watchdog */
- else
- err = wddev->ops->start(wddev); /* restart watchdog */
+ err = _watchdog_ping(wddev);
+
+ if (wddev->hw_max_timeout &&
+ wddev->timeout * 1000 > wddev->hw_max_timeout) {
+ wddev->expires = jiffies + wddev->timeout * HZ;
+ schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+ }
out_ping:
mutex_unlock(&wddev->lock);
@@ -92,7 +95,7 @@ out_ping:
* failure.
*/
-static int watchdog_start(struct watchdog_device *wddev)
+int watchdog_start(struct watchdog_device *wddev)
{
int err = 0;
@@ -100,17 +103,26 @@ static int watchdog_start(struct watchdog_device *wddev)
if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
err = -ENODEV;
- goto out_start;
+ goto out_no_start;
}
+ if (wddev->hw_max_timeout &&
+ wddev->timeout * HZ > msecs_to_jiffies(wddev->hw_max_timeout)) {
+ wddev->expires = jiffies + wddev->timeout * HZ;
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
+ } else
+ cancel_delayed_work(&wddev->work);
+
if (watchdog_hw_active(wddev))
- goto out_start;
+ goto out_started;
err = wddev->ops->start(wddev);
if (err == 0)
set_bit(WDOG_ACTIVE, &wddev->status);
-out_start:
+out_started:
+out_no_start:
mutex_unlock(&wddev->lock);
return err;
}
@@ -125,7 +137,7 @@ out_start:
* If the 'nowayout' feature was set, the watchdog cannot be stopped.
*/
-static int watchdog_stop(struct watchdog_device *wddev)
+int watchdog_stop(struct watchdog_device *wddev)
{
int err = 0;
@@ -146,9 +158,29 @@ static int watchdog_stop(struct watchdog_device *wddev)
}
err = wddev->ops->stop(wddev);
- if (err == 0)
- clear_bit(WDOG_ACTIVE, &wddev->status);
+ if (err == -ENOTSUPP) {
+ /*
+ * Unstoppable watchdogs need the worker to keep them
+ * alive. Ping it once as we don't know how much time
+ * there is left in the watchdog timer and it will
+ * take a while until the worker pings it.
+ */
+ err = _watchdog_ping(wddev);
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
+ goto out_stop;
+ } else {
+ /*
+ * in case there was some other failure to stop the
+ * watchdog, return error and be prepared for a
+ * watchdog reset..
+ */
+ goto out_err_stop;
+ }
+ clear_bit(WDOG_ACTIVE, &wddev->status);
+
+out_err_stop:
out_stop:
mutex_unlock(&wddev->lock);
return err;
@@ -194,14 +226,8 @@ out_status:
static int watchdog_set_timeout(struct watchdog_device *wddev,
unsigned int timeout)
{
- int err;
-
- if ((wddev->ops->set_timeout == NULL) ||
- !(wddev->info->options & WDIOF_SETTIMEOUT))
- return -EOPNOTSUPP;
-
- if (watchdog_timeout_invalid(wddev, timeout))
- return -EINVAL;
+ int hw_timeout_sec;
+ int err = 0;
mutex_lock(&wddev->lock);
@@ -210,7 +236,46 @@ static int watchdog_set_timeout(struct watchdog_device *wddev,
goto out_timeout;
}
- err = wddev->ops->set_timeout(wddev, timeout);
+ if (watchdog_needs_legacy_handling(wddev)) {
+ if ((wddev->ops->set_timeout == NULL) ||
+ !(wddev->info->options & WDIOF_SETTIMEOUT)) {
+ err = -EOPNOTSUPP;
+ goto out_timeout;
+ }
+
+ if (watchdog_timeout_invalid(wddev, timeout)) {
+ err = -EINVAL;
+ goto out_timeout;
+ }
+
+ err = wddev->ops->set_timeout(wddev, timeout);
+ goto out_timeout;
+ }
+
+ if (timeout < wddev->min_timeout)
+ return -EINVAL;
+
+ hw_timeout_sec = min(timeout, wddev->hw_max_timeout / 1000);
+ if (wddev->info->options & WDIOF_SETTIMEOUT)
+ err = wddev->ops->set_timeout(wddev, hw_timeout_sec);
+
+ if (timeout * 1000 > wddev->hw_max_timeout) {
+ /*
+ * Worker heartbeat period might have changed, restart
+ * the worker to ensure the next keepalive ping
+ * happens at the right moment.
+ */
+ cancel_delayed_work(&wddev->work);
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
+ }
+
+ /*
+ * The watchdog core emulates all timeouts that are longer
+ * than what the HW is capable of.
+ */
+ if (hw_timeout_sec < timeout)
+ wddev->timeout = timeout;
out_timeout:
mutex_unlock(&wddev->lock);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 5b84578..03f0553 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -12,6 +12,7 @@
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/cdev.h>
+#include <linux/workqueue.h>
#include <uapi/linux/watchdog.h>
struct watchdog_ops;
@@ -59,11 +60,14 @@ struct watchdog_ops {
* @info: Pointer to a watchdog_info structure.
* @ops: Pointer to the list of watchdog operations.
* @bootstatus: Status of the watchdog device at boot.
- * @timeout: The watchdog devices timeout value.
+ * @timeout: The watchdog devices timeout value for userspace, in seconds.
* @min_timeout:The watchdog devices minimum timeout value.
- * @max_timeout:The watchdog devices maximum timeout value.
+ * @max_timeout:The watchdog devices maximum timeout value. DEPRECATED!
+ * @hw_max_timeout:The watchdog hardware maximum timeout value.
+ * @hw_heartbeat:Time interval in HW between timer pings.
* @driver-data:Pointer to the drivers private data.
* @lock: Lock for watchdog core internal use only.
+ * @work: Worker used to provide longer timeouts than hardware supports.
* @status: Field that contains the devices internal status bits.
*
* The watchdog_device structure contains all information about a
@@ -85,9 +89,13 @@ struct watchdog_device {
unsigned int bootstatus;
unsigned int timeout;
unsigned int min_timeout;
- unsigned int max_timeout;
+ unsigned int max_timeout; /* DEPRECATED, do not use */
+ unsigned int hw_max_timeout; /* in msec */
+ unsigned int hw_heartbeat; /* in msec */
void *driver_data;
struct mutex lock;
+ struct delayed_work work; /* keepalive worker */
+ unsigned long int expires; /* for keepalive worker */
unsigned long status;
/* Bit numbers for status flags */
#define WDOG_ACTIVE 0 /* Is the watchdog hw running/active */
@@ -106,6 +114,22 @@ static inline bool watchdog_hw_active(struct watchdog_device *wdd)
return test_bit(WDOG_ACTIVE, &wdd->status);
}
+/* Use to test whether userspace has /dev/watchdog open */
+static inline bool watchdog_dev_open(struct watchdog_device *wdd)
+{
+ return test_bit(WDOG_DEV_OPEN, &wdd->status);
+}
+
+/*
+ * Test whether driver still expects legacy behavior from core. Once
+ * all drivers are converted, remove this function and all code
+ * depending on it.
+ */
+static inline bool watchdog_needs_legacy_handling(struct watchdog_device *wdd)
+{
+ return !wdd->hw_max_timeout;
+}
+
/* Use the following function to set the nowayout feature */
static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
{
@@ -113,11 +137,30 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
set_bit(WDOG_NO_WAY_OUT, &wdd->status);
}
+/* Use for setting active flag */
+static inline void watchdog_set_hw_active(struct watchdog_device *wdd,
+ bool active)
+{
+ if (active)
+ set_bit(WDOG_ACTIVE, &wdd->status);
+ else
+ clear_bit(WDOG_ACTIVE, &wdd->status);
+}
+
/* Use the following function to check if a timeout value is invalid */
static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
{
- return ((wdd->max_timeout != 0) &&
- (t < wdd->min_timeout || t > wdd->max_timeout));
+ if (!wdd->hw_max_timeout) {
+ /*
+ * Legacy drivers still have meaningful
+ * max_timeout. Newer drivers don't have maximum as
+ * core can extend the timeout indefinitely
+ */
+ return ((wdd->max_timeout != 0) &&
+ (t < wdd->min_timeout || t > wdd->max_timeout));
+ }
+
+ return t > wdd->min_timeout;
}
/* Use the following functions to manipulate watchdog driver specific data */
@@ -131,9 +174,20 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
return wdd->driver_data;
}
+static inline int _watchdog_ping(struct watchdog_device *wddev)
+{
+ if (wddev->ops->ping)
+ return wddev->ops->ping(wddev); /* ping the watchdog */
+ else
+ return wddev->ops->start(wddev); /* restart watchdog */
+}
+
/* drivers/watchdog/watchdog_core.c */
extern int watchdog_init_timeout(struct watchdog_device *wdd,
unsigned int timeout_parm, struct device *dev);
+int watchdog_init_params(struct watchdog_device *wdd, struct device *dev);
+int watchdog_start(struct watchdog_device *wddev);
+int watchdog_stop(struct watchdog_device *wddev);
extern int watchdog_register_device(struct watchdog_device *);
extern void watchdog_unregister_device(struct watchdog_device *);
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 03/10] watchdog: core: Introduce default watchdog policy
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
Traditionally there have been no such thing as a policy for
watchdogs. The watchdog timer was simply explicitly stopped at driver
probe time and there was nothing else to do. Now that we have ability
to work around HW disabilities in the watchdog core, it is time to
introduce more policy options.
The default option is to shut down the watchdog driver, as
before. This ensures backward compatibility with current kernels and
hardware that might have a watchdog, but user has not set up a
watchdog daemon at all.
A new option is introduced to keep the watchdog running or start it up
at boot up. This is useful on many production systems that rely on the
watchdog to reboot the device in case a crash. With this option even
early kernel crashes or early user space crashes will lead to a reboot
even though watchdog daemon has not opened and started the watchdog
device yet.
The third introduced option is to not touch the hardware at all in
case bootloader have made an intelligent decision about the watchdog
state and does not know how to tell kernel about it. The watchdog
state is not touched until userspace takes over it.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/Kconfig | 51 ++++++++++++++++++++++++++++++++++++++++
drivers/watchdog/watchdog_core.c | 23 +++++++++++++++---
2 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..0066d6d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -45,6 +45,57 @@ config WATCHDOG_NOWAYOUT
get killed. If you say Y here, the watchdog cannot be stopped once
it has been started.
+choice
+ prompt "Watchdog default policy"
+ default WATCHDOG_BOOT_STOPPED
+ depends on WATCHDOG_CORE
+ help
+ Choose the default policy for watchdog device during boot up
+ time. The default is to stop the watchdog until a watchdog
+ daemon opens the device. This is also the traditional way
+ watchdogs have been behaving. Some production systems might
+ require the watchdog hardware never stops, but this requires
+ the system has watchdog daemon starting up early in the boot
+ process.
+
+ This config option selects the default built in policy in
+ case no other policy is defined.
+
+ If unsure, select 'Stop at boot'
+
+config WATCHDOG_BOOT_STOPPED
+ bool "Stop at boot up"
+ help
+ Stop the watchdog when driver loads. This is good for
+ distribution kernels that don't know whether there is a
+ watchdog daemon at all or user might start the watchdog
+ manually.
+
+config WATCHDOG_BOOT_RUNNING
+ bool "Start at boot up"
+ help
+ Start the watchdog timer as soon as the driver loads. This
+ is suitable on production systems that always have a
+ watchdog daemon running and it is mandatory for the system
+ to reset on any crash. If watchdog hardware was already
+ running before kernel starts up, this option guarantees the
+ watchdog timer is running continuously until userspace opens
+ the device. Combine this with WATCHDOG_NOWAYOUT and it is
+ guaranteed that watchdog can not be stopped from userspace
+ at all.
+
+config WATCHDOG_BOOT_NOTOUCH
+ bool "Do not change watchdog state"
+ help
+ Do not change the watchdog state at all. If the watchdog is
+ running when driver loads, it is not stopped. If the
+ watchdog is stopped, it is not started. This is useful in
+ case bootloader has already made a rational policy choice
+ with the watchdog and user space knows how to handle that
+ properly. Kernel does not interfere in the between any way.
+
+endchoice
+
#
# General Watchdog drivers
#
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 16e10e0..9ae5b8e 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -193,16 +193,33 @@ static void watchdog_worker(struct work_struct *work)
static int prepare_watchdog(struct watchdog_device *wdd)
{
+ bool watchdog_policy_start = false;
+ bool watchdog_policy_stop = false;
+
if (watchdog_needs_legacy_handling(wdd)) {
pr_info("Incomplete watchdog driver implementation, please report or fix\n");
return 0;
}
- if (!watchdog_hw_active(wdd))
+ /* Figure out proper watchdog boot up state */
+
+ watchdog_policy_start = IS_ENABLED(CONFIG_WATCHDOG_BOOT_RUNNING);
+ watchdog_policy_stop = IS_ENABLED(CONFIG_WATCHDOG_BOOT_STOPPED);
+
+ if (!watchdog_hw_active(wdd) && watchdog_policy_stop)
return 0;
- /* Stop the watchdog now until user space opens the device */
- return watchdog_stop(wdd);
+ if (watchdog_policy_start && !watchdog_hw_active(wdd))
+ return watchdog_start(wdd);
+
+ if (watchdog_policy_stop && watchdog_hw_active(wdd))
+ return watchdog_stop(wdd);
+
+ /*
+ * WATCHDOG_BOOT_NOTOUCH gets us here, don't touch the
+ * watchdog at all.
+ */
+ return 0;
}
/**
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 03/10] watchdog: core: Introduce default watchdog policy
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Traditionally there have been no such thing as a policy for
watchdogs. The watchdog timer was simply explicitly stopped at driver
probe time and there was nothing else to do. Now that we have ability
to work around HW disabilities in the watchdog core, it is time to
introduce more policy options.
The default option is to shut down the watchdog driver, as
before. This ensures backward compatibility with current kernels and
hardware that might have a watchdog, but user has not set up a
watchdog daemon at all.
A new option is introduced to keep the watchdog running or start it up
at boot up. This is useful on many production systems that rely on the
watchdog to reboot the device in case a crash. With this option even
early kernel crashes or early user space crashes will lead to a reboot
even though watchdog daemon has not opened and started the watchdog
device yet.
The third introduced option is to not touch the hardware at all in
case bootloader have made an intelligent decision about the watchdog
state and does not know how to tell kernel about it. The watchdog
state is not touched until userspace takes over it.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/Kconfig | 51 ++++++++++++++++++++++++++++++++++++++++
drivers/watchdog/watchdog_core.c | 23 +++++++++++++++---
2 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..0066d6d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -45,6 +45,57 @@ config WATCHDOG_NOWAYOUT
get killed. If you say Y here, the watchdog cannot be stopped once
it has been started.
+choice
+ prompt "Watchdog default policy"
+ default WATCHDOG_BOOT_STOPPED
+ depends on WATCHDOG_CORE
+ help
+ Choose the default policy for watchdog device during boot up
+ time. The default is to stop the watchdog until a watchdog
+ daemon opens the device. This is also the traditional way
+ watchdogs have been behaving. Some production systems might
+ require the watchdog hardware never stops, but this requires
+ the system has watchdog daemon starting up early in the boot
+ process.
+
+ This config option selects the default built in policy in
+ case no other policy is defined.
+
+ If unsure, select 'Stop at boot'
+
+config WATCHDOG_BOOT_STOPPED
+ bool "Stop at boot up"
+ help
+ Stop the watchdog when driver loads. This is good for
+ distribution kernels that don't know whether there is a
+ watchdog daemon at all or user might start the watchdog
+ manually.
+
+config WATCHDOG_BOOT_RUNNING
+ bool "Start at boot up"
+ help
+ Start the watchdog timer as soon as the driver loads. This
+ is suitable on production systems that always have a
+ watchdog daemon running and it is mandatory for the system
+ to reset on any crash. If watchdog hardware was already
+ running before kernel starts up, this option guarantees the
+ watchdog timer is running continuously until userspace opens
+ the device. Combine this with WATCHDOG_NOWAYOUT and it is
+ guaranteed that watchdog can not be stopped from userspace
+ at all.
+
+config WATCHDOG_BOOT_NOTOUCH
+ bool "Do not change watchdog state"
+ help
+ Do not change the watchdog state at all. If the watchdog is
+ running when driver loads, it is not stopped. If the
+ watchdog is stopped, it is not started. This is useful in
+ case bootloader has already made a rational policy choice
+ with the watchdog and user space knows how to handle that
+ properly. Kernel does not interfere in the between any way.
+
+endchoice
+
#
# General Watchdog drivers
#
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 16e10e0..9ae5b8e 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -193,16 +193,33 @@ static void watchdog_worker(struct work_struct *work)
static int prepare_watchdog(struct watchdog_device *wdd)
{
+ bool watchdog_policy_start = false;
+ bool watchdog_policy_stop = false;
+
if (watchdog_needs_legacy_handling(wdd)) {
pr_info("Incomplete watchdog driver implementation, please report or fix\n");
return 0;
}
- if (!watchdog_hw_active(wdd))
+ /* Figure out proper watchdog boot up state */
+
+ watchdog_policy_start = IS_ENABLED(CONFIG_WATCHDOG_BOOT_RUNNING);
+ watchdog_policy_stop = IS_ENABLED(CONFIG_WATCHDOG_BOOT_STOPPED);
+
+ if (!watchdog_hw_active(wdd) && watchdog_policy_stop)
return 0;
- /* Stop the watchdog now until user space opens the device */
- return watchdog_stop(wdd);
+ if (watchdog_policy_start && !watchdog_hw_active(wdd))
+ return watchdog_start(wdd);
+
+ if (watchdog_policy_stop && watchdog_hw_active(wdd))
+ return watchdog_stop(wdd);
+
+ /*
+ * WATCHDOG_BOOT_NOTOUCH gets us here, don't touch the
+ * watchdog at all.
+ */
+ return 0;
}
/**
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 04/10] watchdog: core: Allow extending early timeout
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
It is possible that the specified watchdog timeout value is shorter
than the time it takes for the watchdog daemon to start up and take
over the watchdog device. This is problem if WATCHDOG_BOOT_RUNNING is
the desired watchdog policy.
Add a new timeout value that specifies how long kernel should extend
the timeout by pinging the watchdog hardware. This option still
ensures a watchdog reset takes place in case watchdog daemon fails to
open the device, but gives more freedom in case user space is slow
starting up and watchdog might trigger prematurely.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/Kconfig | 21 +++++++++++++++++++++
drivers/watchdog/watchdog_core.c | 37 +++++++++++++++++++++++++++++++++----
drivers/watchdog/watchdog_dev.c | 4 ++++
include/linux/watchdog.h | 1 +
4 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0066d6d..33f2f9c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -96,6 +96,27 @@ config WATCHDOG_BOOT_NOTOUCH
endchoice
+config WATCHDOG_EARLY_TIMEOUT_SEC
+ int "Extend watchdog early timeout"
+ depends on WATCHDOG_BOOT_RUNNING
+ default "0"
+ help
+ When WATCHDOG_BOOT_RUNNIN policy is selected the default
+ behavior is to start up or keep the watchdog hardware
+ running during boot up. In some cases the boot up time for
+ the userspace might be too long for the watchdog daemon to
+ take over the watchdog device in time, which might lead to
+ premature device reset.
+
+ This option specifies how long should the watchdog core keep
+ the watchdog hardware running before the device is
+ opened. Should there be a crash that prevents the daemon
+ from starting up, a normal watchdog reset occurs once the
+ initial timeout expires. Normal timeout limits apply once
+ watchdog daemon opens the device.
+
+ The timeout value is in seconds.
+
#
# General Watchdog drivers
#
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 9ae5b8e..7bf0e1e 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -141,6 +141,7 @@ static void watchdog_set_default_timeout(struct watchdog_device *wdd,
*/
int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
{
+ unsigned int t = 0;
int ret = 0;
watchdog_set_default_timeout(wdd, dev);
@@ -148,6 +149,13 @@ int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
if (wdd->max_timeout)
dev_err(dev, "Driver setting deprecated max_timeout, please fix\n");
+ wdd->early_timeout_sec = -1;
+#ifdef CONFIG_WATCHDOG_EARLY_TIMEOUT_SEC
+ wdd->early_timeout_sec = CONFIG_WATCHDOG_EARLY_TIMEOUT_SEC;
+#endif
+ if (!of_property_read_u32(dev->of_node, "early-timeout-sec", &t))
+ wdd->early_timeout_sec = t;
+
if (!wdd->hw_max_timeout)
return -EINVAL;
@@ -161,12 +169,16 @@ static void watchdog_worker(struct work_struct *work)
struct watchdog_device, work);
bool stopped_keepalive;
bool extend_keepalive;
+ bool early_timeout_keepalive;
mutex_lock(&wdd->lock);
/* Watchdog device is not open but HW does not support stopping */
stopped_keepalive = watchdog_hw_active(wdd) &&
- !watchdog_dev_open(wdd);
+ !watchdog_dev_open(wdd) && (wdd->early_timeout_sec < 0);
+
+ early_timeout_keepalive = wdd->early_timeout_sec >= 0 &&
+ time_before(jiffies, wdd->expires);
/* Keepalive time is longer than what hardware is capable */
extend_keepalive = watchdog_dev_open(wdd) &&
@@ -181,7 +193,7 @@ static void watchdog_worker(struct work_struct *work)
return;
}
- if (stopped_keepalive || extend_keepalive) {
+ if (stopped_keepalive || extend_keepalive || early_timeout_keepalive) {
unsigned long expires = msecs_to_jiffies(wdd->hw_heartbeat);
_watchdog_ping(wdd);
@@ -206,11 +218,28 @@ static int prepare_watchdog(struct watchdog_device *wdd)
watchdog_policy_start = IS_ENABLED(CONFIG_WATCHDOG_BOOT_RUNNING);
watchdog_policy_stop = IS_ENABLED(CONFIG_WATCHDOG_BOOT_STOPPED);
+ if (wdd->early_timeout_sec >= 0)
+ watchdog_policy_start = true;
+
+ if (wdd->early_timeout_sec > 0)
+ wdd->expires = jiffies + wdd->early_timeout_sec * HZ;
+
if (!watchdog_hw_active(wdd) && watchdog_policy_stop)
return 0;
- if (watchdog_policy_start && !watchdog_hw_active(wdd))
- return watchdog_start(wdd);
+ if (watchdog_policy_start && !watchdog_hw_active(wdd)) {
+ int ret;
+
+ ret = watchdog_start(wdd);
+
+ if (wdd->early_timeout_sec > 0) {
+ wdd->expires = jiffies + wdd->early_timeout_sec * HZ;
+ schedule_delayed_work(&wdd->work,
+ msecs_to_jiffies(wdd->hw_heartbeat));
+ }
+
+ return ret;
+ }
if (watchdog_policy_stop && watchdog_hw_active(wdd))
return watchdog_stop(wdd);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ac17668..0310eda 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -122,6 +122,10 @@ int watchdog_start(struct watchdog_device *wddev)
set_bit(WDOG_ACTIVE, &wddev->status);
out_started:
+ /* Once we open the device, early timeout can be disabled */
+ if (wddev->early_timeout_sec >= 0 && watchdog_dev_open(wddev))
+ wddev->early_timeout_sec = -1;
+
out_no_start:
mutex_unlock(&wddev->lock);
return err;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 03f0553..b746f7c 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -96,6 +96,7 @@ struct watchdog_device {
struct mutex lock;
struct delayed_work work; /* keepalive worker */
unsigned long int expires; /* for keepalive worker */
+ int early_timeout_sec;
unsigned long status;
/* Bit numbers for status flags */
#define WDOG_ACTIVE 0 /* Is the watchdog hw running/active */
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 04/10] watchdog: core: Allow extending early timeout
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
It is possible that the specified watchdog timeout value is shorter
than the time it takes for the watchdog daemon to start up and take
over the watchdog device. This is problem if WATCHDOG_BOOT_RUNNING is
the desired watchdog policy.
Add a new timeout value that specifies how long kernel should extend
the timeout by pinging the watchdog hardware. This option still
ensures a watchdog reset takes place in case watchdog daemon fails to
open the device, but gives more freedom in case user space is slow
starting up and watchdog might trigger prematurely.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/Kconfig | 21 +++++++++++++++++++++
drivers/watchdog/watchdog_core.c | 37 +++++++++++++++++++++++++++++++++----
drivers/watchdog/watchdog_dev.c | 4 ++++
include/linux/watchdog.h | 1 +
4 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0066d6d..33f2f9c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -96,6 +96,27 @@ config WATCHDOG_BOOT_NOTOUCH
endchoice
+config WATCHDOG_EARLY_TIMEOUT_SEC
+ int "Extend watchdog early timeout"
+ depends on WATCHDOG_BOOT_RUNNING
+ default "0"
+ help
+ When WATCHDOG_BOOT_RUNNIN policy is selected the default
+ behavior is to start up or keep the watchdog hardware
+ running during boot up. In some cases the boot up time for
+ the userspace might be too long for the watchdog daemon to
+ take over the watchdog device in time, which might lead to
+ premature device reset.
+
+ This option specifies how long should the watchdog core keep
+ the watchdog hardware running before the device is
+ opened. Should there be a crash that prevents the daemon
+ from starting up, a normal watchdog reset occurs once the
+ initial timeout expires. Normal timeout limits apply once
+ watchdog daemon opens the device.
+
+ The timeout value is in seconds.
+
#
# General Watchdog drivers
#
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 9ae5b8e..7bf0e1e 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -141,6 +141,7 @@ static void watchdog_set_default_timeout(struct watchdog_device *wdd,
*/
int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
{
+ unsigned int t = 0;
int ret = 0;
watchdog_set_default_timeout(wdd, dev);
@@ -148,6 +149,13 @@ int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
if (wdd->max_timeout)
dev_err(dev, "Driver setting deprecated max_timeout, please fix\n");
+ wdd->early_timeout_sec = -1;
+#ifdef CONFIG_WATCHDOG_EARLY_TIMEOUT_SEC
+ wdd->early_timeout_sec = CONFIG_WATCHDOG_EARLY_TIMEOUT_SEC;
+#endif
+ if (!of_property_read_u32(dev->of_node, "early-timeout-sec", &t))
+ wdd->early_timeout_sec = t;
+
if (!wdd->hw_max_timeout)
return -EINVAL;
@@ -161,12 +169,16 @@ static void watchdog_worker(struct work_struct *work)
struct watchdog_device, work);
bool stopped_keepalive;
bool extend_keepalive;
+ bool early_timeout_keepalive;
mutex_lock(&wdd->lock);
/* Watchdog device is not open but HW does not support stopping */
stopped_keepalive = watchdog_hw_active(wdd) &&
- !watchdog_dev_open(wdd);
+ !watchdog_dev_open(wdd) && (wdd->early_timeout_sec < 0);
+
+ early_timeout_keepalive = wdd->early_timeout_sec >= 0 &&
+ time_before(jiffies, wdd->expires);
/* Keepalive time is longer than what hardware is capable */
extend_keepalive = watchdog_dev_open(wdd) &&
@@ -181,7 +193,7 @@ static void watchdog_worker(struct work_struct *work)
return;
}
- if (stopped_keepalive || extend_keepalive) {
+ if (stopped_keepalive || extend_keepalive || early_timeout_keepalive) {
unsigned long expires = msecs_to_jiffies(wdd->hw_heartbeat);
_watchdog_ping(wdd);
@@ -206,11 +218,28 @@ static int prepare_watchdog(struct watchdog_device *wdd)
watchdog_policy_start = IS_ENABLED(CONFIG_WATCHDOG_BOOT_RUNNING);
watchdog_policy_stop = IS_ENABLED(CONFIG_WATCHDOG_BOOT_STOPPED);
+ if (wdd->early_timeout_sec >= 0)
+ watchdog_policy_start = true;
+
+ if (wdd->early_timeout_sec > 0)
+ wdd->expires = jiffies + wdd->early_timeout_sec * HZ;
+
if (!watchdog_hw_active(wdd) && watchdog_policy_stop)
return 0;
- if (watchdog_policy_start && !watchdog_hw_active(wdd))
- return watchdog_start(wdd);
+ if (watchdog_policy_start && !watchdog_hw_active(wdd)) {
+ int ret;
+
+ ret = watchdog_start(wdd);
+
+ if (wdd->early_timeout_sec > 0) {
+ wdd->expires = jiffies + wdd->early_timeout_sec * HZ;
+ schedule_delayed_work(&wdd->work,
+ msecs_to_jiffies(wdd->hw_heartbeat));
+ }
+
+ return ret;
+ }
if (watchdog_policy_stop && watchdog_hw_active(wdd))
return watchdog_stop(wdd);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ac17668..0310eda 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -122,6 +122,10 @@ int watchdog_start(struct watchdog_device *wddev)
set_bit(WDOG_ACTIVE, &wddev->status);
out_started:
+ /* Once we open the device, early timeout can be disabled */
+ if (wddev->early_timeout_sec >= 0 && watchdog_dev_open(wddev))
+ wddev->early_timeout_sec = -1;
+
out_no_start:
mutex_unlock(&wddev->lock);
return err;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 03f0553..b746f7c 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -96,6 +96,7 @@ struct watchdog_device {
struct mutex lock;
struct delayed_work work; /* keepalive worker */
unsigned long int expires; /* for keepalive worker */
+ int early_timeout_sec;
unsigned long status;
/* Bit numbers for status flags */
#define WDOG_ACTIVE 0 /* Is the watchdog hw running/active */
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 05/10] Documentation/watchdog: Document watchdog core API changes
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
Watchdog core API has changed somewhat and drivers are expected to
change. Document the new requirements so that driver writers know how
to do the change properly.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
.../watchdog/convert_drivers_to_kernel_api.txt | 26 ++++++++
Documentation/watchdog/watchdog-kernel-api.txt | 71 +++++++++++++---------
2 files changed, 68 insertions(+), 29 deletions(-)
diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
index 271b885..c9ba973 100644
--- a/Documentation/watchdog/convert_drivers_to_kernel_api.txt
+++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
@@ -185,6 +185,32 @@ by adding a module parameter. The conversion for this would be something like:
The module parameter itself needs to stay, everything else related to nowayout
can go, though. This will likely be some code in open(), close() or write().
+Remove possible adhoc timer code
+--------------------------------
+
+The watchdog core is capable of handling various features many
+watchdog hardware are missing, such as emulating stopped watchdog with
+unstoppable hardware or extending the timeout when hardware only
+supports very short timeout values. Traditionally watchdog drivers
+needed to implement code of their own to work around the hardware
+limitations. Now it is enough to record the watchdog boot time status
+with watchdog_set_hw_active() function and the watchdog core takes
+care of starting or stopping the watchdog, depending on the used
+policy. If watchdog cannot be stopped, the stop() function should
+return -ENOTSUPP and let core emulate stopping the watchdog.
+
+Initialize parameters with watchdog_init_params()
+-------------------------------------------------
+
+There are parameters that the watchdog driver does not need to be
+aware of and there are parameters that are optional to the driver. The
+watchdog_init_params() function takes care of validating the mandatory
+parameters and filling in reasonable values for the optional ones.
+
+What the driver MUST do is to fill in hw_max_timeout value in
+milliseconds, record the current watchdog hardware active status with
+watchdog_set_hw_active() and then call watchdog_init_params() and
+check the return value to see that everything went right.
Register the watchdog device
----------------------------
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index a0438f3..265306c 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -11,12 +11,21 @@ It also does not describe the API which can be used by user space to communicate
with a WatchDog Timer. If you want to know this then please read the following
file: Documentation/watchdog/watchdog-api.txt .
-So what does this document describe? It describes the API that can be used by
-WatchDog Timer Drivers that want to use the WatchDog Timer Driver Core
-Framework. This framework provides all interfacing towards user space so that
-the same code does not have to be reproduced each time. This also means that
-a watchdog timer driver then only needs to provide the different routines
-(operations) that control the watchdog timer (WDT).
+So what does this document describe? It describes the API that can be
+used by WatchDog Timer Drivers that want to use the WatchDog Timer
+Driver Core Framework. This framework provides all interfacing towards
+user space so that the same code does not have to be reproduced each
+time. This also means that a watchdog timer driver then only needs to
+provide the different routines (operations) that control the watchdog
+timer (WDT).
+
+The Watchdog Timer Driver Core also intermediates between the user
+space and the driver and provides same user experience to the user
+space regardless of the hardware limitations. For example, some
+watchdog hardware can not be stopped or they have very short maximum
+keepalive timeout. In such case watchdog core can emulate a stopped
+watchdog device or extend watchdog timeout by pinging periodically the
+watchdog driver on behalf of the user space.
The API
-------
@@ -48,7 +57,8 @@ struct watchdog_device {
unsigned int bootstatus;
unsigned int timeout;
unsigned int min_timeout;
- unsigned int max_timeout;
+ unsigned int hw_max_timeout;
+ unsigned int hw_keepalive;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -69,7 +79,8 @@ It contains following fields:
* ops: a pointer to the list of watchdog operations that the watchdog supports.
* timeout: the watchdog timer's timeout value (in seconds).
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
-* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* hw_max_timeout: the watchdog timer's maximum timeout value supported
+ in hardware (in milliseconds).
* bootstatus: status of the device after booting (reported with watchdog
WDIOF_* status bits).
* driver_data: a pointer to the drivers private data of a watchdog device.
@@ -124,12 +135,13 @@ are:
The routine needs a pointer to the watchdog timer device structure as a
parameter. It returns zero on success or a negative errno code for failure.
* stop: with this routine the watchdog timer device is being stopped.
- The routine needs a pointer to the watchdog timer device structure as a
- parameter. It returns zero on success or a negative errno code for failure.
- Some watchdog timer hardware can only be started and not be stopped. The
- driver supporting this hardware needs to make sure that a start and stop
- routine is being provided. This can be done by using a timer in the driver
- that regularly sends a keepalive ping to the watchdog timer hardware.
+ The routine needs a pointer to the watchdog timer device structure
+ as a parameter. It returns zero on success or a negative errno code
+ for failure. Some watchdog timer hardware can only be started and
+ not be stopped. If the watchdog timer can not be stopped, the stop
+ routine must return -ENOTSUPP error code. The watchdog core can then
+ take care of pinging the watchdog hardware while user space expects
+ it to be stopped.
Not all watchdog timer hardware supports the same functionality. That's why
all other routines/operations are optional. They only need to be provided if
@@ -168,10 +180,11 @@ they are supported. These optional routines/operations are:
The status bits should (preferably) be set with the set_bit and clear_bit alike
bit-operations. The status bits that are defined are:
-* WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
- is active or not. When the watchdog is active after booting, then you should
- set this status bit (Note: when you register the watchdog timer device with
- this bit set, then opening /dev/watchdog will skip the start operation)
+* WDOG_ACTIVE: this status bit indicates whether or not a watchdog
+ timer hardware is active or not. When the watchdog is active after
+ booting, then you should set this status bit (Note: when you
+ register the watchdog timer device with this bit set, then opening
+ /dev/watchdog will skip the start operation)
* WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
was opened via /dev/watchdog.
(This bit should only be used by the WatchDog Timer Driver Core).
@@ -213,14 +226,14 @@ The watchdog_get_drvdata function allows you to retrieve driver specific data.
The argument of this function is the watchdog device where you want to retrieve
data from. The function returns the pointer to the driver specific data.
-To initialize the timeout field, the following function can be used:
-
-extern int watchdog_init_timeout(struct watchdog_device *wdd,
- unsigned int timeout_parm, struct device *dev);
-
-The watchdog_init_timeout function allows you to initialize the timeout field
-using the module timeout parameter or by retrieving the timeout-sec property from
-the device tree (if the module timeout parameter is invalid). Best practice is
-to set the default timeout value as timeout value in the watchdog_device and
-then use this function to set the user "preferred" timeout value.
-This routine returns zero on success and a negative errno code for failure.
+You can initialize timeout field in the watchdog_device structure
+eg. based from a module parameter in case user has a preference over
+the initial timeout when loading the module.
+
+Your driver also must fill in appropriate hw_max_timeout value in the
+watchdog_device structure and then call watchdog_init_params
+function. This function will initialize watchdog internal parameters
+and fill in missing values, such as timeout value. The function can
+use device tree to fill in the necessary values, but it does not
+override timeout value set by the driver. In case invalid values are
+given, watchdog_inint_params returns a negative error.
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 05/10] Documentation/watchdog: Document watchdog core API changes
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Watchdog core API has changed somewhat and drivers are expected to
change. Document the new requirements so that driver writers know how
to do the change properly.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
.../watchdog/convert_drivers_to_kernel_api.txt | 26 ++++++++
Documentation/watchdog/watchdog-kernel-api.txt | 71 +++++++++++++---------
2 files changed, 68 insertions(+), 29 deletions(-)
diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
index 271b885..c9ba973 100644
--- a/Documentation/watchdog/convert_drivers_to_kernel_api.txt
+++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
@@ -185,6 +185,32 @@ by adding a module parameter. The conversion for this would be something like:
The module parameter itself needs to stay, everything else related to nowayout
can go, though. This will likely be some code in open(), close() or write().
+Remove possible adhoc timer code
+--------------------------------
+
+The watchdog core is capable of handling various features many
+watchdog hardware are missing, such as emulating stopped watchdog with
+unstoppable hardware or extending the timeout when hardware only
+supports very short timeout values. Traditionally watchdog drivers
+needed to implement code of their own to work around the hardware
+limitations. Now it is enough to record the watchdog boot time status
+with watchdog_set_hw_active() function and the watchdog core takes
+care of starting or stopping the watchdog, depending on the used
+policy. If watchdog cannot be stopped, the stop() function should
+return -ENOTSUPP and let core emulate stopping the watchdog.
+
+Initialize parameters with watchdog_init_params()
+-------------------------------------------------
+
+There are parameters that the watchdog driver does not need to be
+aware of and there are parameters that are optional to the driver. The
+watchdog_init_params() function takes care of validating the mandatory
+parameters and filling in reasonable values for the optional ones.
+
+What the driver MUST do is to fill in hw_max_timeout value in
+milliseconds, record the current watchdog hardware active status with
+watchdog_set_hw_active() and then call watchdog_init_params() and
+check the return value to see that everything went right.
Register the watchdog device
----------------------------
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index a0438f3..265306c 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -11,12 +11,21 @@ It also does not describe the API which can be used by user space to communicate
with a WatchDog Timer. If you want to know this then please read the following
file: Documentation/watchdog/watchdog-api.txt .
-So what does this document describe? It describes the API that can be used by
-WatchDog Timer Drivers that want to use the WatchDog Timer Driver Core
-Framework. This framework provides all interfacing towards user space so that
-the same code does not have to be reproduced each time. This also means that
-a watchdog timer driver then only needs to provide the different routines
-(operations) that control the watchdog timer (WDT).
+So what does this document describe? It describes the API that can be
+used by WatchDog Timer Drivers that want to use the WatchDog Timer
+Driver Core Framework. This framework provides all interfacing towards
+user space so that the same code does not have to be reproduced each
+time. This also means that a watchdog timer driver then only needs to
+provide the different routines (operations) that control the watchdog
+timer (WDT).
+
+The Watchdog Timer Driver Core also intermediates between the user
+space and the driver and provides same user experience to the user
+space regardless of the hardware limitations. For example, some
+watchdog hardware can not be stopped or they have very short maximum
+keepalive timeout. In such case watchdog core can emulate a stopped
+watchdog device or extend watchdog timeout by pinging periodically the
+watchdog driver on behalf of the user space.
The API
-------
@@ -48,7 +57,8 @@ struct watchdog_device {
unsigned int bootstatus;
unsigned int timeout;
unsigned int min_timeout;
- unsigned int max_timeout;
+ unsigned int hw_max_timeout;
+ unsigned int hw_keepalive;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -69,7 +79,8 @@ It contains following fields:
* ops: a pointer to the list of watchdog operations that the watchdog supports.
* timeout: the watchdog timer's timeout value (in seconds).
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
-* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* hw_max_timeout: the watchdog timer's maximum timeout value supported
+ in hardware (in milliseconds).
* bootstatus: status of the device after booting (reported with watchdog
WDIOF_* status bits).
* driver_data: a pointer to the drivers private data of a watchdog device.
@@ -124,12 +135,13 @@ are:
The routine needs a pointer to the watchdog timer device structure as a
parameter. It returns zero on success or a negative errno code for failure.
* stop: with this routine the watchdog timer device is being stopped.
- The routine needs a pointer to the watchdog timer device structure as a
- parameter. It returns zero on success or a negative errno code for failure.
- Some watchdog timer hardware can only be started and not be stopped. The
- driver supporting this hardware needs to make sure that a start and stop
- routine is being provided. This can be done by using a timer in the driver
- that regularly sends a keepalive ping to the watchdog timer hardware.
+ The routine needs a pointer to the watchdog timer device structure
+ as a parameter. It returns zero on success or a negative errno code
+ for failure. Some watchdog timer hardware can only be started and
+ not be stopped. If the watchdog timer can not be stopped, the stop
+ routine must return -ENOTSUPP error code. The watchdog core can then
+ take care of pinging the watchdog hardware while user space expects
+ it to be stopped.
Not all watchdog timer hardware supports the same functionality. That's why
all other routines/operations are optional. They only need to be provided if
@@ -168,10 +180,11 @@ they are supported. These optional routines/operations are:
The status bits should (preferably) be set with the set_bit and clear_bit alike
bit-operations. The status bits that are defined are:
-* WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
- is active or not. When the watchdog is active after booting, then you should
- set this status bit (Note: when you register the watchdog timer device with
- this bit set, then opening /dev/watchdog will skip the start operation)
+* WDOG_ACTIVE: this status bit indicates whether or not a watchdog
+ timer hardware is active or not. When the watchdog is active after
+ booting, then you should set this status bit (Note: when you
+ register the watchdog timer device with this bit set, then opening
+ /dev/watchdog will skip the start operation)
* WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
was opened via /dev/watchdog.
(This bit should only be used by the WatchDog Timer Driver Core).
@@ -213,14 +226,14 @@ The watchdog_get_drvdata function allows you to retrieve driver specific data.
The argument of this function is the watchdog device where you want to retrieve
data from. The function returns the pointer to the driver specific data.
-To initialize the timeout field, the following function can be used:
-
-extern int watchdog_init_timeout(struct watchdog_device *wdd,
- unsigned int timeout_parm, struct device *dev);
-
-The watchdog_init_timeout function allows you to initialize the timeout field
-using the module timeout parameter or by retrieving the timeout-sec property from
-the device tree (if the module timeout parameter is invalid). Best practice is
-to set the default timeout value as timeout value in the watchdog_device and
-then use this function to set the user "preferred" timeout value.
-This routine returns zero on success and a negative errno code for failure.
+You can initialize timeout field in the watchdog_device structure
+eg. based from a module parameter in case user has a preference over
+the initial timeout when loading the module.
+
+Your driver also must fill in appropriate hw_max_timeout value in the
+watchdog_device structure and then call watchdog_init_params
+function. This function will initialize watchdog internal parameters
+and fill in missing values, such as timeout value. The function can
+use device tree to fill in the necessary values, but it does not
+override timeout value set by the driver. In case invalid values are
+given, watchdog_inint_params returns a negative error.
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 06/10] devicetree: Document generic watchdog properties
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
There is no documentation for the watchdog properties that are common
among most of the watchdog drivers. Add document where these generic
properties can be described and told how they should be used in
drivers.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
.../devicetree/bindings/watchdog/watchdog.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/watchdog.txt
diff --git a/Documentation/devicetree/bindings/watchdog/watchdog.txt b/Documentation/devicetree/bindings/watchdog/watchdog.txt
new file mode 100644
index 0000000..3781406
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/watchdog.txt
@@ -0,0 +1,20 @@
+These properties are common among most watchdog drivers. Any driver
+that requires the functionality listed below should implement them
+using these definitions.
+
+Optional properties:
+- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specify the timeout in seconds for
+ how long it can take for the watchdog daemon to take over the
+ watchdog device. If driver supports this property it must ensure the
+ watchdog hardware is running during this period and a watchdog reset
+ must occur if user space fails to open the device in time. If left
+ zero, the driver only needs to guarantee the watchdog is not
+ stopped or is started during driver init.
+
+Example:
+
+watchdog {
+ timeout-sec = <60>;
+ early-timeout-sec = <120>;
+};
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 06/10] devicetree: Document generic watchdog properties
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
There is no documentation for the watchdog properties that are common
among most of the watchdog drivers. Add document where these generic
properties can be described and told how they should be used in
drivers.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
.../devicetree/bindings/watchdog/watchdog.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/watchdog.txt
diff --git a/Documentation/devicetree/bindings/watchdog/watchdog.txt b/Documentation/devicetree/bindings/watchdog/watchdog.txt
new file mode 100644
index 0000000..3781406
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/watchdog.txt
@@ -0,0 +1,20 @@
+These properties are common among most watchdog drivers. Any driver
+that requires the functionality listed below should implement them
+using these definitions.
+
+Optional properties:
+- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specify the timeout in seconds for
+ how long it can take for the watchdog daemon to take over the
+ watchdog device. If driver supports this property it must ensure the
+ watchdog hardware is running during this period and a watchdog reset
+ must occur if user space fails to open the device in time. If left
+ zero, the driver only needs to guarantee the watchdog is not
+ stopped or is started during driver init.
+
+Example:
+
+watchdog {
+ timeout-sec = <60>;
+ early-timeout-sec = <120>;
+};
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 07/10] Documentation/watchdog: watchdog-test.c: Add support for changing timeout
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
It might be useful for the random watchdog developer to also test
changing the watchdog timeout. Therefore, change the test application
to also support changing timeout.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
Documentation/watchdog/src/watchdog-test.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/watchdog/src/watchdog-test.c b/Documentation/watchdog/src/watchdog-test.c
index 3da8229..fb20d70 100644
--- a/Documentation/watchdog/src/watchdog-test.c
+++ b/Documentation/watchdog/src/watchdog-test.c
@@ -63,6 +63,12 @@ int main(int argc, char *argv[])
fprintf(stderr, "Watchdog card enabled.\n");
fflush(stderr);
goto end;
+ } else if (!strncasecmp(argv[1], "-t", 2)) {
+ flags = atoi(argv[2]);
+ ioctl(fd, WDIOC_SETTIMEOUT, &flags);
+ fprintf(stderr, "Watchdog timeout set to %d seconds.\n", flags);
+ fflush(stderr);
+ goto end;
} else {
fprintf(stderr, "-d to disable, -e to enable.\n");
fprintf(stderr, "run by itself to tick the card.\n");
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 07/10] Documentation/watchdog: watchdog-test.c: Add support for changing timeout
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
It might be useful for the random watchdog developer to also test
changing the watchdog timeout. Therefore, change the test application
to also support changing timeout.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
Documentation/watchdog/src/watchdog-test.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/watchdog/src/watchdog-test.c b/Documentation/watchdog/src/watchdog-test.c
index 3da8229..fb20d70 100644
--- a/Documentation/watchdog/src/watchdog-test.c
+++ b/Documentation/watchdog/src/watchdog-test.c
@@ -63,6 +63,12 @@ int main(int argc, char *argv[])
fprintf(stderr, "Watchdog card enabled.\n");
fflush(stderr);
goto end;
+ } else if (!strncasecmp(argv[1], "-t", 2)) {
+ flags = atoi(argv[2]);
+ ioctl(fd, WDIOC_SETTIMEOUT, &flags);
+ fprintf(stderr, "Watchdog timeout set to %d seconds.\n", flags);
+ fflush(stderr);
+ goto end;
} else {
fprintf(stderr, "-d to disable, -e to enable.\n");
fprintf(stderr, "run by itself to tick the card.\n");
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 08/10] watchdog: at91sam9_wdt: Convert to use new watchdog core extensions
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
Fill in the new watchdog core parameters and call
watchdog_init_params() to let core know we are ready to support the
new core API extensions. This allows the ad hoc timer code to be
removed from the driver as the watchdog core takes care of petting the
driver as needed.
Tested-by: Wenyou Yang <wenyou.yang@atmel.com>
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/at91sam9_wdt.c | 73 +++++++++--------------------------------
drivers/watchdog/watchdog_dev.c | 3 +-
2 files changed, 18 insertions(+), 58 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 411b7a6..ef1e8d0a 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -29,7 +29,6 @@
#include <linux/types.h>
#include <linux/watchdog.h>
#include <linux/jiffies.h>
-#include <linux/timer.h>
#include <linux/bitops.h>
#include <linux/uaccess.h>
#include <linux/of.h>
@@ -83,11 +82,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
struct at91wdt {
struct watchdog_device wdd;
void __iomem *base;
- unsigned long next_heartbeat; /* the next_heartbeat for the timer */
- struct timer_list timer; /* The timer that pings the watchdog */
u32 mr;
u32 mr_mask;
- unsigned long heartbeat; /* WDT heartbeat in jiffies */
bool nowayout;
unsigned int irq;
};
@@ -115,39 +111,17 @@ static inline void at91_wdt_reset(struct at91wdt *wdt)
wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
}
-/*
- * Timer tick
- */
-static void at91_ping(unsigned long data)
-{
- struct at91wdt *wdt = (struct at91wdt *)data;
- if (time_before(jiffies, wdt->next_heartbeat) ||
- !watchdog_hw_active(&wdt->wdd)) {
- at91_wdt_reset(wdt);
- mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
- } else {
- pr_crit("I will reset your machine !\n");
- }
-}
-
static int at91_wdt_start(struct watchdog_device *wdd)
{
struct at91wdt *wdt = to_wdt(wdd);
- /* calculate when the next userspace timeout will be */
- wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
- return 0;
-}
-static int at91_wdt_stop(struct watchdog_device *wdd)
-{
- /* The watchdog timer hardware can not be stopped... */
+ at91_wdt_reset(wdt);
return 0;
}
-static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
+static int at91_wdt_stop(struct watchdog_device *wdd)
{
- wdd->timeout = new_timeout;
- return at91_wdt_start(wdd);
+ return -ENOTSUPP;
}
static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
@@ -196,11 +170,11 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
* it at the min_heartbeat period.
*/
if ((max_heartbeat / 4) >= min_heartbeat)
- wdt->heartbeat = max_heartbeat / 4;
+ wdt->wdd.hw_heartbeat = jiffies_to_msecs(max_heartbeat / 4);
else if ((max_heartbeat / 2) >= min_heartbeat)
- wdt->heartbeat = max_heartbeat / 2;
+ wdt->wdd.hw_heartbeat = jiffies_to_msecs(max_heartbeat / 2);
else
- wdt->heartbeat = min_heartbeat;
+ wdt->wdd.hw_heartbeat = jiffies_to_msecs(min_heartbeat);
if (max_heartbeat < min_heartbeat + 4)
dev_warn(dev,
@@ -220,47 +194,28 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
"watchdog already configured differently (mr = %x expecting %x)\n",
tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
- setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
-
- /*
- * Use min_heartbeat the first time to avoid spurious watchdog reset:
- * we don't know for how long the watchdog counter is running, and
- * - resetting it right now might trigger a watchdog fault reset
- * - waiting for heartbeat time might lead to a watchdog timeout
- * reset
- */
- mod_timer(&wdt->timer, jiffies + min_heartbeat);
+ watchdog_set_hw_active(&wdt->wdd, 1);
+ watchdog_init_params(&wdt->wdd, dev);
- /* Try to set timeout from device tree first */
- if (watchdog_init_timeout(&wdt->wdd, 0, dev))
- watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
err = watchdog_register_device(&wdt->wdd);
if (err)
- goto out_stop_timer;
-
- wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
+ return err;
return 0;
-
-out_stop_timer:
- del_timer(&wdt->timer);
- return err;
}
/* ......................................................................... */
static const struct watchdog_info at91_wdt_info = {
.identity = DRV_NAME,
- .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
- WDIOF_MAGICCLOSE,
+ .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
};
static const struct watchdog_ops at91_wdt_ops = {
.owner = THIS_MODULE,
.start = at91_wdt_start,
.stop = at91_wdt_stop,
- .set_timeout = at91_wdt_set_timeout,
};
#if defined(CONFIG_OF)
@@ -287,6 +242,8 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
}
}
+ wdt->wdd.hw_max_timeout = max * 1000;
+
min = secs_to_ticks(min);
max = secs_to_ticks(max);
@@ -345,7 +302,7 @@ static int __init at91wdt_probe(struct platform_device *pdev)
wdt->wdd.ops = &at91_wdt_ops;
wdt->wdd.timeout = WDT_HEARTBEAT;
wdt->wdd.min_timeout = 1;
- wdt->wdd.max_timeout = 0xFFFF;
+ wdt->wdd.hw_max_timeout = WDT_COUNTER_MAX_SECS * 1000;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
wdt->base = devm_ioremap_resource(&pdev->dev, r);
@@ -358,6 +315,9 @@ static int __init at91wdt_probe(struct platform_device *pdev)
return err;
}
+ if (heartbeat > 0)
+ wdt->wdd.timeout = heartbeat;
+
err = at91_wdt_init(pdev, wdt);
if (err)
return err;
@@ -376,7 +336,6 @@ static int __exit at91wdt_remove(struct platform_device *pdev)
watchdog_unregister_device(&wdt->wdd);
pr_warn("I quit now, hardware will probably reboot!\n");
- del_timer(&wdt->timer);
return 0;
}
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 0310eda..8f99020 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -78,7 +78,8 @@ static int watchdog_ping(struct watchdog_device *wddev)
if (wddev->hw_max_timeout &&
wddev->timeout * 1000 > wddev->hw_max_timeout) {
wddev->expires = jiffies + wddev->timeout * HZ;
- schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
}
out_ping:
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 08/10] watchdog: at91sam9_wdt: Convert to use new watchdog core extensions
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Fill in the new watchdog core parameters and call
watchdog_init_params() to let core know we are ready to support the
new core API extensions. This allows the ad hoc timer code to be
removed from the driver as the watchdog core takes care of petting the
driver as needed.
Tested-by: Wenyou Yang <wenyou.yang@atmel.com>
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/at91sam9_wdt.c | 73 +++++++++--------------------------------
drivers/watchdog/watchdog_dev.c | 3 +-
2 files changed, 18 insertions(+), 58 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 411b7a6..ef1e8d0a 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -29,7 +29,6 @@
#include <linux/types.h>
#include <linux/watchdog.h>
#include <linux/jiffies.h>
-#include <linux/timer.h>
#include <linux/bitops.h>
#include <linux/uaccess.h>
#include <linux/of.h>
@@ -83,11 +82,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
struct at91wdt {
struct watchdog_device wdd;
void __iomem *base;
- unsigned long next_heartbeat; /* the next_heartbeat for the timer */
- struct timer_list timer; /* The timer that pings the watchdog */
u32 mr;
u32 mr_mask;
- unsigned long heartbeat; /* WDT heartbeat in jiffies */
bool nowayout;
unsigned int irq;
};
@@ -115,39 +111,17 @@ static inline void at91_wdt_reset(struct at91wdt *wdt)
wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
}
-/*
- * Timer tick
- */
-static void at91_ping(unsigned long data)
-{
- struct at91wdt *wdt = (struct at91wdt *)data;
- if (time_before(jiffies, wdt->next_heartbeat) ||
- !watchdog_hw_active(&wdt->wdd)) {
- at91_wdt_reset(wdt);
- mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
- } else {
- pr_crit("I will reset your machine !\n");
- }
-}
-
static int at91_wdt_start(struct watchdog_device *wdd)
{
struct at91wdt *wdt = to_wdt(wdd);
- /* calculate when the next userspace timeout will be */
- wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
- return 0;
-}
-static int at91_wdt_stop(struct watchdog_device *wdd)
-{
- /* The watchdog timer hardware can not be stopped... */
+ at91_wdt_reset(wdt);
return 0;
}
-static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
+static int at91_wdt_stop(struct watchdog_device *wdd)
{
- wdd->timeout = new_timeout;
- return at91_wdt_start(wdd);
+ return -ENOTSUPP;
}
static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
@@ -196,11 +170,11 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
* it at the min_heartbeat period.
*/
if ((max_heartbeat / 4) >= min_heartbeat)
- wdt->heartbeat = max_heartbeat / 4;
+ wdt->wdd.hw_heartbeat = jiffies_to_msecs(max_heartbeat / 4);
else if ((max_heartbeat / 2) >= min_heartbeat)
- wdt->heartbeat = max_heartbeat / 2;
+ wdt->wdd.hw_heartbeat = jiffies_to_msecs(max_heartbeat / 2);
else
- wdt->heartbeat = min_heartbeat;
+ wdt->wdd.hw_heartbeat = jiffies_to_msecs(min_heartbeat);
if (max_heartbeat < min_heartbeat + 4)
dev_warn(dev,
@@ -220,47 +194,28 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
"watchdog already configured differently (mr = %x expecting %x)\n",
tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
- setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
-
- /*
- * Use min_heartbeat the first time to avoid spurious watchdog reset:
- * we don't know for how long the watchdog counter is running, and
- * - resetting it right now might trigger a watchdog fault reset
- * - waiting for heartbeat time might lead to a watchdog timeout
- * reset
- */
- mod_timer(&wdt->timer, jiffies + min_heartbeat);
+ watchdog_set_hw_active(&wdt->wdd, 1);
+ watchdog_init_params(&wdt->wdd, dev);
- /* Try to set timeout from device tree first */
- if (watchdog_init_timeout(&wdt->wdd, 0, dev))
- watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
err = watchdog_register_device(&wdt->wdd);
if (err)
- goto out_stop_timer;
-
- wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
+ return err;
return 0;
-
-out_stop_timer:
- del_timer(&wdt->timer);
- return err;
}
/* ......................................................................... */
static const struct watchdog_info at91_wdt_info = {
.identity = DRV_NAME,
- .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
- WDIOF_MAGICCLOSE,
+ .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
};
static const struct watchdog_ops at91_wdt_ops = {
.owner = THIS_MODULE,
.start = at91_wdt_start,
.stop = at91_wdt_stop,
- .set_timeout = at91_wdt_set_timeout,
};
#if defined(CONFIG_OF)
@@ -287,6 +242,8 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
}
}
+ wdt->wdd.hw_max_timeout = max * 1000;
+
min = secs_to_ticks(min);
max = secs_to_ticks(max);
@@ -345,7 +302,7 @@ static int __init at91wdt_probe(struct platform_device *pdev)
wdt->wdd.ops = &at91_wdt_ops;
wdt->wdd.timeout = WDT_HEARTBEAT;
wdt->wdd.min_timeout = 1;
- wdt->wdd.max_timeout = 0xFFFF;
+ wdt->wdd.hw_max_timeout = WDT_COUNTER_MAX_SECS * 1000;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
wdt->base = devm_ioremap_resource(&pdev->dev, r);
@@ -358,6 +315,9 @@ static int __init at91wdt_probe(struct platform_device *pdev)
return err;
}
+ if (heartbeat > 0)
+ wdt->wdd.timeout = heartbeat;
+
err = at91_wdt_init(pdev, wdt);
if (err)
return err;
@@ -376,7 +336,6 @@ static int __exit at91wdt_remove(struct platform_device *pdev)
watchdog_unregister_device(&wdt->wdd);
pr_warn("I quit now, hardware will probably reboot!\n");
- del_timer(&wdt->timer);
return 0;
}
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 0310eda..8f99020 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -78,7 +78,8 @@ static int watchdog_ping(struct watchdog_device *wddev)
if (wddev->hw_max_timeout &&
wddev->timeout * 1000 > wddev->hw_max_timeout) {
wddev->expires = jiffies + wddev->timeout * HZ;
- schedule_delayed_work(&wddev->work, wddev->hw_heartbeat);
+ schedule_delayed_work(&wddev->work,
+ msecs_to_jiffies(wddev->hw_heartbeat));
}
out_ping:
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 09/10] watchdog: imx2_wdt: Convert to use new core extensions
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
Fill in the HW capabilities in watchdog_device structure and call
watchdgog_init_params() to let watchdog core to init itself
properly. The watchdog core can then ping stopped watchdog and the
timer code in the driver can be removed.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/imx2_wdt.c | 49 ++++++++++-----------------------------------
1 file changed, 11 insertions(+), 38 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 48fcce8..7af4d0f 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -62,7 +62,6 @@
struct imx2_wdt_device {
struct clk *clk;
struct regmap *regmap;
- struct timer_list timer; /* Pings the watchdog when closed */
struct watchdog_device wdog;
struct notifier_block restart_handler;
};
@@ -151,21 +150,13 @@ static int imx2_wdt_ping(struct watchdog_device *wdog)
return 0;
}
-static void imx2_wdt_timer_ping(unsigned long arg)
-{
- struct watchdog_device *wdog = (struct watchdog_device *)arg;
- struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
-
- /* ping it every wdog->timeout / 2 seconds to prevent reboot */
- imx2_wdt_ping(wdog);
- mod_timer(&wdev->timer, jiffies + wdog->timeout * HZ / 2);
-}
-
static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int new_timeout)
{
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
+ wdog->hw_heartbeat = new_timeout / 2 * 1000;
+ wdog->timeout = new_timeout;
regmap_update_bits(wdev->regmap, IMX2_WDT_WCR, IMX2_WDT_WCR_WT,
WDOG_SEC_TO_COUNT(new_timeout));
return 0;
@@ -176,8 +167,6 @@ static int imx2_wdt_start(struct watchdog_device *wdog)
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
if (imx2_wdt_is_running(wdev)) {
- /* delete the timer that pings the watchdog after close */
- del_timer_sync(&wdev->timer);
imx2_wdt_set_timeout(wdog, wdog->timeout);
} else
imx2_wdt_setup(wdog);
@@ -187,12 +176,7 @@ static int imx2_wdt_start(struct watchdog_device *wdog)
static int imx2_wdt_stop(struct watchdog_device *wdog)
{
- /*
- * We don't need a clk_disable, it cannot be disabled once started.
- * We use a timer to ping the watchdog while /dev/watchdog is closed
- */
- imx2_wdt_timer_ping((unsigned long)wdog);
- return 0;
+ return -ENOTSUPP;
}
static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog)
@@ -201,7 +185,7 @@ static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog)
if (imx2_wdt_is_running(wdev)) {
imx2_wdt_set_timeout(wdog, wdog->timeout);
- imx2_wdt_timer_ping((unsigned long)wdog);
+ imx2_wdt_ping(wdog);
}
}
@@ -256,6 +240,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
wdog->ops = &imx2_wdt_ops;
wdog->min_timeout = 1;
wdog->max_timeout = IMX2_WDT_MAX_TIME;
+ wdog->hw_max_timeout = wdog->max_timeout * 1000;
+ watchdog_set_hw_active(wdog, imx2_wdt_is_running(wdev));
clk_prepare_enable(wdev->clk);
@@ -267,12 +253,12 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n",
timeout, wdog->timeout);
+ wdog->timeout = timeout;
+ wdog->hw_heartbeat = timeout * 1000 / 2;
platform_set_drvdata(pdev, wdog);
watchdog_set_drvdata(wdog, wdev);
watchdog_set_nowayout(wdog, nowayout);
- watchdog_init_timeout(wdog, timeout, &pdev->dev);
-
- setup_timer(&wdev->timer, imx2_wdt_timer_ping, (unsigned long)wdog);
+ watchdog_init_params(wdog, &pdev->dev);
imx2_wdt_ping_if_active(wdog);
@@ -311,7 +297,6 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
watchdog_unregister_device(wdog);
if (imx2_wdt_is_running(wdev)) {
- del_timer_sync(&wdev->timer);
imx2_wdt_ping(wdog);
dev_crit(&pdev->dev, "Device removed: Expect reboot!\n");
}
@@ -325,10 +310,9 @@ static void imx2_wdt_shutdown(struct platform_device *pdev)
if (imx2_wdt_is_running(wdev)) {
/*
- * We are running, we need to delete the timer but will
- * give max timeout before reboot will take place
+ * We are running, give max timeout before reboot will
+ * take place
*/
- del_timer_sync(&wdev->timer);
imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
imx2_wdt_ping(wdog);
dev_crit(&pdev->dev, "Device shutdown: Expect reboot!\n");
@@ -346,10 +330,6 @@ static int imx2_wdt_suspend(struct device *dev)
if (imx2_wdt_is_running(wdev)) {
imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
imx2_wdt_ping(wdog);
-
- /* The watchdog is not active */
- if (!watchdog_hw_active(wdog))
- del_timer_sync(&wdev->timer);
}
clk_disable_unprepare(wdev->clk);
@@ -378,13 +358,6 @@ static int imx2_wdt_resume(struct device *dev)
/* Resuming from non-deep sleep state. */
imx2_wdt_set_timeout(wdog, wdog->timeout);
imx2_wdt_ping(wdog);
- /*
- * But the watchdog is not active, then start
- * the timer again.
- */
- if (!watchdog_hw_active(wdog))
- mod_timer(&wdev->timer,
- jiffies + wdog->timeout * HZ / 2);
}
return 0;
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 09/10] watchdog: imx2_wdt: Convert to use new core extensions
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Fill in the HW capabilities in watchdog_device structure and call
watchdgog_init_params() to let watchdog core to init itself
properly. The watchdog core can then ping stopped watchdog and the
timer code in the driver can be removed.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/imx2_wdt.c | 49 ++++++++++-----------------------------------
1 file changed, 11 insertions(+), 38 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 48fcce8..7af4d0f 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -62,7 +62,6 @@
struct imx2_wdt_device {
struct clk *clk;
struct regmap *regmap;
- struct timer_list timer; /* Pings the watchdog when closed */
struct watchdog_device wdog;
struct notifier_block restart_handler;
};
@@ -151,21 +150,13 @@ static int imx2_wdt_ping(struct watchdog_device *wdog)
return 0;
}
-static void imx2_wdt_timer_ping(unsigned long arg)
-{
- struct watchdog_device *wdog = (struct watchdog_device *)arg;
- struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
-
- /* ping it every wdog->timeout / 2 seconds to prevent reboot */
- imx2_wdt_ping(wdog);
- mod_timer(&wdev->timer, jiffies + wdog->timeout * HZ / 2);
-}
-
static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int new_timeout)
{
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
+ wdog->hw_heartbeat = new_timeout / 2 * 1000;
+ wdog->timeout = new_timeout;
regmap_update_bits(wdev->regmap, IMX2_WDT_WCR, IMX2_WDT_WCR_WT,
WDOG_SEC_TO_COUNT(new_timeout));
return 0;
@@ -176,8 +167,6 @@ static int imx2_wdt_start(struct watchdog_device *wdog)
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
if (imx2_wdt_is_running(wdev)) {
- /* delete the timer that pings the watchdog after close */
- del_timer_sync(&wdev->timer);
imx2_wdt_set_timeout(wdog, wdog->timeout);
} else
imx2_wdt_setup(wdog);
@@ -187,12 +176,7 @@ static int imx2_wdt_start(struct watchdog_device *wdog)
static int imx2_wdt_stop(struct watchdog_device *wdog)
{
- /*
- * We don't need a clk_disable, it cannot be disabled once started.
- * We use a timer to ping the watchdog while /dev/watchdog is closed
- */
- imx2_wdt_timer_ping((unsigned long)wdog);
- return 0;
+ return -ENOTSUPP;
}
static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog)
@@ -201,7 +185,7 @@ static inline void imx2_wdt_ping_if_active(struct watchdog_device *wdog)
if (imx2_wdt_is_running(wdev)) {
imx2_wdt_set_timeout(wdog, wdog->timeout);
- imx2_wdt_timer_ping((unsigned long)wdog);
+ imx2_wdt_ping(wdog);
}
}
@@ -256,6 +240,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
wdog->ops = &imx2_wdt_ops;
wdog->min_timeout = 1;
wdog->max_timeout = IMX2_WDT_MAX_TIME;
+ wdog->hw_max_timeout = wdog->max_timeout * 1000;
+ watchdog_set_hw_active(wdog, imx2_wdt_is_running(wdev));
clk_prepare_enable(wdev->clk);
@@ -267,12 +253,12 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n",
timeout, wdog->timeout);
+ wdog->timeout = timeout;
+ wdog->hw_heartbeat = timeout * 1000 / 2;
platform_set_drvdata(pdev, wdog);
watchdog_set_drvdata(wdog, wdev);
watchdog_set_nowayout(wdog, nowayout);
- watchdog_init_timeout(wdog, timeout, &pdev->dev);
-
- setup_timer(&wdev->timer, imx2_wdt_timer_ping, (unsigned long)wdog);
+ watchdog_init_params(wdog, &pdev->dev);
imx2_wdt_ping_if_active(wdog);
@@ -311,7 +297,6 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
watchdog_unregister_device(wdog);
if (imx2_wdt_is_running(wdev)) {
- del_timer_sync(&wdev->timer);
imx2_wdt_ping(wdog);
dev_crit(&pdev->dev, "Device removed: Expect reboot!\n");
}
@@ -325,10 +310,9 @@ static void imx2_wdt_shutdown(struct platform_device *pdev)
if (imx2_wdt_is_running(wdev)) {
/*
- * We are running, we need to delete the timer but will
- * give max timeout before reboot will take place
+ * We are running, give max timeout before reboot will
+ * take place
*/
- del_timer_sync(&wdev->timer);
imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
imx2_wdt_ping(wdog);
dev_crit(&pdev->dev, "Device shutdown: Expect reboot!\n");
@@ -346,10 +330,6 @@ static int imx2_wdt_suspend(struct device *dev)
if (imx2_wdt_is_running(wdev)) {
imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
imx2_wdt_ping(wdog);
-
- /* The watchdog is not active */
- if (!watchdog_hw_active(wdog))
- del_timer_sync(&wdev->timer);
}
clk_disable_unprepare(wdev->clk);
@@ -378,13 +358,6 @@ static int imx2_wdt_resume(struct device *dev)
/* Resuming from non-deep sleep state. */
imx2_wdt_set_timeout(wdog, wdog->timeout);
imx2_wdt_ping(wdog);
- /*
- * But the watchdog is not active, then start
- * the timer again.
- */
- if (!watchdog_hw_active(wdog))
- mod_timer(&wdev->timer,
- jiffies + wdog->timeout * HZ / 2);
}
return 0;
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 10/10] watchdog: omap_wdt: Convert to use new core extensions
2015-05-19 8:25 ` Timo Kokkonen
@ 2015-05-19 8:26 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-watchdog, boris.brezillon, nicolas.ferre,
alexandre.belloni
Cc: Wenyou.Yang, Timo Kokkonen
Use the new watchdog core extensions to let watchdog core take over
boot time watchdog behavior. The difference is that early-timeout-sec
device tree property becomes available for this driver and a running
watchdog is not stopped unless the core decides to stop it.
Omap watchdog is running by default in the boot up but bootloader
might have stopped it. Therefore we will set the watchdog_active flag
depending on the actual watchdog state so that the watchdog core can
act properly depending on the watchdog policy.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/omap_wdt.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 1e6be9e..fc230b0 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -78,6 +78,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
/* reloaded WCRR from WLDR */
}
+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+ void __iomem *base = wdev->base;
+
+ return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}
+
static void omap_wdt_enable(struct omap_wdt_dev *wdev)
{
void __iomem *base = wdev->base;
@@ -183,6 +190,7 @@ static int omap_wdt_set_timeout(struct watchdog_device *wdog,
omap_wdt_enable(wdev);
omap_wdt_reload(wdev);
wdog->timeout = timeout;
+ wdog->hw_heartbeat = timeout * 1000;
mutex_unlock(&wdev->lock);
return 0;
@@ -233,12 +241,14 @@ static int omap_wdt_probe(struct platform_device *pdev)
omap_wdt->ops = &omap_wdt_ops;
omap_wdt->min_timeout = TIMER_MARGIN_MIN;
omap_wdt->max_timeout = TIMER_MARGIN_MAX;
+ omap_wdt->hw_max_timeout = TIMER_MARGIN_MAX * 1000;
if (timer_margin >= TIMER_MARGIN_MIN &&
timer_margin <= TIMER_MARGIN_MAX)
omap_wdt->timeout = timer_margin;
else
omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
+ omap_wdt->hw_heartbeat = omap_wdt->timeout / 2 * 1000;
watchdog_set_drvdata(omap_wdt, wdev);
watchdog_set_nowayout(omap_wdt, nowayout);
@@ -248,6 +258,18 @@ static int omap_wdt_probe(struct platform_device *pdev)
pm_runtime_enable(wdev->dev);
pm_runtime_get_sync(wdev->dev);
+ if (omap_wdt_is_running(wdev))
+ watchdog_set_hw_active(omap_wdt, 1);
+ else
+ /*
+ * The watchdog should be stopped already by
+ * bootloader. But unless we call disable here, the
+ * timeout might not be set correctly on the first
+ * start. So call disable anyway to make sure the
+ * watchdog really is stopped properly.
+ */
+ omap_wdt_disable(wdev);
+
if (pdata && pdata->read_reset_sources)
rs = pdata->read_reset_sources();
else
@@ -255,7 +277,9 @@ static int omap_wdt_probe(struct platform_device *pdev)
omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
WDIOF_CARDRESET : 0;
- omap_wdt_disable(wdev);
+ ret = watchdog_init_params(omap_wdt, &pdev->dev);
+ if (ret)
+ return ret;
ret = watchdog_register_device(omap_wdt);
if (ret) {
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCHv8 10/10] watchdog: omap_wdt: Convert to use new core extensions
@ 2015-05-19 8:26 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-19 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Use the new watchdog core extensions to let watchdog core take over
boot time watchdog behavior. The difference is that early-timeout-sec
device tree property becomes available for this driver and a running
watchdog is not stopped unless the core decides to stop it.
Omap watchdog is running by default in the boot up but bootloader
might have stopped it. Therefore we will set the watchdog_active flag
depending on the actual watchdog state so that the watchdog core can
act properly depending on the watchdog policy.
Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
---
drivers/watchdog/omap_wdt.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 1e6be9e..fc230b0 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -78,6 +78,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
/* reloaded WCRR from WLDR */
}
+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
+{
+ void __iomem *base = wdev->base;
+
+ return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
+}
+
static void omap_wdt_enable(struct omap_wdt_dev *wdev)
{
void __iomem *base = wdev->base;
@@ -183,6 +190,7 @@ static int omap_wdt_set_timeout(struct watchdog_device *wdog,
omap_wdt_enable(wdev);
omap_wdt_reload(wdev);
wdog->timeout = timeout;
+ wdog->hw_heartbeat = timeout * 1000;
mutex_unlock(&wdev->lock);
return 0;
@@ -233,12 +241,14 @@ static int omap_wdt_probe(struct platform_device *pdev)
omap_wdt->ops = &omap_wdt_ops;
omap_wdt->min_timeout = TIMER_MARGIN_MIN;
omap_wdt->max_timeout = TIMER_MARGIN_MAX;
+ omap_wdt->hw_max_timeout = TIMER_MARGIN_MAX * 1000;
if (timer_margin >= TIMER_MARGIN_MIN &&
timer_margin <= TIMER_MARGIN_MAX)
omap_wdt->timeout = timer_margin;
else
omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
+ omap_wdt->hw_heartbeat = omap_wdt->timeout / 2 * 1000;
watchdog_set_drvdata(omap_wdt, wdev);
watchdog_set_nowayout(omap_wdt, nowayout);
@@ -248,6 +258,18 @@ static int omap_wdt_probe(struct platform_device *pdev)
pm_runtime_enable(wdev->dev);
pm_runtime_get_sync(wdev->dev);
+ if (omap_wdt_is_running(wdev))
+ watchdog_set_hw_active(omap_wdt, 1);
+ else
+ /*
+ * The watchdog should be stopped already by
+ * bootloader. But unless we call disable here, the
+ * timeout might not be set correctly on the first
+ * start. So call disable anyway to make sure the
+ * watchdog really is stopped properly.
+ */
+ omap_wdt_disable(wdev);
+
if (pdata && pdata->read_reset_sources)
rs = pdata->read_reset_sources();
else
@@ -255,7 +277,9 @@ static int omap_wdt_probe(struct platform_device *pdev)
omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
WDIOF_CARDRESET : 0;
- omap_wdt_disable(wdev);
+ ret = watchdog_init_params(omap_wdt, &pdev->dev);
+ if (ret)
+ return ret;
ret = watchdog_register_device(omap_wdt);
if (ret) {
--
2.1.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
2015-05-19 8:26 ` Timo Kokkonen
@ 2015-05-20 1:10 ` Guenter Roeck
-1 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:10 UTC (permalink / raw)
To: Timo Kokkonen, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> Before extending the watchdog core midlayer, it is useful to rename
> the watchdog_active function so that it states explicitly what it
> really does. That is, "active" watchdog means really that the watchdog
> hardware is running and needs pinging to prevent a watchdog reset
> taking place in near future.
>
> This is different to "watchdog open" state, which simply states that
> kernel is expecting the user space to keep the watchdog alive. These
> states might become different mainly because some hardware have
> limitations that prevent them from being stopped at will.
>
I don't see why this is needed. If you need another state, per your
description, it would be "open" in addition to "active".
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
@ 2015-05-20 1:10 ` Guenter Roeck
0 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:10 UTC (permalink / raw)
To: linux-arm-kernel
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> Before extending the watchdog core midlayer, it is useful to rename
> the watchdog_active function so that it states explicitly what it
> really does. That is, "active" watchdog means really that the watchdog
> hardware is running and needs pinging to prevent a watchdog reset
> taking place in near future.
>
> This is different to "watchdog open" state, which simply states that
> kernel is expecting the user space to keep the watchdog alive. These
> states might become different mainly because some hardware have
> limitations that prevent them from being stopped at will.
>
I don't see why this is needed. If you need another state, per your
description, it would be "open" in addition to "active".
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 03/10] watchdog: core: Introduce default watchdog policy
2015-05-19 8:26 ` Timo Kokkonen
@ 2015-05-20 1:13 ` Guenter Roeck
-1 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:13 UTC (permalink / raw)
To: Timo Kokkonen, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> Traditionally there have been no such thing as a policy for
> watchdogs. The watchdog timer was simply explicitly stopped at driver
> probe time and there was nothing else to do. Now that we have ability
> to work around HW disabilities in the watchdog core, it is time to
> introduce more policy options.
>
> The default option is to shut down the watchdog driver, as
> before. This ensures backward compatibility with current kernels and
> hardware that might have a watchdog, but user has not set up a
> watchdog daemon at all.
>
> A new option is introduced to keep the watchdog running or start it up
> at boot up. This is useful on many production systems that rely on the
> watchdog to reboot the device in case a crash. With this option even
> early kernel crashes or early user space crashes will lead to a reboot
> even though watchdog daemon has not opened and started the watchdog
> device yet.
>
> The third introduced option is to not touch the hardware at all in
> case bootloader have made an intelligent decision about the watchdog
> state and does not know how to tell kernel about it. The watchdog
> state is not touched until userspace takes over it.
>
> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
Dislike. I don't think it is a good idea to have configuration options
like this. This means users will have to deal with those "default"
options for an entire distribution, which I don't think is desirable.
Maybe for you, but not for everyone, and definitely not for me.
I think you are trying to do too much in your patch set.
Can we possibly focus on getting early timeout to an acceptable form
instead of trying to do everything at the same time.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 03/10] watchdog: core: Introduce default watchdog policy
@ 2015-05-20 1:13 ` Guenter Roeck
0 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:13 UTC (permalink / raw)
To: linux-arm-kernel
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> Traditionally there have been no such thing as a policy for
> watchdogs. The watchdog timer was simply explicitly stopped at driver
> probe time and there was nothing else to do. Now that we have ability
> to work around HW disabilities in the watchdog core, it is time to
> introduce more policy options.
>
> The default option is to shut down the watchdog driver, as
> before. This ensures backward compatibility with current kernels and
> hardware that might have a watchdog, but user has not set up a
> watchdog daemon at all.
>
> A new option is introduced to keep the watchdog running or start it up
> at boot up. This is useful on many production systems that rely on the
> watchdog to reboot the device in case a crash. With this option even
> early kernel crashes or early user space crashes will lead to a reboot
> even though watchdog daemon has not opened and started the watchdog
> device yet.
>
> The third introduced option is to not touch the hardware at all in
> case bootloader have made an intelligent decision about the watchdog
> state and does not know how to tell kernel about it. The watchdog
> state is not touched until userspace takes over it.
>
> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
Dislike. I don't think it is a good idea to have configuration options
like this. This means users will have to deal with those "default"
options for an entire distribution, which I don't think is desirable.
Maybe for you, but not for everyone, and definitely not for me.
I think you are trying to do too much in your patch set.
Can we possibly focus on getting early timeout to an acceptable form
instead of trying to do everything at the same time.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 04/10] watchdog: core: Allow extending early timeout
2015-05-19 8:26 ` Timo Kokkonen
@ 2015-05-20 1:16 ` Guenter Roeck
-1 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:16 UTC (permalink / raw)
To: Timo Kokkonen, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> It is possible that the specified watchdog timeout value is shorter
> than the time it takes for the watchdog daemon to start up and take
> over the watchdog device. This is problem if WATCHDOG_BOOT_RUNNING is
> the desired watchdog policy.
>
> Add a new timeout value that specifies how long kernel should extend
> the timeout by pinging the watchdog hardware. This option still
> ensures a watchdog reset takes place in case watchdog daemon fails to
> open the device, but gives more freedom in case user space is slow
> starting up and watchdog might trigger prematurely.
>
Same as my other comment. This should not be a configuration option
(just like the default timeout isn't one).
I really think we are going into the wrong direction. I can understand
the point why it makes sense to have a devicetree parameter for this,
and we should focus (have focused) on getting it accepted. Now we have
to deal with kernel configuration options, and we get more and more
distracted from the real goal.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 04/10] watchdog: core: Allow extending early timeout
@ 2015-05-20 1:16 ` Guenter Roeck
0 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:16 UTC (permalink / raw)
To: linux-arm-kernel
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> It is possible that the specified watchdog timeout value is shorter
> than the time it takes for the watchdog daemon to start up and take
> over the watchdog device. This is problem if WATCHDOG_BOOT_RUNNING is
> the desired watchdog policy.
>
> Add a new timeout value that specifies how long kernel should extend
> the timeout by pinging the watchdog hardware. This option still
> ensures a watchdog reset takes place in case watchdog daemon fails to
> open the device, but gives more freedom in case user space is slow
> starting up and watchdog might trigger prematurely.
>
Same as my other comment. This should not be a configuration option
(just like the default timeout isn't one).
I really think we are going into the wrong direction. I can understand
the point why it makes sense to have a devicetree parameter for this,
and we should focus (have focused) on getting it accepted. Now we have
to deal with kernel configuration options, and we get more and more
distracted from the real goal.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 02/10] watchdog: core: Ping watchdog on behalf of user space when needed
2015-05-19 8:26 ` Timo Kokkonen
@ 2015-05-20 1:22 ` Guenter Roeck
-1 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:22 UTC (permalink / raw)
To: Timo Kokkonen, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> Many watchdogs are not stoppable on the hardware level at all. Some
> others have a very short maximum timeout value. Both of these limits
> are problematic to the userspace and watchdog drivers have been
> traditionally implementing workarounds of their own in order to
> overcome the limitations. This obviously results in duplicate code in
> the driver level and makes it harder to implement generic hardware
> independent features.
>
> Extend the watchdog core by allowing it do the most common tasks on
> behalf of the driver. For this to work the watchdog core needs two new
> values from the driver, hw_max_timeout and hw_heartbeat. The first one
> tells the core what is the maximum supported timeout value in the
> hardware and the second one tells the preferred heartbeat value for
> the hardware. Both values are in milliseconds.
>
> The driver needs to also call watchdog_init_params() in order to let
> watchdog core fill in default watchdog paramters and let the core
> check the validity of the values.
>
> The watchdog core api changes slightly because of this change. Most
> importantly, the watchdog core now stands out as a clear midlayer
> between the driver and the user space. That is, the hw_max_timeout and
> hw_heartbeat values are meant to be filled in by the driver for the
> core. They will not be visible to user space any way. The timeout
> variable however is part of user space API. The comments in watchdog.h
> are changed also to reflect more clearly the difference. The
> max_timeout also becomes obsolete as the worker can support arbitrary
> timeout values.
>
> As long as we have still old drivers that don't tell the core about
> their hw capabilities, we need to support the old driver handling
> too. Add watchdog_needs_legacy_handling() function to watchdog.h so we
> can implement easily the old driver behavior and it becomes clear from
> the code which parts can be removed once all existing drivers supply
> the new parameters to watchdog core.
>
> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
> ---
> drivers/watchdog/watchdog_core.c | 124 ++++++++++++++++++++++++++++++++++++++-
> drivers/watchdog/watchdog_dev.c | 105 ++++++++++++++++++++++++++-------
> include/linux/watchdog.h | 64 ++++++++++++++++++--
> 3 files changed, 265 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..16e10e0 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -45,6 +45,9 @@ static struct class *watchdog_class;
>
> static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> {
> + /* Newer drivers don't need this check any more */
> + if (!watchdog_needs_legacy_handling(wdd))
> + return;
Something is conceptually wrong here.
> /*
> * Check that we have valid min and max timeout values, if
> * not reset them both to 0 (=not used or unknown)
> @@ -98,6 +101,110 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> }
> EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>
> +static void watchdog_set_default_timeout(struct watchdog_device *wdd,
> + struct device *dev)
> +{
> + int ret;
> + /*
> + * If driver has already set up a timeout, eg. from a module
> + * parameter, no need to do anything here
> + */
> + if (!watchdog_timeout_invalid(wdd, wdd->timeout))
> + return;
> +
> + /* Query device tree */
> + if (dev && dev->of_node) {
> + ret = of_property_read_u32(dev->of_node, "timeout-sec",
> + &wdd->timeout);
> + if (!ret && !watchdog_timeout_invalid(wdd, wdd->timeout))
> + return;
> + }
> +
> + /*
> + * If no other preference is given, use 60 seconds as the
> + * default timeout value
> + */
> + wdd->timeout = 60;
> +}
> +
> +/**
> + * watchdog_init_parms() - initialize generic watchdog parameters
> + * @wdd: Watchdog device to operate
> + * @dev: Device that stores the device tree properties
> + *
> + * Initialize the generic watchdog parameters. At least hw_max_timeout
> + * must be set prior calling this function. Other unset parameters are
> + * set based on information gathered from different sources (kernel
> + * config, device tree, ...) or set up with a reasonable defaults.
> + *
> + * A zero is returned on success and -EINVAL for failure.
> + */
> +int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
> +{
> + int ret = 0;
> +
> + watchdog_set_default_timeout(wdd, dev);
> +
> + if (wdd->max_timeout)
> + dev_err(dev, "Driver setting deprecated max_timeout, please fix\n");
> +
> + if (!wdd->hw_max_timeout)
> + return -EINVAL;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(watchdog_init_params);
> +
I really think things are going into the wrong direction. I prefer the approach
that is taken with the pretimeout introduction code, where we try to change
as little as possible. Specifically I dislike here that this function takes
parameters from wdd, instead of having function arguments. Again, the approach
used by the pretimeout code, where we introduce watchdog_init_timeouts() with
an additional parameter (while keeping the original watchdog_init_timeout API)
makes much more sense to me.
Overall I really think we should keep the changes minimalistic. This patch set
is growing larger and larger, and I see less and less benefit and more and
more changes.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 02/10] watchdog: core: Ping watchdog on behalf of user space when needed
@ 2015-05-20 1:22 ` Guenter Roeck
0 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 1:22 UTC (permalink / raw)
To: linux-arm-kernel
On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
> Many watchdogs are not stoppable on the hardware level at all. Some
> others have a very short maximum timeout value. Both of these limits
> are problematic to the userspace and watchdog drivers have been
> traditionally implementing workarounds of their own in order to
> overcome the limitations. This obviously results in duplicate code in
> the driver level and makes it harder to implement generic hardware
> independent features.
>
> Extend the watchdog core by allowing it do the most common tasks on
> behalf of the driver. For this to work the watchdog core needs two new
> values from the driver, hw_max_timeout and hw_heartbeat. The first one
> tells the core what is the maximum supported timeout value in the
> hardware and the second one tells the preferred heartbeat value for
> the hardware. Both values are in milliseconds.
>
> The driver needs to also call watchdog_init_params() in order to let
> watchdog core fill in default watchdog paramters and let the core
> check the validity of the values.
>
> The watchdog core api changes slightly because of this change. Most
> importantly, the watchdog core now stands out as a clear midlayer
> between the driver and the user space. That is, the hw_max_timeout and
> hw_heartbeat values are meant to be filled in by the driver for the
> core. They will not be visible to user space any way. The timeout
> variable however is part of user space API. The comments in watchdog.h
> are changed also to reflect more clearly the difference. The
> max_timeout also becomes obsolete as the worker can support arbitrary
> timeout values.
>
> As long as we have still old drivers that don't tell the core about
> their hw capabilities, we need to support the old driver handling
> too. Add watchdog_needs_legacy_handling() function to watchdog.h so we
> can implement easily the old driver behavior and it becomes clear from
> the code which parts can be removed once all existing drivers supply
> the new parameters to watchdog core.
>
> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
> ---
> drivers/watchdog/watchdog_core.c | 124 ++++++++++++++++++++++++++++++++++++++-
> drivers/watchdog/watchdog_dev.c | 105 ++++++++++++++++++++++++++-------
> include/linux/watchdog.h | 64 ++++++++++++++++++--
> 3 files changed, 265 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..16e10e0 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -45,6 +45,9 @@ static struct class *watchdog_class;
>
> static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> {
> + /* Newer drivers don't need this check any more */
> + if (!watchdog_needs_legacy_handling(wdd))
> + return;
Something is conceptually wrong here.
> /*
> * Check that we have valid min and max timeout values, if
> * not reset them both to 0 (=not used or unknown)
> @@ -98,6 +101,110 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> }
> EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>
> +static void watchdog_set_default_timeout(struct watchdog_device *wdd,
> + struct device *dev)
> +{
> + int ret;
> + /*
> + * If driver has already set up a timeout, eg. from a module
> + * parameter, no need to do anything here
> + */
> + if (!watchdog_timeout_invalid(wdd, wdd->timeout))
> + return;
> +
> + /* Query device tree */
> + if (dev && dev->of_node) {
> + ret = of_property_read_u32(dev->of_node, "timeout-sec",
> + &wdd->timeout);
> + if (!ret && !watchdog_timeout_invalid(wdd, wdd->timeout))
> + return;
> + }
> +
> + /*
> + * If no other preference is given, use 60 seconds as the
> + * default timeout value
> + */
> + wdd->timeout = 60;
> +}
> +
> +/**
> + * watchdog_init_parms() - initialize generic watchdog parameters
> + * @wdd: Watchdog device to operate
> + * @dev: Device that stores the device tree properties
> + *
> + * Initialize the generic watchdog parameters. At least hw_max_timeout
> + * must be set prior calling this function. Other unset parameters are
> + * set based on information gathered from different sources (kernel
> + * config, device tree, ...) or set up with a reasonable defaults.
> + *
> + * A zero is returned on success and -EINVAL for failure.
> + */
> +int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
> +{
> + int ret = 0;
> +
> + watchdog_set_default_timeout(wdd, dev);
> +
> + if (wdd->max_timeout)
> + dev_err(dev, "Driver setting deprecated max_timeout, please fix\n");
> +
> + if (!wdd->hw_max_timeout)
> + return -EINVAL;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(watchdog_init_params);
> +
I really think things are going into the wrong direction. I prefer the approach
that is taken with the pretimeout introduction code, where we try to change
as little as possible. Specifically I dislike here that this function takes
parameters from wdd, instead of having function arguments. Again, the approach
used by the pretimeout code, where we introduce watchdog_init_timeouts() with
an additional parameter (while keeping the original watchdog_init_timeout API)
makes much more sense to me.
Overall I really think we should keep the changes minimalistic. This patch set
is growing larger and larger, and I see less and less benefit and more and
more changes.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
2015-05-20 1:10 ` Guenter Roeck
@ 2015-05-20 5:37 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-20 5:37 UTC (permalink / raw)
To: Guenter Roeck, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 20.05.2015 04:10, Guenter Roeck wrote:
> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>> Before extending the watchdog core midlayer, it is useful to rename
>> the watchdog_active function so that it states explicitly what it
>> really does. That is, "active" watchdog means really that the watchdog
>> hardware is running and needs pinging to prevent a watchdog reset
>> taking place in near future.
>>
>> This is different to "watchdog open" state, which simply states that
>> kernel is expecting the user space to keep the watchdog alive. These
>> states might become different mainly because some hardware have
>> limitations that prevent them from being stopped at will.
>>
>
> I don't see why this is needed. If you need another state, per your
> description, it would be "open" in addition to "active".
Yes, the watchdog_is_open() is introduced on patch number two. The
original watchdog_is_active() is really confusing. It doesn't really
state what it means. Most of the drivers are using it to test whether
the watchdog HW is active when going to suspend, but at least atmel
watchdog was testing it to see whether the watchdog device is open from
user space. The HW itself is always active in that driver.
If we are about to distinguish between "device open from user space" and
"hardware timer running", we better be clear about the naming.
"watchdog_is_active" doesn't really tell what it does.
This was originally suggested by Uwe Kleine-König. He also recommended
changing the timeout parameter so that is would state more clearly that
it is the SW timeout and not HW timeout. But I felt that it would have
been too invasive to change the timeout parameter as well. The
watchdog_is_active was not used very much so the change was easy.
-Timo
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
@ 2015-05-20 5:37 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-20 5:37 UTC (permalink / raw)
To: linux-arm-kernel
On 20.05.2015 04:10, Guenter Roeck wrote:
> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>> Before extending the watchdog core midlayer, it is useful to rename
>> the watchdog_active function so that it states explicitly what it
>> really does. That is, "active" watchdog means really that the watchdog
>> hardware is running and needs pinging to prevent a watchdog reset
>> taking place in near future.
>>
>> This is different to "watchdog open" state, which simply states that
>> kernel is expecting the user space to keep the watchdog alive. These
>> states might become different mainly because some hardware have
>> limitations that prevent them from being stopped at will.
>>
>
> I don't see why this is needed. If you need another state, per your
> description, it would be "open" in addition to "active".
Yes, the watchdog_is_open() is introduced on patch number two. The
original watchdog_is_active() is really confusing. It doesn't really
state what it means. Most of the drivers are using it to test whether
the watchdog HW is active when going to suspend, but at least atmel
watchdog was testing it to see whether the watchdog device is open from
user space. The HW itself is always active in that driver.
If we are about to distinguish between "device open from user space" and
"hardware timer running", we better be clear about the naming.
"watchdog_is_active" doesn't really tell what it does.
This was originally suggested by Uwe Kleine-K?nig. He also recommended
changing the timeout parameter so that is would state more clearly that
it is the SW timeout and not HW timeout. But I felt that it would have
been too invasive to change the timeout parameter as well. The
watchdog_is_active was not used very much so the change was easy.
-Timo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 03/10] watchdog: core: Introduce default watchdog policy
2015-05-20 1:13 ` Guenter Roeck
@ 2015-05-20 5:45 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-20 5:45 UTC (permalink / raw)
To: Guenter Roeck, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 20.05.2015 04:13, Guenter Roeck wrote:
> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>> Traditionally there have been no such thing as a policy for
>> watchdogs. The watchdog timer was simply explicitly stopped at driver
>> probe time and there was nothing else to do. Now that we have ability
>> to work around HW disabilities in the watchdog core, it is time to
>> introduce more policy options.
>>
>> The default option is to shut down the watchdog driver, as
>> before. This ensures backward compatibility with current kernels and
>> hardware that might have a watchdog, but user has not set up a
>> watchdog daemon at all.
>>
>> A new option is introduced to keep the watchdog running or start it up
>> at boot up. This is useful on many production systems that rely on the
>> watchdog to reboot the device in case a crash. With this option even
>> early kernel crashes or early user space crashes will lead to a reboot
>> even though watchdog daemon has not opened and started the watchdog
>> device yet.
>>
>> The third introduced option is to not touch the hardware at all in
>> case bootloader have made an intelligent decision about the watchdog
>> state and does not know how to tell kernel about it. The watchdog
>> state is not touched until userspace takes over it.
>>
>> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
>
> Dislike. I don't think it is a good idea to have configuration options
> like this. This means users will have to deal with those "default"
> options for an entire distribution, which I don't think is desirable.
> Maybe for you, but not for everyone, and definitely not for me.
The configuration option was suggested by Uwe Kleine-König. I think it
makes sense as there would not be otherwise any other way to enable the
early-timeout-sec as device tree. I would think that for example in some
server environment it would be good to enable watchdogs as early as
possible and there probably are no device tree available.
> I think you are trying to do too much in your patch set.
> Can we possibly focus on getting early timeout to an acceptable form
> instead of trying to do everything at the same time.
I would be happy to do it the way that guarantees the inclusion, but I
feel that I am getting conflicting feedback from different people giving
review. I appreciate any concrete guidance you can give about the
direction I should go with the patch set.
Thanks,
-Timo
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 03/10] watchdog: core: Introduce default watchdog policy
@ 2015-05-20 5:45 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-20 5:45 UTC (permalink / raw)
To: linux-arm-kernel
On 20.05.2015 04:13, Guenter Roeck wrote:
> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>> Traditionally there have been no such thing as a policy for
>> watchdogs. The watchdog timer was simply explicitly stopped at driver
>> probe time and there was nothing else to do. Now that we have ability
>> to work around HW disabilities in the watchdog core, it is time to
>> introduce more policy options.
>>
>> The default option is to shut down the watchdog driver, as
>> before. This ensures backward compatibility with current kernels and
>> hardware that might have a watchdog, but user has not set up a
>> watchdog daemon at all.
>>
>> A new option is introduced to keep the watchdog running or start it up
>> at boot up. This is useful on many production systems that rely on the
>> watchdog to reboot the device in case a crash. With this option even
>> early kernel crashes or early user space crashes will lead to a reboot
>> even though watchdog daemon has not opened and started the watchdog
>> device yet.
>>
>> The third introduced option is to not touch the hardware at all in
>> case bootloader have made an intelligent decision about the watchdog
>> state and does not know how to tell kernel about it. The watchdog
>> state is not touched until userspace takes over it.
>>
>> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
>
> Dislike. I don't think it is a good idea to have configuration options
> like this. This means users will have to deal with those "default"
> options for an entire distribution, which I don't think is desirable.
> Maybe for you, but not for everyone, and definitely not for me.
The configuration option was suggested by Uwe Kleine-K?nig. I think it
makes sense as there would not be otherwise any other way to enable the
early-timeout-sec as device tree. I would think that for example in some
server environment it would be good to enable watchdogs as early as
possible and there probably are no device tree available.
> I think you are trying to do too much in your patch set.
> Can we possibly focus on getting early timeout to an acceptable form
> instead of trying to do everything at the same time.
I would be happy to do it the way that guarantees the inclusion, but I
feel that I am getting conflicting feedback from different people giving
review. I appreciate any concrete guidance you can give about the
direction I should go with the patch set.
Thanks,
-Timo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 02/10] watchdog: core: Ping watchdog on behalf of user space when needed
2015-05-20 1:22 ` Guenter Roeck
@ 2015-05-20 6:18 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-20 6:18 UTC (permalink / raw)
To: Guenter Roeck, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 20.05.2015 04:22, Guenter Roeck wrote:
> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>> Many watchdogs are not stoppable on the hardware level at all. Some
>> others have a very short maximum timeout value. Both of these limits
>> are problematic to the userspace and watchdog drivers have been
>> traditionally implementing workarounds of their own in order to
>> overcome the limitations. This obviously results in duplicate code in
>> the driver level and makes it harder to implement generic hardware
>> independent features.
>>
>> Extend the watchdog core by allowing it do the most common tasks on
>> behalf of the driver. For this to work the watchdog core needs two new
>> values from the driver, hw_max_timeout and hw_heartbeat. The first one
>> tells the core what is the maximum supported timeout value in the
>> hardware and the second one tells the preferred heartbeat value for
>> the hardware. Both values are in milliseconds.
>>
>> The driver needs to also call watchdog_init_params() in order to let
>> watchdog core fill in default watchdog paramters and let the core
>> check the validity of the values.
>>
>> The watchdog core api changes slightly because of this change. Most
>> importantly, the watchdog core now stands out as a clear midlayer
>> between the driver and the user space. That is, the hw_max_timeout and
>> hw_heartbeat values are meant to be filled in by the driver for the
>> core. They will not be visible to user space any way. The timeout
>> variable however is part of user space API. The comments in watchdog.h
>> are changed also to reflect more clearly the difference. The
>> max_timeout also becomes obsolete as the worker can support arbitrary
>> timeout values.
>>
>> As long as we have still old drivers that don't tell the core about
>> their hw capabilities, we need to support the old driver handling
>> too. Add watchdog_needs_legacy_handling() function to watchdog.h so we
>> can implement easily the old driver behavior and it becomes clear from
>> the code which parts can be removed once all existing drivers supply
>> the new parameters to watchdog core.
>>
>> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
>> ---
>> drivers/watchdog/watchdog_core.c | 124
>> ++++++++++++++++++++++++++++++++++++++-
>> drivers/watchdog/watchdog_dev.c | 105
>> ++++++++++++++++++++++++++-------
>> include/linux/watchdog.h | 64 ++++++++++++++++++--
>> 3 files changed, 265 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_core.c
>> b/drivers/watchdog/watchdog_core.c
>> index cec9b55..16e10e0 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -45,6 +45,9 @@ static struct class *watchdog_class;
>>
>> static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>> {
>> + /* Newer drivers don't need this check any more */
>> + if (!watchdog_needs_legacy_handling(wdd))
>> + return;
>
> Something is conceptually wrong here.
>
The max timeout concept becomes obsolete once the watchdog core starts
intermediating between the driver and the user space and extend the
watchdog period as needed. Once you remove the max timeout from the
check, it becomes so trivial that I don't see point factoring it out as
a separate check. If you had something else on your mind, please elaborate.
>> /*
>> * Check that we have valid min and max timeout values, if
>> * not reset them both to 0 (=not used or unknown)
>> @@ -98,6 +101,110 @@ int watchdog_init_timeout(struct watchdog_device
>> *wdd,
>> }
>> EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>>
>> +static void watchdog_set_default_timeout(struct watchdog_device *wdd,
>> + struct device *dev)
>> +{
>> + int ret;
>> + /*
>> + * If driver has already set up a timeout, eg. from a module
>> + * parameter, no need to do anything here
>> + */
>> + if (!watchdog_timeout_invalid(wdd, wdd->timeout))
>> + return;
>> +
>> + /* Query device tree */
>> + if (dev && dev->of_node) {
>> + ret = of_property_read_u32(dev->of_node, "timeout-sec",
>> + &wdd->timeout);
>> + if (!ret && !watchdog_timeout_invalid(wdd, wdd->timeout))
>> + return;
>> + }
>> +
>> + /*
>> + * If no other preference is given, use 60 seconds as the
>> + * default timeout value
>> + */
>> + wdd->timeout = 60;
>> +}
>> +
>> +/**
>> + * watchdog_init_parms() - initialize generic watchdog parameters
>> + * @wdd: Watchdog device to operate
>> + * @dev: Device that stores the device tree properties
>> + *
>> + * Initialize the generic watchdog parameters. At least hw_max_timeout
>> + * must be set prior calling this function. Other unset parameters are
>> + * set based on information gathered from different sources (kernel
>> + * config, device tree, ...) or set up with a reasonable defaults.
>> + *
>> + * A zero is returned on success and -EINVAL for failure.
>> + */
>> +int watchdog_init_params(struct watchdog_device *wdd, struct device
>> *dev)
>> +{
>> + int ret = 0;
>> +
>> + watchdog_set_default_timeout(wdd, dev);
>> +
>> + if (wdd->max_timeout)
>> + dev_err(dev, "Driver setting deprecated max_timeout, please
>> fix\n");
>> +
>> + if (!wdd->hw_max_timeout)
>> + return -EINVAL;
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(watchdog_init_params);
>> +
>
> I really think things are going into the wrong direction. I prefer the
> approach
> that is taken with the pretimeout introduction code, where we try to change
> as little as possible. Specifically I dislike here that this function takes
> parameters from wdd, instead of having function arguments. Again, the
> approach
> used by the pretimeout code, where we introduce watchdog_init_timeouts()
> with
> an additional parameter (while keeping the original
> watchdog_init_timeout API)
> makes much more sense to me.
The reason why I replaced the watchdog_init_timeout() call with a new
one was that I give a chance to the watchdog core to do generic
initialization for the driver. The core would parse timeout and
early-timeout-sec parameters from device tree and possible do something
else that the driver does not need to know about. We could keep the
watchdog_init_timeout and add another call for parsing the
early-timeout-sec, but the timeout handling changes a bit too as the
concept of max timeout disappears.
And if we are going to change the drivers calling watchdog_init_timeout
anyway, why not think something that initializes all parameters in one
call? That's what I thought. We can do it on some other way, but now is
a chance to think about a bit how it should be done right. I'm open to
suggestions.
> Overall I really think we should keep the changes minimalistic. This
> patch set
> is growing larger and larger, and I see less and less benefit and more and
> more changes.
I have got feedback about what I should do and I have learned more about
the problems the current watchdog api has. My goal is to keep changes to
most of the drivers minimal, simply let core know about hw_max_timeout
and hw_heartbeat values. Some drivers can also remove a lot of timer
handling code that they no longer need to duplicate. The end result is
that watchdogs appear to userspace more as a unified subsystem instead
of a collection of separate drivers that all behave different to each
others. This is something that I think makes things better.
I hope you can give clear directions on where to go with the patches. I
have spent a lot of time with these now and I probably don't have that
luxury much longer any more, so I am all open in keeping the changes
minimalistic.
Thanks,
-Timo
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 02/10] watchdog: core: Ping watchdog on behalf of user space when needed
@ 2015-05-20 6:18 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-20 6:18 UTC (permalink / raw)
To: linux-arm-kernel
On 20.05.2015 04:22, Guenter Roeck wrote:
> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>> Many watchdogs are not stoppable on the hardware level at all. Some
>> others have a very short maximum timeout value. Both of these limits
>> are problematic to the userspace and watchdog drivers have been
>> traditionally implementing workarounds of their own in order to
>> overcome the limitations. This obviously results in duplicate code in
>> the driver level and makes it harder to implement generic hardware
>> independent features.
>>
>> Extend the watchdog core by allowing it do the most common tasks on
>> behalf of the driver. For this to work the watchdog core needs two new
>> values from the driver, hw_max_timeout and hw_heartbeat. The first one
>> tells the core what is the maximum supported timeout value in the
>> hardware and the second one tells the preferred heartbeat value for
>> the hardware. Both values are in milliseconds.
>>
>> The driver needs to also call watchdog_init_params() in order to let
>> watchdog core fill in default watchdog paramters and let the core
>> check the validity of the values.
>>
>> The watchdog core api changes slightly because of this change. Most
>> importantly, the watchdog core now stands out as a clear midlayer
>> between the driver and the user space. That is, the hw_max_timeout and
>> hw_heartbeat values are meant to be filled in by the driver for the
>> core. They will not be visible to user space any way. The timeout
>> variable however is part of user space API. The comments in watchdog.h
>> are changed also to reflect more clearly the difference. The
>> max_timeout also becomes obsolete as the worker can support arbitrary
>> timeout values.
>>
>> As long as we have still old drivers that don't tell the core about
>> their hw capabilities, we need to support the old driver handling
>> too. Add watchdog_needs_legacy_handling() function to watchdog.h so we
>> can implement easily the old driver behavior and it becomes clear from
>> the code which parts can be removed once all existing drivers supply
>> the new parameters to watchdog core.
>>
>> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
>> ---
>> drivers/watchdog/watchdog_core.c | 124
>> ++++++++++++++++++++++++++++++++++++++-
>> drivers/watchdog/watchdog_dev.c | 105
>> ++++++++++++++++++++++++++-------
>> include/linux/watchdog.h | 64 ++++++++++++++++++--
>> 3 files changed, 265 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_core.c
>> b/drivers/watchdog/watchdog_core.c
>> index cec9b55..16e10e0 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -45,6 +45,9 @@ static struct class *watchdog_class;
>>
>> static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>> {
>> + /* Newer drivers don't need this check any more */
>> + if (!watchdog_needs_legacy_handling(wdd))
>> + return;
>
> Something is conceptually wrong here.
>
The max timeout concept becomes obsolete once the watchdog core starts
intermediating between the driver and the user space and extend the
watchdog period as needed. Once you remove the max timeout from the
check, it becomes so trivial that I don't see point factoring it out as
a separate check. If you had something else on your mind, please elaborate.
>> /*
>> * Check that we have valid min and max timeout values, if
>> * not reset them both to 0 (=not used or unknown)
>> @@ -98,6 +101,110 @@ int watchdog_init_timeout(struct watchdog_device
>> *wdd,
>> }
>> EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>>
>> +static void watchdog_set_default_timeout(struct watchdog_device *wdd,
>> + struct device *dev)
>> +{
>> + int ret;
>> + /*
>> + * If driver has already set up a timeout, eg. from a module
>> + * parameter, no need to do anything here
>> + */
>> + if (!watchdog_timeout_invalid(wdd, wdd->timeout))
>> + return;
>> +
>> + /* Query device tree */
>> + if (dev && dev->of_node) {
>> + ret = of_property_read_u32(dev->of_node, "timeout-sec",
>> + &wdd->timeout);
>> + if (!ret && !watchdog_timeout_invalid(wdd, wdd->timeout))
>> + return;
>> + }
>> +
>> + /*
>> + * If no other preference is given, use 60 seconds as the
>> + * default timeout value
>> + */
>> + wdd->timeout = 60;
>> +}
>> +
>> +/**
>> + * watchdog_init_parms() - initialize generic watchdog parameters
>> + * @wdd: Watchdog device to operate
>> + * @dev: Device that stores the device tree properties
>> + *
>> + * Initialize the generic watchdog parameters. At least hw_max_timeout
>> + * must be set prior calling this function. Other unset parameters are
>> + * set based on information gathered from different sources (kernel
>> + * config, device tree, ...) or set up with a reasonable defaults.
>> + *
>> + * A zero is returned on success and -EINVAL for failure.
>> + */
>> +int watchdog_init_params(struct watchdog_device *wdd, struct device
>> *dev)
>> +{
>> + int ret = 0;
>> +
>> + watchdog_set_default_timeout(wdd, dev);
>> +
>> + if (wdd->max_timeout)
>> + dev_err(dev, "Driver setting deprecated max_timeout, please
>> fix\n");
>> +
>> + if (!wdd->hw_max_timeout)
>> + return -EINVAL;
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(watchdog_init_params);
>> +
>
> I really think things are going into the wrong direction. I prefer the
> approach
> that is taken with the pretimeout introduction code, where we try to change
> as little as possible. Specifically I dislike here that this function takes
> parameters from wdd, instead of having function arguments. Again, the
> approach
> used by the pretimeout code, where we introduce watchdog_init_timeouts()
> with
> an additional parameter (while keeping the original
> watchdog_init_timeout API)
> makes much more sense to me.
The reason why I replaced the watchdog_init_timeout() call with a new
one was that I give a chance to the watchdog core to do generic
initialization for the driver. The core would parse timeout and
early-timeout-sec parameters from device tree and possible do something
else that the driver does not need to know about. We could keep the
watchdog_init_timeout and add another call for parsing the
early-timeout-sec, but the timeout handling changes a bit too as the
concept of max timeout disappears.
And if we are going to change the drivers calling watchdog_init_timeout
anyway, why not think something that initializes all parameters in one
call? That's what I thought. We can do it on some other way, but now is
a chance to think about a bit how it should be done right. I'm open to
suggestions.
> Overall I really think we should keep the changes minimalistic. This
> patch set
> is growing larger and larger, and I see less and less benefit and more and
> more changes.
I have got feedback about what I should do and I have learned more about
the problems the current watchdog api has. My goal is to keep changes to
most of the drivers minimal, simply let core know about hw_max_timeout
and hw_heartbeat values. Some drivers can also remove a lot of timer
handling code that they no longer need to duplicate. The end result is
that watchdogs appear to userspace more as a unified subsystem instead
of a collection of separate drivers that all behave different to each
others. This is something that I think makes things better.
I hope you can give clear directions on where to go with the patches. I
have spent a lot of time with these now and I probably don't have that
luxury much longer any more, so I am all open in keeping the changes
minimalistic.
Thanks,
-Timo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
2015-05-20 5:37 ` Timo Kokkonen
@ 2015-05-20 13:46 ` Guenter Roeck
-1 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 13:46 UTC (permalink / raw)
To: Timo Kokkonen, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Cc: Wenyou.Yang
On 05/19/2015 10:37 PM, Timo Kokkonen wrote:
> On 20.05.2015 04:10, Guenter Roeck wrote:
>> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>>> Before extending the watchdog core midlayer, it is useful to rename
>>> the watchdog_active function so that it states explicitly what it
>>> really does. That is, "active" watchdog means really that the watchdog
>>> hardware is running and needs pinging to prevent a watchdog reset
>>> taking place in near future.
>>>
>>> This is different to "watchdog open" state, which simply states that
>>> kernel is expecting the user space to keep the watchdog alive. These
>>> states might become different mainly because some hardware have
>>> limitations that prevent them from being stopped at will.
>>>
>>
>> I don't see why this is needed. If you need another state, per your
>> description, it would be "open" in addition to "active".
>
> Yes, the watchdog_is_open() is introduced on patch number two. The original watchdog_is_active() is really confusing. It doesn't really state what it means. Most of the drivers are using it to test whether the watchdog HW is active when going to suspend, but at least atmel watchdog was testing it to see whether the watchdog device is open from user space. The HW itself is always active in that driver.
>
> If we are about to distinguish between "device open from user space" and "hardware timer running", we better be clear about the naming. "watchdog_is_active" doesn't really tell what it does.
>
> This was originally suggested by Uwe Kleine-König. He also recommended changing the timeout parameter so that is would state more clearly that it is the SW timeout and not HW timeout. But I felt that it would have been too invasive to change the timeout parameter as well. The watchdog_is_active was not used very much so the change was easy.
>
> -Timo
>
You could just clarify what it means.
Anyway, I think I'll have to step back from this for a while.
As I mentioned, I think it is getting too invasive, which clouds
my judgment. I think I'll leave this patch set up to Wim to handle.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
@ 2015-05-20 13:46 ` Guenter Roeck
0 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2015-05-20 13:46 UTC (permalink / raw)
To: linux-arm-kernel
On 05/19/2015 10:37 PM, Timo Kokkonen wrote:
> On 20.05.2015 04:10, Guenter Roeck wrote:
>> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>>> Before extending the watchdog core midlayer, it is useful to rename
>>> the watchdog_active function so that it states explicitly what it
>>> really does. That is, "active" watchdog means really that the watchdog
>>> hardware is running and needs pinging to prevent a watchdog reset
>>> taking place in near future.
>>>
>>> This is different to "watchdog open" state, which simply states that
>>> kernel is expecting the user space to keep the watchdog alive. These
>>> states might become different mainly because some hardware have
>>> limitations that prevent them from being stopped at will.
>>>
>>
>> I don't see why this is needed. If you need another state, per your
>> description, it would be "open" in addition to "active".
>
> Yes, the watchdog_is_open() is introduced on patch number two. The original watchdog_is_active() is really confusing. It doesn't really state what it means. Most of the drivers are using it to test whether the watchdog HW is active when going to suspend, but at least atmel watchdog was testing it to see whether the watchdog device is open from user space. The HW itself is always active in that driver.
>
> If we are about to distinguish between "device open from user space" and "hardware timer running", we better be clear about the naming. "watchdog_is_active" doesn't really tell what it does.
>
> This was originally suggested by Uwe Kleine-K?nig. He also recommended changing the timeout parameter so that is would state more clearly that it is the SW timeout and not HW timeout. But I felt that it would have been too invasive to change the timeout parameter as well. The watchdog_is_active was not used very much so the change was easy.
>
> -Timo
>
You could just clarify what it means.
Anyway, I think I'll have to step back from this for a while.
As I mentioned, I think it is getting too invasive, which clouds
my judgment. I think I'll leave this patch set up to Wim to handle.
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
2015-05-20 13:46 ` Guenter Roeck
@ 2015-05-29 12:43 ` Timo Kokkonen
-1 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-29 12:43 UTC (permalink / raw)
To: Guenter Roeck, linux-arm-kernel, linux-watchdog, boris.brezillon,
nicolas.ferre, alexandre.belloni
Hi,
Other work has begun piling on my desk, sorry I haven't had time to take
this any forward.
On 20.05.2015 16:46, Guenter Roeck wrote:
> On 05/19/2015 10:37 PM, Timo Kokkonen wrote:
>> On 20.05.2015 04:10, Guenter Roeck wrote:
>>> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>>>> Before extending the watchdog core midlayer, it is useful to rename
>>>> the watchdog_active function so that it states explicitly what it
>>>> really does. That is, "active" watchdog means really that the watchdog
>>>> hardware is running and needs pinging to prevent a watchdog reset
>>>> taking place in near future.
>>>>
>>>> This is different to "watchdog open" state, which simply states that
>>>> kernel is expecting the user space to keep the watchdog alive. These
>>>> states might become different mainly because some hardware have
>>>> limitations that prevent them from being stopped at will.
>>>>
>>>
>>> I don't see why this is needed. If you need another state, per your
>>> description, it would be "open" in addition to "active".
>>
>> Yes, the watchdog_is_open() is introduced on patch number two. The
>> original watchdog_is_active() is really confusing. It doesn't really
>> state what it means. Most of the drivers are using it to test whether
>> the watchdog HW is active when going to suspend, but at least atmel
>> watchdog was testing it to see whether the watchdog device is open
>> from user space. The HW itself is always active in that driver.
>>
>> If we are about to distinguish between "device open from user space"
>> and "hardware timer running", we better be clear about the naming.
>> "watchdog_is_active" doesn't really tell what it does.
>>
>> This was originally suggested by Uwe Kleine-König. He also recommended
>> changing the timeout parameter so that is would state more clearly
>> that it is the SW timeout and not HW timeout. But I felt that it would
>> have been too invasive to change the timeout parameter as well. The
>> watchdog_is_active was not used very much so the change was easy.
>>
>> -Timo
>>
> You could just clarify what it means.
>
> Anyway, I think I'll have to step back from this for a while.
> As I mentioned, I think it is getting too invasive, which clouds
> my judgment. I think I'll leave this patch set up to Wim to handle.
Let me try to elaborate my self a little more, maybe it helps taking the
discussion forward.
The early-timeout-sec feature I am trying to get merged is something
that is not tied into any hardware at all. It is a new policy that is
needed. The current policy, explicitly stopping the watchdog, is not a
very good policy if your intention is to keep it running at all times.
The early-timeout-sec would allow to choose a policy where the watchdog
is not stopped at all. Also optionally the watchdog core could extend
the initial expiration of the watchdog in case userspace is slow in
starting up for any reason.
As this is not a hardware related feature but a policy feature, clearly
it should be implemented in the core instead of the drivers.
Unfortunately this feature comes with a hard requirement that the
watchdog should not be stopped by the driver. Currently all drivers
implement explicitly the policy to stop the hardware. There is no way
early-timeout-sec can be implemented in watchdog core without taking the
decision over the policy from the drivers to the core.
Fortunately this change alone is really straightforward to implement in
most of the drivers. As can be seen in my patch to omap_wdt.c, there are
just a few lines of code that really need to change. Also as can be seen
from at91sam9_wdt.c and imx2_wdt.c patches, the change can also remove
quite a lot of code in case the driver is already implementing things
that early-timeout-sec would need anyway in watchdog core.
The thing that really needs to be thought well is what exactly should be
changed in the watchdog core API in order to allow the core to do its
things correctly. The way I thought is that the API should be simple,
not complex. Drivers should be simple and only implement necessary code
to implement functions that the hardware actually supports. Obviously
the changes to the drivers should be also kept minimal to reduce the
conversion work, so this puts quite a deal of limits what changes are
reasonable.
The core needs to know at least the actual HW maximum timeout value and
heartbeat period. Otherwise it can't make any reasonable assumptions
about how to do pinging right. The old second based max_timeout handling
is too limited to be useful for all hardware, which is why I proposed
deprecating it in favour of the millisecond based hw_max_timeout. The
current pretimeout patches in review are unfortunately adding more code
for handling max_timeout, which is colliding with my goals of making the
variables be more useful with describing the actual HW features. Maybe
we don't need to remove the max_timeout, but the logic becomes quite
complex if there are too many different kind of timeouts, especially if
some of them are logically overlapping. This is why I think it would be
better to streamline the timeout handling a bit.
I want to take this work forward, but I see no point in starting to work
with patches until there is at least some sort of agreement of the
correct direction where to take it at. I am hoping to get more
discussion ongoing over this.
Thanks,
-Timo
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active
@ 2015-05-29 12:43 ` Timo Kokkonen
0 siblings, 0 replies; 40+ messages in thread
From: Timo Kokkonen @ 2015-05-29 12:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Other work has begun piling on my desk, sorry I haven't had time to take
this any forward.
On 20.05.2015 16:46, Guenter Roeck wrote:
> On 05/19/2015 10:37 PM, Timo Kokkonen wrote:
>> On 20.05.2015 04:10, Guenter Roeck wrote:
>>> On 05/19/2015 01:26 AM, Timo Kokkonen wrote:
>>>> Before extending the watchdog core midlayer, it is useful to rename
>>>> the watchdog_active function so that it states explicitly what it
>>>> really does. That is, "active" watchdog means really that the watchdog
>>>> hardware is running and needs pinging to prevent a watchdog reset
>>>> taking place in near future.
>>>>
>>>> This is different to "watchdog open" state, which simply states that
>>>> kernel is expecting the user space to keep the watchdog alive. These
>>>> states might become different mainly because some hardware have
>>>> limitations that prevent them from being stopped at will.
>>>>
>>>
>>> I don't see why this is needed. If you need another state, per your
>>> description, it would be "open" in addition to "active".
>>
>> Yes, the watchdog_is_open() is introduced on patch number two. The
>> original watchdog_is_active() is really confusing. It doesn't really
>> state what it means. Most of the drivers are using it to test whether
>> the watchdog HW is active when going to suspend, but at least atmel
>> watchdog was testing it to see whether the watchdog device is open
>> from user space. The HW itself is always active in that driver.
>>
>> If we are about to distinguish between "device open from user space"
>> and "hardware timer running", we better be clear about the naming.
>> "watchdog_is_active" doesn't really tell what it does.
>>
>> This was originally suggested by Uwe Kleine-K?nig. He also recommended
>> changing the timeout parameter so that is would state more clearly
>> that it is the SW timeout and not HW timeout. But I felt that it would
>> have been too invasive to change the timeout parameter as well. The
>> watchdog_is_active was not used very much so the change was easy.
>>
>> -Timo
>>
> You could just clarify what it means.
>
> Anyway, I think I'll have to step back from this for a while.
> As I mentioned, I think it is getting too invasive, which clouds
> my judgment. I think I'll leave this patch set up to Wim to handle.
Let me try to elaborate my self a little more, maybe it helps taking the
discussion forward.
The early-timeout-sec feature I am trying to get merged is something
that is not tied into any hardware at all. It is a new policy that is
needed. The current policy, explicitly stopping the watchdog, is not a
very good policy if your intention is to keep it running at all times.
The early-timeout-sec would allow to choose a policy where the watchdog
is not stopped at all. Also optionally the watchdog core could extend
the initial expiration of the watchdog in case userspace is slow in
starting up for any reason.
As this is not a hardware related feature but a policy feature, clearly
it should be implemented in the core instead of the drivers.
Unfortunately this feature comes with a hard requirement that the
watchdog should not be stopped by the driver. Currently all drivers
implement explicitly the policy to stop the hardware. There is no way
early-timeout-sec can be implemented in watchdog core without taking the
decision over the policy from the drivers to the core.
Fortunately this change alone is really straightforward to implement in
most of the drivers. As can be seen in my patch to omap_wdt.c, there are
just a few lines of code that really need to change. Also as can be seen
from at91sam9_wdt.c and imx2_wdt.c patches, the change can also remove
quite a lot of code in case the driver is already implementing things
that early-timeout-sec would need anyway in watchdog core.
The thing that really needs to be thought well is what exactly should be
changed in the watchdog core API in order to allow the core to do its
things correctly. The way I thought is that the API should be simple,
not complex. Drivers should be simple and only implement necessary code
to implement functions that the hardware actually supports. Obviously
the changes to the drivers should be also kept minimal to reduce the
conversion work, so this puts quite a deal of limits what changes are
reasonable.
The core needs to know at least the actual HW maximum timeout value and
heartbeat period. Otherwise it can't make any reasonable assumptions
about how to do pinging right. The old second based max_timeout handling
is too limited to be useful for all hardware, which is why I proposed
deprecating it in favour of the millisecond based hw_max_timeout. The
current pretimeout patches in review are unfortunately adding more code
for handling max_timeout, which is colliding with my goals of making the
variables be more useful with describing the actual HW features. Maybe
we don't need to remove the max_timeout, but the logic becomes quite
complex if there are too many different kind of timeouts, especially if
some of them are logically overlapping. This is why I think it would be
better to streamline the timeout handling a bit.
I want to take this work forward, but I see no point in starting to work
with patches until there is at least some sort of agreement of the
correct direction where to take it at. I am hoping to get more
discussion ongoing over this.
Thanks,
-Timo
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2015-05-29 12:43 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-19 8:25 [PATCHv8 00/10] watchdog: Extend kernel API and add early_timeout_sec feature Timo Kokkonen
2015-05-19 8:25 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 01/10] watchdog: Rename watchdog_active to watchdog_hw_active Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-20 1:10 ` Guenter Roeck
2015-05-20 1:10 ` Guenter Roeck
2015-05-20 5:37 ` Timo Kokkonen
2015-05-20 5:37 ` Timo Kokkonen
2015-05-20 13:46 ` Guenter Roeck
2015-05-20 13:46 ` Guenter Roeck
2015-05-29 12:43 ` Timo Kokkonen
2015-05-29 12:43 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 02/10] watchdog: core: Ping watchdog on behalf of user space when needed Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-20 1:22 ` Guenter Roeck
2015-05-20 1:22 ` Guenter Roeck
2015-05-20 6:18 ` Timo Kokkonen
2015-05-20 6:18 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 03/10] watchdog: core: Introduce default watchdog policy Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-20 1:13 ` Guenter Roeck
2015-05-20 1:13 ` Guenter Roeck
2015-05-20 5:45 ` Timo Kokkonen
2015-05-20 5:45 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 04/10] watchdog: core: Allow extending early timeout Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-20 1:16 ` Guenter Roeck
2015-05-20 1:16 ` Guenter Roeck
2015-05-19 8:26 ` [PATCHv8 05/10] Documentation/watchdog: Document watchdog core API changes Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 06/10] devicetree: Document generic watchdog properties Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 07/10] Documentation/watchdog: watchdog-test.c: Add support for changing timeout Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 08/10] watchdog: at91sam9_wdt: Convert to use new watchdog core extensions Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 09/10] watchdog: imx2_wdt: Convert to use new " Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
2015-05-19 8:26 ` [PATCHv8 10/10] watchdog: omap_wdt: " Timo Kokkonen
2015-05-19 8:26 ` Timo Kokkonen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.