Backport of watchdog core triggered keepalive infrastructure
Subsystems (3)
cover Backport of watchdog core triggered keepalive infrastructure
Hello Ben,
This series contains
* backport of patches of watchdog core infrastructure supporting handling watchdog keepalive.
* imx21_wdt converted to use infrastructure triggered keepalives.
* backported support of WATCHDOG_HANDLE_BOOT_ENABLED option
On some systems it's desirable to have watchdog reboot the system when it does not come up fast enough. This adds a kernel parameter to disable the auto-update of watchdog before userspace takes over and a kernel option to set the default.
One of the important use cases is safe system update.
In that case, the bootloader enables the watchdog, and Linux should only take it over at the point the whole Linux system is sure to be able to have a fully working system again, including userspace services that could take the next update. If we start the trigger the watchdog too early, like it was so far, we risk to enter a system state where the kernel works but not all other services we need.
i.MX6 based board was used as a test platform.
We suppose that watchdog is enabled in bootloader and set to 120 seconds (maximum timeout supported by i.MX watchdog timer)
After applying these patches, built-in i.MX watchdog (imx21_wdt) into kernel and perform the following test cases:
1) option WATCHDOG_HANDLE_BOOT_ENABLED is turned off in kernel configuration w/o userspace application triggering the watchdog.
Make sure that no userspace application is triggering watchdog.
After around 120 seconds (timeout set in bootloader) board will reboot.
2) option WATCHDOG_HANDLE_BOOT_ENABLED is turned off in kernel configuration but userspace application is triggering watchdog more frequently that watchdog's timeout (by default set to 60 seconds for imx21_wdt).
Watchdog will not reboot the board until the application re-arms the watchdog.
3) option WATCHDOG_HANDLE_BOOT_ENABLED is turned on in kernel configuration w/o userspace application triggering the watchdog.
Make sure that no userspace application is triggering watchdog. Board will not reboot (watchdog keepalive is supported by watchdog core infrastructure, not by driver itself)
Kind regards,
Maxim.
Guenter Roeck (6):
watchdog: Introduce hardware maximum heartbeat in watchdog core
watchdog: Introduce WDOG_HW_RUNNING flag
watchdog: Make stop function optional
watchdog: imx2: Convert to use infrastructure triggered keepalives
watchdog: core: Fix circular locking dependency
watchdog: core: Clear WDOG_HW_RUNNING before calling the stop function
Pratyush Anand (1):
watchdog: skip min and max timeout validity check when
max_hw_heartbeat_ms is defined
Rasmus Villemoes (1):
watchdog: change watchdog_need_worker logic
Sebastian Reichel (1):
watchdog: core: add option to avoid early handling of watchdog
Wei Yongjun (1):
watchdog: core: Fix error handling of watchdog_dev_init()
Documentation/watchdog/watchdog-kernel-api.txt | 51 +++++-- drivers/watchdog/Kconfig | 11 ++ drivers/watchdog/imx2_wdt.c | 74 ++------- drivers/watchdog/watchdog_core.c | 4 +- drivers/watchdog/watchdog_dev.c | 198 +++++++++++++++++++++++-- include/linux/watchdog.h | 40 ++++- 6 files changed, 279 insertions(+), 99 deletions(-)
Signature · 1 line
2.11.0
PATCH 1/10 watchdog: Introduce hardware maximum heartbeat in watchdog core
From: Guenter Roeck <[email protected]>
Backport from kernel.org, upstream commit 664a39236e71
Introduce an optional hardware maximum heartbeat in the watchdog core. The hardware maximum heartbeat can be lower than the maximum timeout.
Drivers can set the maximum hardware heartbeat value in the watchdog data structure. If the configured timeout exceeds the maximum hardware heartbeat, the watchdog core enables a timer function to assist sending keepalive requests to the watchdog driver.
--- Documentation/watchdog/watchdog-kernel-api.txt | 19 +++- drivers/watchdog/watchdog_dev.c | 122 +++++++++++++++++++++++-- include/linux/watchdog.h | 30 ++++-- 3 files changed, 154 insertions(+), 17 deletions(-)
Documentation/watchdog/watchdog-kernel-api.txt +17 -2
@@ -53,6 +53,7 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int max_hw_heartbeat_ms; void *driver_data; struct mutex lock; unsigned long status;
@@ -73,8 +74,18 @@ It contains following fields: additional information about the watchdog timer itself. (Like it's unique name) * ops: a pointer to the list of watchdog operations that the watchdog supports. * timeout: the watchdog timer's timeout value (in seconds). + This is the time after which the system will reboot if user space does + not send a heartbeat request if WDOG_ACTIVE is set. * min_timeout: the watchdog timer's minimum timeout value (in seconds). -* max_timeout: the watchdog timer's maximum timeout value (in seconds). + If set, the minimum configurable value for 'timeout'. +* max_timeout: the watchdog timer's maximum timeout value (in seconds), + as seen from userspace. If set, the maximum configurable value for + 'timeout'. Not used if max_hw_heartbeat_ms is non-zero. +* max_hw_heartbeat_ms: Maximum hardware heartbeat, in milli-seconds. + If set, the infrastructure will send heartbeats to the watchdog driver + if 'timeout' is larger than max_hw_heartbeat_ms, unless WDOG_ACTIVE + is set and userspace failed to send a heartbeat for at least 'timeout' + seconds. * 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.
@@ -160,7 +171,11 @@ they are supported. These optional routines/operations are: and -EIO for "could not write value to the watchdog". On success this routine should set the timeout value of the watchdog_device to the achieved timeout value (which may be different from the requested one - because the watchdog does not necessarily has a 1 second resolution). + because the watchdog does not necessarily have a 1 second resolution). + Drivers implementing max_hw_heartbeat_ms set the hardware watchdog heartbeat + to the minimum of timeout and max_hw_heartbeat_ms. Those drivers set the + timeout value of the watchdog_device either to the requested timeout value + (if it is larger than max_hw_heartbeat_ms), or to the achieved timeout value. (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the watchdog's info structure). * get_timeleft: this routines returns the time that's left before a reset.
drivers/watchdog/watchdog_dev.c +115 -7
204 lines not on this page · raw message
include/linux/watchdog.h +22 -8
@@ -10,8 +10,9 @@#include <linux/bitops.h> -#include <linux/device.h> #include <linux/cdev.h> +#include <linux/device.h> +#include <linux/kernel.h> #include <uapi/linux/watchdog.h> struct watchdog_ops ;
@@ -61,12 +62,17 @@ struct watchdog_ops {* @bootstatus: Status of the watchdog device at boot. * @timeout: The watchdog devices timeout value (in seconds). * @min_timeout:The watchdog devices minimum timeout value (in seconds). -* @max_timeout:The watchdog devices maximum timeout value (in seconds). +* @max_timeout:The watchdog devices maximum timeout value (in seconds) +* as configurable from user space. Only relevant if +* max_hw_heartbeat_ms is not provided. +* @max_hw_heartbeat_ms: +* Hardware limit for maximum timeout, in milli-seconds. +* Replaces max_timeout if specified. * @driver-data:Pointer to the drivers private data. * @lock: Lock for watchdog core internal use only. * @status: Field that contains the devices internal status bits. -* @deferred: entry in wtd_deferred_reg_list which is used to -* register early initialized watchdogs. +* @deferred: Entry in wtd_deferred_reg_list which is used to +* register early initialized watchdogs. * * The watchdog_device structure contains all information about a * watchdog timer device.
@@ -88,8 +94,11 @@ struct watchdog_device {unsigned int timeout;unsigned int min_timeout;unsigned int max_timeout; +unsigned int max_hw_heartbeat_ms;void *driver_data;struct mutex lock; +unsigned long last_keepalive; +struct delayed_work work;unsigned long status;/* Bit numbers for status flags */ #define WDOG_ACTIVE 0/* Is the watchdog running/active */
@@ -121,13 +130,18 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne {/* * The timeout is invalid if +* - the requested value is larger than UINT_MAX / 1000 +* (since internal calculations are done in milli-seconds), +* or * - the requested value is smaller than the configured minimum timeout, * or -* - a maximum timeout is configured, and the requested value is larger -* than the maximum timeout. +* - a maximum hardware timeout is not configured, a maximum timeout +* is configured, and the requested value is larger than the +* configured maximum timeout. */ -return t < wdd->min_timeout || - (wdd->max_timeout && t > wdd->max_timeout); +return t >UINT_MAX /1000 || t < wdd->min_timeout || + (!wdd->max_hw_heartbeat_ms && wdd->max_timeout && + t > wdd->max_timeout); }/* Use the following functions to manipulate watchdog driver specific data */
Signature · 1 line
2.11.0
PATCH 2/10 watchdog: Introduce WDOG_HW_RUNNING flag
In reply to Maxim Yu. OsipovFrom: Guenter Roeck <[email protected]>
Backport from kernel.org, upstream commit ee142889e32f
The WDOG_HW_RUNNING flag is expected to be set by watchdog drivers if the hardware watchdog is running. If the flag is set, the watchdog subsystem will ping the watchdog even if the watchdog device is closed.
The watchdog driver stop function is now optional and may be omitted if the watchdog can not be stopped. If stopping the watchdog is not possible but the driver implements a stop function, it is responsible to set the WDOG_HW_RUNNING flag in its stop function.
--- Documentation/watchdog/watchdog-kernel-api.txt | 22 ++++++++---- drivers/watchdog/watchdog_dev.c | 49 ++++++++++++++++++++------ include/linux/watchdog.h | 10 ++++++ 3 files changed, 64 insertions(+), 17 deletions(-)
Documentation/watchdog/watchdog-kernel-api.txt +15 -7
@@ -144,10 +144,10 @@ are: * 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. + Some watchdog timer hardware can only be started and not be stopped. + If a watchdog can not be stopped, the watchdog driver must set the + WDOG_HW_RUNNING flag in its stop function to inform the watchdog core that + the watchdog is still running. 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
@@ -191,9 +191,8 @@ 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) + is active or not from user perspective. User space is expected to send + heartbeat requests to the driver while this flag is set. * 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).
@@ -202,6 +201,15 @@ bit-operations. The status bits that are defined are: (This bit should only be used by the WatchDog Timer Driver Core). * WDOG_NO_WAY_OUT: this bit stores the nowayout setting for the watchdog. If this bit is set then the watchdog timer will not be able to stop. +* WDOG_HW_RUNNING: Set by the watchdog driver if the hardware watchdog is + running. The bit must be set if the watchdog timer hardware can not be + stopped. The bit may also be set if the watchdog timer is running after + booting, before the watchdog device is opened. If set, the watchdog + infrastructure will send keepalives to the watchdog hardware while + WDOG_ACTIVE is not set. + Note: when you register the watchdog timer device with this bit set, + then opening /dev/watchdog will skip the start operation but send a keepalive + request instead. * WDOG_UNREGISTERED: this bit gets set by the WatchDog Timer Driver Core after calling watchdog_unregister_device, and then checked before calling any watchdog_ops, so that you can be sure that no operations (other then
drivers/watchdog/watchdog_dev.c +39 -10
118 lines not on this page · raw message
include/linux/watchdog.h +10 -0
@@ -106,6 +106,7 @@ struct watchdog_device {#define WDOG_ALLOW_RELEASE 2/* Did we receive the magic char ? */ #define WDOG_NO_WAY_OUT 3/* Is 'nowayout' feature set ? */ #define WDOG_UNREGISTERED 4/* Has the device been unregistered */ +#define WDOG_HW_RUNNING 5/* True if HW watchdog running */ struct list_head deferred; };
@@ -118,6 +119,15 @@ static inline bool watchdog_active(struct watchdog_device *wdd)return test_bit (WDOG_ACTIVE , &wdd->status); } +/* +* Use the following function to check whether or not the hardware watchdog +* is running +*/ +static inline bool watchdog_hw_running (struct watchdog_device *wdd) +{ +return test_bit (WDOG_HW_RUNNING , &wdd->status); +} +/* Use the following function to set the nowayout feature */ static inline void watchdog_set_nowayout (struct watchdog_device *wdd,bool nowayout) {
Signature · 1 line
2.11.0
PATCH 2/10 watchdog: Introduce WDOG_HW_RUNNING flag
Quoted 13 lines · Maxim Yu. Osipov
From: Guenter Roeck <[email protected]>
Backport from kernel.org, upstream commit
ee142889e32fThe
WDOG_HW_RUNNINGflag is expected to be set by watchdog drivers if the hardware watchdog is running. If the flag is set, the watchdog subsystem will ping the watchdog even if the watchdog device is closed.The watchdog driver stop function is now optional and may be omitted if the watchdog can not be stopped. If stopping the watchdog is not possible but the driver implements a stop function, it is responsible to set the
WDOG_HW_RUNNINGflag in its stop function.
[...]
Quoted 2 lines
drivers/watchdog/watchdog_dev.c +0
-0
[...]
drivers/watchdog/watchdog_dev.c int watchdog_dev_register(struct watchdog_device · line 670
Quoted 7 lines
drivers/watchdog/watchdog_dev.c +2
-0@@ -651,9 +665,24 @@ int watchdog_dev_register(struct watchdog_device *wdd) ? if (wdd->id == 0) { ? misc_deregister(&watchdog_miscdev); ? old_wdd = NULL; +if (wdd->ops->unref) + wdd->ops->unref (wdd);
[...]
This doesn't look right. This is decrementing a reference counter because the old_wdd reference is going away, but we never increment a reference counter when old_wdd is set.
The upstream commit doesn't make any change here. There is a kref_put() here in the upstream version but that was added as part of the watchdog_device/watchdog_core_data split.
Ben.
Signature · 2 lines
Ben Hutchings Software Developer, Codethink Ltd.
PATCH 3/10 watchdog: Make stop function optional
In reply to Maxim Yu. OsipovFrom: Guenter Roeck <[email protected]>
Backport from kernel.org, upstream commit d0684c8a9354
Not all hardware watchdogs can be stopped. The driver for such watchdogs would typically only set the WATCHDOG_HW_RUNNING flag in its stop function. Make the stop function optional and set WATCHDOG_HW_RUNNING in the watchdog core if it is not provided.
--- Documentation/watchdog/watchdog-kernel-api.txt | 20 ++++++++++++-------- drivers/watchdog/watchdog_core.c | 2 +- drivers/watchdog/watchdog_dev.c | 6 +++++- 3 files changed, 18 insertions(+), 10 deletions(-)
Documentation/watchdog/watchdog-kernel-api.txt +12 -8
@@ -85,7 +85,8 @@ It contains following fields: If set, the infrastructure will send heartbeats to the watchdog driver if 'timeout' is larger than max_hw_heartbeat_ms, unless WDOG_ACTIVE is set and userspace failed to send a heartbeat for at least 'timeout' - seconds. + seconds. max_hw_heartbeat_ms must be set if a driver does not implement + the stop function. * 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.
@@ -141,17 +142,20 @@ are: device. 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. - If a watchdog can not be stopped, the watchdog driver must set the - WDOG_HW_RUNNING flag in its stop function to inform the watchdog core that - the watchdog is still running. 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 they are supported. These optional routines/operations are: +* 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. A + driver supporting such hardware does not have to implement the stop routine. + If a driver has no stop function, the watchdog core will set WDOG_HW_RUNNING + and start calling the driver's keepalive pings function after the watchdog + device is closed. + If a watchdog driver does not implement the stop function, it must set + max_hw_heartbeat_ms. * ping: this is the routine that sends a keepalive ping to the watchdog timer hardware. The routine needs a pointer to the watchdog timer device structure as a
drivers/watchdog/watchdog_core.c +1 -1
@@ -145,7 +145,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)return -EINVAL ;/* Mandatory operations need to be supported */ -if (wdd->ops->start ==NULL || wdd->ops->stop ==NULL ) +if (!wdd->ops->start || (!wdd->ops->stop && !wdd->max_hw_heartbeat_ms))return -EINVAL ;watchdog_check_min_max_timeout (wdd);
drivers/watchdog/watchdog_dev.c +5 -1
@@ -237,7 +237,11 @@ static int watchdog_stop(struct watchdog_device *wdd) goto out_stop; } - err = wdd->ops->stop (wdd); +if (wdd->ops->stop) + err = wdd->ops->stop (wdd); +else +set_bit (WDOG_HW_RUNNING , &wdd->status); +if (err ==0 ) {clear_bit (WDOG_ACTIVE , &wdd->status);watchdog_update_worker (wdd);
Signature · 1 line
2.11.0
PATCH 4/10 watchdog: imx2: Convert to use infrastructure triggered keepalives
In reply to Maxim Yu. OsipovFrom: Guenter Roeck <[email protected]>
Backport from kernel.org, upstream commit 11d7aba9ceb7
The watchdog infrastructure now supports handling watchdog keepalive if the watchdog is running while the watchdog device is closed. Convert the driver to use this infrastructure.
--- drivers/watchdog/imx2_wdt.c | 74 ++++++++------------------------------------- 1 file changed, 13 insertions(+), 61 deletions(-)
drivers/watchdog/imx2_wdt.c +13 -61
@@ -25,7 +25,6 @@#include <linux/delay.h> #include <linux/init.h> #include <linux/io.h> -#include <linux/jiffies.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/moduleparam.h>
@@ -34,7 +33,6 @@#include <linux/platform_device.h> #include <linux/reboot.h> #include <linux/regmap.h> -#include <linux/timer.h> #include <linux/watchdog.h> #define DRIVER_NAME "imx2-wdt"
@@ -62,7 +60,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,16 +148,6 @@ 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) {
@@ -177,40 +164,19 @@ 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); +if (imx2_wdt_is_running (wdev))imx2_wdt_set_timeout (wdog, wdog->timeout); - }else +else imx2_wdt_setup (wdog); -return imx2_wdt_ping (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 ; -} - -static inline void imx2_wdt_ping_if_active (struct watchdog_device *wdog) -{ -struct imx2_wdt_device *wdev =watchdog_get_drvdata (wdog); +set_bit (WDOG_HW_RUNNING , &wdog->status); -if (imx2_wdt_is_running (wdev)) { -imx2_wdt_set_timeout (wdog, wdog->timeout); -imx2_wdt_timer_ping ((unsigned long )wdog); - } +return imx2_wdt_ping (wdog); }static const struct watchdog_ops imx2_wdt_ops = { .owner =THIS_MODULE , .start = imx2_wdt_start, - .stop = imx2_wdt_stop, .ping = imx2_wdt_ping, .set_timeout = imx2_wdt_set_timeout, };
@@ -257,7 +223,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev) wdog->info = &imx2_wdt_info; wdog->ops = &imx2_wdt_ops; wdog->min_timeout =1 ; - wdog->max_timeout =IMX2_WDT_MAX_TIME ; + wdog->max_hw_heartbeat_ms =IMX2_WDT_MAX_TIME *1000 ; wdog->parent = &pdev->dev; ret =clk_prepare_enable (wdev->clk);
@@ -277,9 +243,10 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)watchdog_set_nowayout (wdog, nowayout);watchdog_init_timeout (wdog, timeout, &pdev->dev); -setup_timer (&wdev->timer, imx2_wdt_timer_ping, (unsigned long )wdog); - -imx2_wdt_ping_if_active (wdog); +if (imx2_wdt_is_running (wdev)) { +imx2_wdt_set_timeout (wdog, wdog->timeout); +set_bit (WDOG_HW_RUNNING , &wdog->status); + }/* * Disable the watchdog power down counter at boot. Otherwise the power
@@ -320,7 +287,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" ); }
@@ -334,10 +300,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, configure 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" );
@@ -355,10 +320,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_active (wdog)) -del_timer_sync (&wdev->timer); }clk_disable_unprepare (wdev->clk);
@@ -384,19 +345,10 @@ static int imx2_wdt_resume(struct device *dev)* watchdog again. */ imx2_wdt_setup (wdog); + } +if (imx2_wdt_is_running (wdev)) {imx2_wdt_set_timeout (wdog, wdog->timeout);imx2_wdt_ping (wdog); - }else if (imx2_wdt_is_running (wdev)) { -/* 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_active (wdog)) -mod_timer (&wdev->timer, - jiffies + wdog->timeout *HZ /2 ); }return 0 ;
Signature · 1 line
2.11.0
PATCH 5/10 watchdog: core: Fix circular locking dependency
In reply to Maxim Yu. OsipovFrom: Guenter Roeck <[email protected]>
Backport from kernel.org, upstream commit e1f30282a1d3
lockdep reports the following circular locking dependency.
======================================================
INFO: possible circular locking dependency detected ]
4.6.0-rc3-00191-gfabf418 #162 Not tainted
-------------------------------------------------------
systemd/1 is trying to acquire lock:
((&(&wd_data->work)->work)){+.+...}, at: [<80141650>] flush_work+0x0/0x280
but task is already holding lock:
(&wd_data->lock){+.+...}, at: [<804acfa8>] watchdog_release+0x18/0x190
which lock already depends on the new lock. the existing dependency chain (in reverse order) is:
-> #1 (&wd_data->lock){+.+...}:
Quoted 6 lines
-------[<80662310>]
mutex_lock_nested+0x64/0x4a8
-------[<804aca4c>]watchdog_ping_work+0x18/0x4c
-------[<80143128>]process_one_work+0x1ac/0x500
-------[<801434b4>]worker_thread+0x38/0x554
-------[<80149510>]kthread+0xf4/0x108
-------[<80107c10>]ret_from_fork+0x14/0x24
-> #0 ((&(&wd_data->work)->work)){+.+...}:
Quoted 8 lines
-------[<8017c4e8>]
lock_acquire+0x70/0x90-------[<8014169c>]flush_work+0x4c/0x280-------[<801440f8>]__cancel_work_timer+0x9c/0x1e0-------[<804acfcc>]watchdog_release+0x3c/0x190-------[<8022c5e8>]__fput+0x80/0x1c8-------[<80147b28>]task_work_run+0x94/0xc8-------[<8010b998>]do_work_pending+0x8c/0xb4-------[<80107ba8>]slow_work_pending+0xc/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&wd_data->lock);
lock((&(&wd_data->work)->work));
lock(&wd_data->lock);lock((&(&wd_data->work)->work));
*** DEADLOCK ***
1 lock held by systemd/1:
stack backtrace: CPU: 2 PID: 1 Comm: systemd Not tainted 4.6.0-rc3-00191-gfabf418 #162 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [<8010f5e4>] (unwind_backtrace) from [<8010c038>] (show_stack+0x10/0x14) [<8010c038>] (show_stack) from [<8039d7fc>] (dump_stack+0xa8/0xd4) [<8039d7fc>] (dump_stack) from [<80177ee0>] (print_circular_bug+0x214/0x334) [<80177ee0>] (print_circular_bug) from [<80179230>] (check_prevs_add+0x4dc/0x8e8) [<80179230>] (check_prevs_add) from [<8017b3d8>] (__lock_acquire+0xc6c/0x14ec) [<8017b3d8>] (__lock_acquire) from [<8017c4e8>] (lock_acquire+0x70/0x90) [<8017c4e8>] (lock_acquire) from [<8014169c>] (flush_work+0x4c/0x280) [<8014169c>] (flush_work) from [<801440f8>] (__cancel_work_timer+0x9c/0x1e0) [<801440f8>] (__cancel_work_timer) from [<804acfcc>] (watchdog_release+0x3c/0x190) [<804acfcc>] (watchdog_release) from [<8022c5e8>] (__fput+0x80/0x1c8) [<8022c5e8>] (__fput) from [<80147b28>] (task_work_run+0x94/0xc8) [<80147b28>] (task_work_run) from [<8010b998>] (do_work_pending+0x8c/0xb4) [<8010b998>] (do_work_pending) from [<80107ba8>] (slow_work_pending+0xc/0x20)
Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.
--- drivers/watchdog/watchdog_dev.c | 1 - 1 file changed, 1 deletion(-)
drivers/watchdog/watchdog_dev.c +0 -1
@@ -596,7 +596,6 @@ static int watchdog_release(struct inode *inode, struct file *file)if (!watchdog_hw_running (wdd))module_put (wdd->ops->owner); -cancel_delayed_work_sync (&wdd->work);watchdog_update_worker (wdd);/* make sure that /dev/watchdog can be re-opened */
Signature · 1 line
2.11.0
PATCH 6/10 watchdog: skip min and max timeout validity check when max_hw_heartbeat_ms is defined
In reply to Maxim Yu. OsipovFrom: Pratyush Anand <[email protected]>
Backport from kernel.org, upstream commit 1894cad9bf2c
When max_hw_heartbeat_ms has a none zero value, max_timeout is not used. So it's value can be 0. In such case if a driver uses min_timeout functionality, then check will always fail.
This patch fixes above issue.
--- drivers/watchdog/watchdog_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/watchdog_core.c +1 -1
@@ -88,7 +88,7 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)* Check that we have valid min and max timeout values, if * not reset them both to 0 (=not used or unknown) */ -if (wdd->min_timeout > wdd->max_timeout) { +if (!wdd->max_hw_heartbeat_ms && wdd->min_timeout > wdd->max_timeout) {pr_info ("Invalid min and max timeout values, resetting to 0!\n" ); wdd->min_timeout =0 ; wdd->max_timeout =0 ;
Signature · 1 line
2.11.0
PATCH 7/10 watchdog: change watchdog_need_worker logic
In reply to Maxim Yu. OsipovFrom: Rasmus Villemoes <[email protected]>
Backport from kernel.org, upstream commit 3fbfe9264756
If the driver indicates that the watchdog is running, the framework should feed it until userspace opens the device, regardless of whether the driver has set max_hw_heartbeat_ms.
This patch only affects the case where wdd->max_hw_heartbeat_ms is zero, wdd->timeout is non-zero, the watchdog is not active and the hardware device is running (*):
- If wdd->timeout is zero, watchdog_need_worker() returns false both before and after this patch, and watchdog_next_keepalive() is not called.
- If watchdog_active(wdd), the return value from watchdog_need_worker is also the same as before (namely, hm && t > hm). Hence in that case, watchdog_next_keepalive() is only called if hm == max_hw_heartbeat_ms is non-zero, so the change to min_not_zero there is a no-op.
- If the watchdog is not active and the device is not running, we return false from watchdog_need_worker just as before.
That leaves the watchdog_hw_running(wdd) && !watchdog_active(wdd) && wdd->timeout case. Again, it's easy to see that if wdd->max_hw_heartbeat_ms is non-zero, we return true from watchdog_need_worker with and without this patch, and the logic in watchdog_next_keepalive is unchanged. Finally, if wdd->max_hw_heartbeat_ms is 0, we used to end up in the cancel_delayed_work branch, whereas with this patch we end up scheduling a ping timeout_ms/2 from now.
(*) This should imply that no current kernel drivers are affected, since the only drivers which explicitly set WDOG_HW_RUNNING are imx2_wdt.c and dw_wdt.c, both of which also provide a non-zero value for max_hw_heartbeat_ms. The watchdog core also sets WDOG_HW_RUNNING, but only when the driver doesn't provide ->stop, in which case it must, according to Documentation/watchdog/watchdog-kernel-api.txt, set max_hw_heartbeat_ms.
--- drivers/watchdog/watchdog_dev.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/watchdog/watchdog_dev.c +7 -3
@@ -67,9 +67,13 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)* thus is aware that the framework supports generating heartbeat * requests. * - Userspace requests a longer timeout than the hardware can handle. +* +* Alternatively, if userspace has not opened the watchdog +* device, we take care of feeding the watchdog if it is +* running. */ -return hm && ((watchdog_active (wdd) && t > hm) || - (t && !watchdog_active (wdd) &&watchdog_hw_running (wdd))); +return (hm &&watchdog_active (wdd) && t > hm) || + (t && !watchdog_active (wdd) &&watchdog_hw_running (wdd)); }static long watchdog_next_keepalive (struct watchdog_device *wdd)
@@ -81,7 +85,7 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)unsigned int hw_heartbeat_ms; virt_timeout = wdd->last_keepalive +msecs_to_jiffies (timeout_ms); - hw_heartbeat_ms =min (timeout_ms, wdd->max_hw_heartbeat_ms); + hw_heartbeat_ms =min_not_zero (timeout_ms, wdd->max_hw_heartbeat_ms); keepalive_interval =msecs_to_jiffies (hw_heartbeat_ms /2 );if (!watchdog_active (wdd))
Signature · 1 line
2.11.0
PATCH 8/10 watchdog: core: Fix error handling of watchdog_dev_init()
In reply to Maxim Yu. OsipovFrom: Wei Yongjun <[email protected]>
Backport from kernel.org, upstream commit 138913cb632b
Fix the error handling paths of watchdog_dev_init().
--- drivers/watchdog/watchdog_dev.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/watchdog/watchdog_dev.c +8 -1
@@ -733,8 +733,15 @@ int __init watchdog_dev_init(void) } err =alloc_chrdev_region (&watchdog_devt,0 ,MAX_DOGS ,"watchdog" ); -if (err <0 ) +if (err <0 ) {pr_err ("watchdog: unable to allocate char dev region\n" ); + goto err_alloc; + } + +return 0 ; + +err_alloc: +destroy_workqueue (watchdog_wq);return err; }
Signature · 1 line
2.11.0
PATCH 9/10 watchdog: core: Clear WDOG_HW_RUNNING before calling the stop function
In reply to Maxim Yu. OsipovFrom: Guenter Roeck <[email protected]>
Backport from kernel.org, upstream commit 3c10bbde10fe
WDOG_HW_RUNNING indicates that the hardware watchdog is running while the watchdog device is closed. The flag may be set by the driver when it is instantiated to indicate that the watchdog is running, and that the watchdog core needs to send heartbeat requests to the driver until the watchdog device is opened.
When the watchdog device is closed, the flag can be used by the driver's stop function to indicate to the watchdog core that it was unable to stop the watchdog, and that the watchdog core needs to send heartbeat requests. This only works if the flag is actually cleared when the watchdog is stopped. To avoid having to clear the flag in each driver's stop function, clear it in the watchdog core before calling the stop function.
--- drivers/watchdog/watchdog_dev.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/watchdog/watchdog_dev.c +4 -2
@@ -241,10 +241,12 @@ static int watchdog_stop(struct watchdog_device *wdd) goto out_stop; } -if (wdd->ops->stop) +if (wdd->ops->stop) { +clear_bit (WDOG_HW_RUNNING , &wdd->status); err = wdd->ops->stop (wdd); -else + }else {set_bit (WDOG_HW_RUNNING , &wdd->status); + }if (err ==0 ) {clear_bit (WDOG_ACTIVE , &wdd->status);
Signature · 1 line
2.11.0
PATCH 10/10 watchdog: core: add option to avoid early handling of watchdog
In reply to Maxim Yu. OsipovFrom: Sebastian Reichel <[email protected]>
Backport from kernel.org, upstream commit 2501b015313f
On some systems its desirable to have watchdog reboot the system when it does not come up fast enough. This adds a kernel parameter to disable the auto-update of watchdog before userspace takes over and a kernel option to set the default. The info messages were added to shorten error searching on misconfigured systems.
--- drivers/watchdog/Kconfig | 11 +++++++++++ drivers/watchdog/watchdog_dev.c | 21 +++++++++++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-)
drivers/watchdog/Kconfig +11 -0
@@ -46,6 +46,17 @@ config WATCHDOG_NOWAYOUT get killed. If you say Y here, the watchdog cannot be stopped once it has been started. +config WATCHDOG_HANDLE_BOOT_ENABLED + bool "Update boot-enabled watchdog until userspace takes over" + default y + help + The default watchdog behaviour (which you get if you say Y here) is + to ping watchdog devices that were enabled before the driver has + been loaded until control is taken over from userspace using the + /dev/watchdog file. If you say N here, the kernel will not update + the watchdog on its own. Thus if your userspace does not start fast + enough your device will reboot. + # # General Watchdog drivers #
drivers/watchdog/watchdog_dev.c +17 -4
@@ -53,6 +53,9 @@ static struct watchdog_device *old_wdd;static struct workqueue_struct *watchdog_wq; +static bool handle_boot_enabled = +IS_ENABLED (CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED ); +static inline bool watchdog_need_worker (struct watchdog_device *wdd) {/* All variables in milli-seconds */
@@ -685,10 +688,15 @@ int watchdog_dev_register(struct watchdog_device *wdd)* and schedule an immediate ping. */ if (watchdog_hw_running (wdd)) { -__module_get (wdd->ops->owner); -if (wdd->ops->ref) - wdd->ops->ref (wdd); -queue_delayed_work (watchdog_wq, &wdd->work,0 ); +if (handle_boot_enabled) { +__module_get (wdd->ops->owner); +if (wdd->ops->ref) + wdd->ops->ref (wdd); +queue_delayed_work (watchdog_wq, &wdd->work,0 ); + }else { +pr_info ("watchdog%d running and kernel based pre-userspace handler disabled\n" , + wdd->id); + } }return 0 ;
@@ -758,3 +766,8 @@ void __exit watchdog_dev_exit(void)unregister_chrdev_region (watchdog_devt,MAX_DOGS );destroy_workqueue (watchdog_wq); } + +module_param (handle_boot_enabled, bool,0444 ); +MODULE_PARM_DESC (handle_boot_enabled, + "Watchdog core auto-updates boot enabled watchdogs before userspace takes over (default=" + __MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) ")");
Signature · 1 line
2.11.0
cover Backport of watchdog core triggered keepalive infrastructure
In reply to Maxim Yu. OsipovQuoted 10 lines · Maxim Yu. Osipov
Hello Ben,
This series contains
* backport of patches of watchdog core infrastructure supporting handling watchdog keepalive.
*
imx21_wdtconverted to use infrastructure triggered keepalives.* backported support of
WATCHDOG_HANDLE_BOOT_ENABLEDoption
[...]
Are you working for Siemens on this? Can you provide the config you are using?
The reason I ask is that CIP kernel branches will have hardware support limited to the needs of members. Currently, none of the kernel configurations I collected enable CONFIG_IMX2_WDT, or even CONFIG_ARCH_MXC, which implies that imx2_wdt won't be supported.
Ben.
Signature · 2 lines
Ben Hutchings Software Developer, Codethink Ltd.
cover Backport of watchdog core triggered keepalive infrastructure
Quoted 8 lines · Ben Hutchings
Quoted 7 lines · Maxim Yu. Osipov
Hello Ben,
This series contains
* backport of patches of watchdog core infrastructure supporting handling watchdog keepalive.
Quoted 7 lines
Quoted 3 lines
*
imx21_wdtconverted to use infrastructure triggered keepalives.* backported support of
WATCHDOG_HANDLE_BOOT_ENABLEDoption[...]
Are you working for Siemens on this? Can you provide the config you are using?
Yes, Maxim is working for a project of us, and I asked him to propose these backports for CIP. They will likely become relevant for other boards as well because the bootloader-started watchdog is a cornerstone of safe system updates.
Quoted 5 lines
The reason I ask is that CIP kernel branches will have hardware support limited to the needs of members. Currently, none of the kernel configurations I collected enable
CONFIG_IMX2_WDT, or evenCONFIG_ARCH_MXC, which implies thatimx2_wdtwon't be supported.
Yeah, this config wasn't included in the set I sent back then. Maxim, please provide it to Ben. We are in the process of migrating to the CIP kernel right now.
Thanks,
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux
cover Backport of watchdog core triggered keepalive infrastructure
In reply to Maxim Yu. OsipovPlease use one of these formats for upstream references in the commit messages:
commit 0123456789012345678901234567890123456789 upstream.
[ Upstream commit 0123456789012345678901234567890123456789 ]Ben.
Signature · 2 lines
Ben Hutchings Software Developer, Codethink Ltd.