* watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [not found] <S1751381AbaIDOwq/20140904145246Z+988@vger.kernel.org> @ 2014-09-04 15:47 ` Janusz Użycki 2014-09-04 16:05 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] Janusz Użycki 2014-09-07 17:18 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space Guenter Roeck 0 siblings, 2 replies; 23+ messages in thread From: Janusz Użycki @ 2014-09-04 15:47 UTC (permalink / raw) To: linux-watchdog; +Cc: Wim Van Sebroeck [-- Attachment #1: Type: text/plain, Size: 1260 bytes --] Hi, Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. * Can you comment the proposed patch? * Shoud dynamic or static timer_list be used (small structure...)? * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? best regards Janusz Uzycki [-- Attachment #2: wdt_kernel_ping.diff --] [-- Type: text/plain, Size: 4939 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..52c5f1c 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -78,6 +78,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev) stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); + stmp3xxx_wdd.status |= WATCHDOG_KERNEL_PING; ret = watchdog_register_device(&stmp3xxx_wdd); if (ret < 0) { dev_err(&pdev->dev, "cannot register watchdog device\n"); diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..d4fae2c 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/slab.h> /* for kernel ping timer (dynamic) */ +#include <linux/jiffies.h> /* for kernel ping timer */ #include "watchdog_core.h" @@ -277,6 +279,34 @@ out_ioctl: return err; } +/* kernel ping feature */ +static void watchdog_kping_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { + watchdog_ping(wdd); + /* call next ping a second or 0.5s before hardware timeout */ + mod_timer(wdd->kping_timer, wdd->timeout > 1 ? + jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); + } +} + +static void watchdog_kping_start(struct watchdog_device *wdd) +{ + if (wdd->ops->ref) + wdd->ops->ref(wdd); + watchdog_start(wdd); + watchdog_ping(wdd); + mod_timer(wdd->kping_timer, jiffies + HZ/2); +} + +static void watchdog_kping_stop(struct watchdog_device *wdd) +{ + del_timer_sync(wdd->kping_timer); + if (wdd->ops->unref) + wdd->ops->unref(wdd); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +460,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) + watchdog_kping_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +505,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { + watchdog_kping_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +562,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); + if (!watchdog->kping_timer) + return -ENOMEM; + setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); + watchdog_kping_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -575,6 +623,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { + watchdog_kping_stop(watchdog); + watchdog_stop(watchdog); + kfree(watchdog->kping_timer); + watchdog->kping_timer = NULL; + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..8cf9148 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for kernel ping timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ + struct timer_list *kping_timer; /* kernel ping timer */ +#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT ^ permalink raw reply related [flat|nested] 23+ messages in thread
* watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] 2014-09-04 15:47 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space Janusz Użycki @ 2014-09-04 16:05 ` Janusz Użycki 2014-09-04 16:24 ` Janusz Użycki 2014-09-07 17:18 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space Guenter Roeck 1 sibling, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-04 16:05 UTC (permalink / raw) To: linux-kernel, Wim Van Sebroeck [-- Attachment #1: Type: text/plain, Size: 1252 bytes --] Hi, Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. * Can you comment the proposed patch? * Shoud dynamic or static timer_list be used (small structure...)? * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? best regards Janusz Uzycki [-- Attachment #2: wdt_kernel_ping.diff --] [-- Type: text/plain, Size: 4939 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..52c5f1c 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -78,6 +78,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev) stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); + stmp3xxx_wdd.status |= WATCHDOG_KERNEL_PING; ret = watchdog_register_device(&stmp3xxx_wdd); if (ret < 0) { dev_err(&pdev->dev, "cannot register watchdog device\n"); diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..d4fae2c 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/slab.h> /* for kernel ping timer (dynamic) */ +#include <linux/jiffies.h> /* for kernel ping timer */ #include "watchdog_core.h" @@ -277,6 +279,34 @@ out_ioctl: return err; } +/* kernel ping feature */ +static void watchdog_kping_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { + watchdog_ping(wdd); + /* call next ping a second or 0.5s before hardware timeout */ + mod_timer(wdd->kping_timer, wdd->timeout > 1 ? + jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); + } +} + +static void watchdog_kping_start(struct watchdog_device *wdd) +{ + if (wdd->ops->ref) + wdd->ops->ref(wdd); + watchdog_start(wdd); + watchdog_ping(wdd); + mod_timer(wdd->kping_timer, jiffies + HZ/2); +} + +static void watchdog_kping_stop(struct watchdog_device *wdd) +{ + del_timer_sync(wdd->kping_timer); + if (wdd->ops->unref) + wdd->ops->unref(wdd); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +460,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) + watchdog_kping_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +505,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { + watchdog_kping_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +562,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); + if (!watchdog->kping_timer) + return -ENOMEM; + setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); + watchdog_kping_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -575,6 +623,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { + watchdog_kping_stop(watchdog); + watchdog_stop(watchdog); + kfree(watchdog->kping_timer); + watchdog->kping_timer = NULL; + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..8cf9148 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for kernel ping timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ + struct timer_list *kping_timer; /* kernel ping timer */ +#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT ^ permalink raw reply related [flat|nested] 23+ messages in thread
* watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] 2014-09-04 16:05 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] Janusz Użycki @ 2014-09-04 16:24 ` Janusz Użycki 2014-09-04 17:23 ` Fwd: " Janusz Użycki 0 siblings, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-04 16:24 UTC (permalink / raw) To: linux-kernel, linux-watchdog; +Cc: Wim Van Sebroeck [-- Attachment #1: Type: text/plain, Size: 1245 bytes --] Hi, Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. * Can you comment the proposed patch? * Shoud dynamic or static timer_list be used (small structure...)? * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? best regards Janusz Uzycki [-- Attachment #2: wdt_kernel_ping.diff --] [-- Type: text/plain, Size: 4940 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..52c5f1c 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -78,6 +78,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev) stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); + stmp3xxx_wdd.status |= WATCHDOG_KERNEL_PING; ret = watchdog_register_device(&stmp3xxx_wdd); if (ret < 0) { dev_err(&pdev->dev, "cannot register watchdog device\n"); diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..d4fae2c 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/slab.h> /* for kernel ping timer (dynamic) */ +#include <linux/jiffies.h> /* for kernel ping timer */ #include "watchdog_core.h" @@ -277,6 +279,34 @@ out_ioctl: return err; } +/* kernel ping feature */ +static void watchdog_kping_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { + watchdog_ping(wdd); + /* call next ping a second or 0.5s before hardware timeout */ + mod_timer(wdd->kping_timer, wdd->timeout > 1 ? + jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); + } +} + +static void watchdog_kping_start(struct watchdog_device *wdd) +{ + if (wdd->ops->ref) + wdd->ops->ref(wdd); + watchdog_start(wdd); + watchdog_ping(wdd); + mod_timer(wdd->kping_timer, jiffies + HZ/2); +} + +static void watchdog_kping_stop(struct watchdog_device *wdd) +{ + del_timer_sync(wdd->kping_timer); + if (wdd->ops->unref) + wdd->ops->unref(wdd); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +460,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) + watchdog_kping_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +505,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { + watchdog_kping_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +562,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); + if (!watchdog->kping_timer) + return -ENOMEM; + setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); + watchdog_kping_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -575,6 +623,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { + watchdog_kping_stop(watchdog); + watchdog_stop(watchdog); + kfree(watchdog->kping_timer); + watchdog->kping_timer = NULL; + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..8cf9148 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for kernel ping timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ + struct timer_list *kping_timer; /* kernel ping timer */ +#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Fwd: watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] 2014-09-04 16:24 ` Janusz Użycki @ 2014-09-04 17:23 ` Janusz Użycki 2014-09-05 6:47 ` Janusz Użycki 0 siblings, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-04 17:23 UTC (permalink / raw) To: linux-watchdog [-- Attachment #1: Type: text/plain, Size: 1250 bytes --] Hi, Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. * Can you comment the proposed patch? * Shoud dynamic or static timer_list be used (small structure...)? * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? best regards Janusz Uzycki [-- Attachment #2: wdt_kernel_ping.diff --] [-- Type: text/plain, Size: 4941 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..52c5f1c 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -78,6 +78,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev) stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); + stmp3xxx_wdd.status |= WATCHDOG_KERNEL_PING; ret = watchdog_register_device(&stmp3xxx_wdd); if (ret < 0) { dev_err(&pdev->dev, "cannot register watchdog device\n"); diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..d4fae2c 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/slab.h> /* for kernel ping timer (dynamic) */ +#include <linux/jiffies.h> /* for kernel ping timer */ #include "watchdog_core.h" @@ -277,6 +279,34 @@ out_ioctl: return err; } +/* kernel ping feature */ +static void watchdog_kping_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { + watchdog_ping(wdd); + /* call next ping a second or 0.5s before hardware timeout */ + mod_timer(wdd->kping_timer, wdd->timeout > 1 ? + jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); + } +} + +static void watchdog_kping_start(struct watchdog_device *wdd) +{ + if (wdd->ops->ref) + wdd->ops->ref(wdd); + watchdog_start(wdd); + watchdog_ping(wdd); + mod_timer(wdd->kping_timer, jiffies + HZ/2); +} + +static void watchdog_kping_stop(struct watchdog_device *wdd) +{ + del_timer_sync(wdd->kping_timer); + if (wdd->ops->unref) + wdd->ops->unref(wdd); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +460,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) + watchdog_kping_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +505,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { + watchdog_kping_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +562,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); + if (!watchdog->kping_timer) + return -ENOMEM; + setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); + watchdog_kping_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -575,6 +623,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { + watchdog_kping_stop(watchdog); + watchdog_stop(watchdog); + kfree(watchdog->kping_timer); + watchdog->kping_timer = NULL; + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..8cf9148 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for kernel ping timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ + struct timer_list *kping_timer; /* kernel ping timer */ +#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Fwd: watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] 2014-09-04 17:23 ` Fwd: " Janusz Użycki @ 2014-09-05 6:47 ` Janusz Użycki 0 siblings, 0 replies; 23+ messages in thread From: Janusz Użycki @ 2014-09-05 6:47 UTC (permalink / raw) To: linux-kernel [-- Attachment #1: Type: text/plain, Size: 1246 bytes --] Hi, Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. * Can you comment the proposed patch? * Shoud dynamic or static timer_list be used (small structure...)? * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? best regards Janusz Uzycki [-- Attachment #2: wdt_kernel_ping.diff --] [-- Type: text/plain, Size: 4942 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..52c5f1c 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -78,6 +78,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev) stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); + stmp3xxx_wdd.status |= WATCHDOG_KERNEL_PING; ret = watchdog_register_device(&stmp3xxx_wdd); if (ret < 0) { dev_err(&pdev->dev, "cannot register watchdog device\n"); diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..d4fae2c 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/slab.h> /* for kernel ping timer (dynamic) */ +#include <linux/jiffies.h> /* for kernel ping timer */ #include "watchdog_core.h" @@ -277,6 +279,34 @@ out_ioctl: return err; } +/* kernel ping feature */ +static void watchdog_kping_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { + watchdog_ping(wdd); + /* call next ping a second or 0.5s before hardware timeout */ + mod_timer(wdd->kping_timer, wdd->timeout > 1 ? + jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); + } +} + +static void watchdog_kping_start(struct watchdog_device *wdd) +{ + if (wdd->ops->ref) + wdd->ops->ref(wdd); + watchdog_start(wdd); + watchdog_ping(wdd); + mod_timer(wdd->kping_timer, jiffies + HZ/2); +} + +static void watchdog_kping_stop(struct watchdog_device *wdd) +{ + del_timer_sync(wdd->kping_timer); + if (wdd->ops->unref) + wdd->ops->unref(wdd); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +460,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) + watchdog_kping_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +505,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { + watchdog_kping_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +562,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); + if (!watchdog->kping_timer) + return -ENOMEM; + setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); + watchdog_kping_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -575,6 +623,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { + watchdog_kping_stop(watchdog); + watchdog_stop(watchdog); + kfree(watchdog->kping_timer); + watchdog->kping_timer = NULL; + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..8cf9148 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for kernel ping timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ + struct timer_list *kping_timer; /* kernel ping timer */ +#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space 2014-09-04 15:47 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space Janusz Użycki 2014-09-04 16:05 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] Janusz Użycki @ 2014-09-07 17:18 ` Guenter Roeck 2014-09-08 1:14 ` watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature Janusz Użycki 1 sibling, 1 reply; 23+ messages in thread From: Guenter Roeck @ 2014-09-07 17:18 UTC (permalink / raw) To: Janusz Użycki, linux-watchdog; +Cc: Wim Van Sebroeck On 09/04/2014 08:47 AM, Janusz Użycki wrote: > Hi, > > Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. > * Can you comment the proposed patch? > * Shoud dynamic or static timer_list be used (small structure...)? > * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. > * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? > > best regards > Janusz Uzycki > I like the basic idea. Couple of comments. Please read and follow Documentation/SubmittingPatches. Long lines as above are discouraged, as is sending patches as attachment. As you can see, the patch disappears in the reply, making it very hard to comment on it. I would suggest to just modify the timer to half the timeout value, and then just ping the watchdog unconditionally whenever the callback runs. The timer should stop when the watchdog is opened, so there should not be a need to check its open status in the callback. I don't think there is a need to manipulate the driver reference count when the kernel timer starts and stops. You already run module_get and module_put during registration / unregistration. I would not call this "kernel ping". We'll need to find a better name. Proposals welcome. Something indicating the status, ie something indicating that the wdt is always running and can not be stopped. You would not update the status in the affected driver(s) by setting the flag in the probe function. You can use the status flag initialization for that purpose. Also, we'll need to know if the driver you changed is always affected or only in your system. Either case, driver patches need to be submitted separately. We'll need to tie this functionality with parallel efforts to add similar code into individual drivers. There is one patch along that line pending right now [1], and I think there is similar support in other drivers. dw_wdt is one example where the wdt can not be stopped after it was started, of_xilinx_wdt is another. Given that, there are two use states to consider: WDT is always running, and WDT can not be stopped after it was started once. We should cover both cases. The feature should have DT support from the beginning if possible, though it should be added as a separate patch in case there is a hiccup with the DT folks. It might be worthwhile exploring if the same mechanism can be used to augment user space pinging with kernel ping if the maximum timeout is too short to be handled by user space alone. That should be a separate patch, but we need to keep this use case in mind. Guenter --- [1] http://patchwork.roeck-us.net/patch/1890/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-07 17:18 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space Guenter Roeck @ 2014-09-08 1:14 ` Janusz Użycki 2014-09-08 1:18 ` Janusz Użycki 2014-09-08 3:16 ` Guenter Roeck 0 siblings, 2 replies; 23+ messages in thread From: Janusz Użycki @ 2014-09-08 1:14 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog; +Cc: Wim Van Sebroeck [-- Attachment #1: Type: text/plain, Size: 11217 bytes --] W dniu 2014-09-07 19:18, Guenter Roeck pisze: > On 09/04/2014 08:47 AM, Janusz Użycki wrote: >> Some applications require to start watchdog before userspace >> software. This patch enables such feature. Only WATCHDOG_KERNEL_PING >> flag is necessary to enable it (attached example for >> stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when >> userspace software closed watchdog using the magic character. The >> features improves kernel's reliable if hardware watchdog is available. >> * Can you comment the proposed patch? >> * Shoud dynamic or static timer_list be used (small structure...)? dynamic or static timer? >> * I also added wdd->ops->ref/unref calls but I'm afraid that even >> original code is buggy in watchdog_dev.c. Is any driver that uses the >> pointers? In my opinion watchdog_open() should call wdd->ops->ref() >> before watchdog_start() and watchdog_release() should call >> wdd->ops->unref() before module_put(). Otherwise fault is possible if >> watchdog module is unloaded. >> * I noticed that current watchdog core does not support >> suspend/resume case. If you want to use suspend without the patch you >> need to close a watchdog in userspace using the magic character >> before suspend command. With the patch you must to use >> WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is >> there any other method to suspend with watchdog? Can kernel suspend a started (stoppable) watchdog? It dissapeared in 3.x. Now userland reaction seems to be required. > I like the basic idea. Couple of comments. Thanks. Below a fixed patch code. > Please read and follow Documentation/SubmittingPatches. > Long lines as above are discouraged, as is sending patches > as attachment. As you can see, the patch disappears in the > reply, making it very hard to comment on it. fixed. I also attached the patch file - I don't trust email client in text formatting (tabs) > I would suggest to just modify the timer to half the timeout value, > and then just ping the watchdog unconditionally whenever the callback > runs. The timer should stop when the watchdog is opened, so there > should not be a need to check its open status in the callback. you are right, fixed > I don't think there is a need to manipulate the driver reference > count when the kernel timer starts and stops. You already run > module_get and module_put during registration / unregistration. removed > I would not call this "kernel ping". We'll need to find a better > name. Proposals welcome. Something indicating the status, ie something > indicating that the wdt is always running and can not be stopped. "keep on" proposed (I changed the subject also) > You would not update the status in the affected driver(s) by > setting the flag in the probe function. You can use the status > flag initialization for that purpose. fixed as example > Also, we'll need to know if > the driver you changed is always affected or only in your system. > Either case, driver patches need to be submitted separately. I used to add similar code to different wachdog drivers for embedded boards. This time I work under quite new kernel 3.14 and therefore I finally decided to submit a patch. The feature should be generally used for all hardware watchdogs. Unfortunately some drivers uses own timers, eg. to increase timeout period. Another exception is softdog. > We'll need to tie this functionality with parallel efforts > to add similar code into individual drivers. There is one patch > along that line pending right now [1], I saw it and agree - they can cooperate > and I think there is similar > support in other drivers. dw_wdt is one example where the wdt can > not be stopped after it was started, of_xilinx_wdt is another. So it is similar to at91sam9g20 as I remember but Atmel's watchdog driver uses constant hardware timeout stretched by the driver. > Given that, there are two use states to consider: WDT is always running, > and WDT can not be stopped after it was started once. We should cover > both cases. and third: ability to stop watchdog (if possible) for suspend > The feature should have DT support from the beginning if possible, > though it should be added as a separate patch in case there is > a hiccup with the DT folks. Can you give more details? > It might be worthwhile exploring if the same mechanism can be used > to augment user space pinging with kernel ping if the maximum timeout > is too short to be handled by user space alone. That should be a > separate patch, but we need to keep this use case in mind. I agree - unification is welcomed. Janusz > > Guenter > > --- > [1] http://patchwork.roeck-us.net/patch/1890/ Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..3546f03 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { .ops = &stmp3xxx_wdt_ops, .min_timeout = 1, .max_timeout = STMP3XXX_MAX_TIMEOUT, - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, }; static int stmp3xxx_wdt_probe(struct platform_device *pdev) diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index d4fae2c..f16955d 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,8 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ -#include <linux/slab.h> /* for kernel ping timer (dynamic) */ -#include <linux/jiffies.h> /* for kernel ping timer */ +#include <linux/slab.h> /* for 'keep on' timer (dynamic) */ +#include <linux/jiffies.h> /* for 'keep on' timer */ #include "watchdog_core.h" @@ -279,32 +279,27 @@ out_ioctl: return err; } -/* kernel ping feature */ -static void watchdog_kping_timer_cb(unsigned long data) +/* 'keep on' feature */ +static void watchdog_keepon_timer_cb(unsigned long data) { struct watchdog_device *wdd = (struct watchdog_device *)data; - if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { - watchdog_ping(wdd); - /* call next ping a second or 0.5s before hardware timeout */ - mod_timer(wdd->kping_timer, wdd->timeout > 1 ? - jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); - } + watchdog_ping(wdd); + /* call next ping half the timeout value */ + mod_timer(wdd->keepon_timer, + jiffies + wdd->timeout * (HZ/2)); } -static void watchdog_kping_start(struct watchdog_device *wdd) +static void watchdog_keepon_start(struct watchdog_device *wdd) { - if (wdd->ops->ref) - wdd->ops->ref(wdd); watchdog_start(wdd); + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code below: */ watchdog_ping(wdd); - mod_timer(wdd->kping_timer, jiffies + HZ/2); + mod_timer(wdd->keepon_timer, jiffies + HZ/2); } -static void watchdog_kping_stop(struct watchdog_device *wdd) +static void watchdog_keepon_stop(struct watchdog_device *wdd) { - del_timer_sync(wdd->kping_timer); - if (wdd->ops->unref) - wdd->ops->unref(wdd); + del_timer_sync(wdd->keepon_timer); } /* @@ -460,8 +455,8 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; - if (test_bit(WDOG_KERNEL_PING, &wdd->status)) - watchdog_kping_stop(wdd); + if (test_bit(WDOG_KEEP_ON, &wdd->status)) + watchdog_keepon_stop(wdd); err = watchdog_start(wdd); if (err < 0) @@ -506,8 +501,8 @@ static int watchdog_release(struct inode *inode, struct file *file) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || !(wdd->info->options & WDIOF_MAGICCLOSE)) { - if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { - watchdog_kping_start(wdd); + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { + watchdog_keepon_start(wdd); err = 0; } else err = watchdog_stop(wdd); @@ -562,14 +557,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; - if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { if (!try_module_get(watchdog->ops->owner)) return -ENODEV; - watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); - if (!watchdog->kping_timer) + watchdog->keepon_timer = + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); + if (!watchdog->keepon_timer) return -ENOMEM; - setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); - watchdog_kping_start(watchdog); + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, + (unsigned long)watchdog); + watchdog_keepon_start(watchdog); } if (watchdog->id == 0) { @@ -624,11 +621,11 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) old_wdd = NULL; } - if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { - watchdog_kping_stop(watchdog); - watchdog_stop(watchdog); - kfree(watchdog->kping_timer); - watchdog->kping_timer = NULL; + if (test_bit(WDOG_KEEP_ON, &watchdog->status) || + watchdog->keepon_timer) { + watchdog_keepon_stop(watchdog); + kfree(watchdog->keepon_timer); + watchdog->keepon_timer = NULL; watchdog_stop(watchdog); module_put(watchdog->ops->owner); } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 8cf9148..57b552a 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,7 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> -#include <linux/timer.h> /* for kernel ping timer */ +#include <linux/timer.h> /* for 'keep on' timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -96,9 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ - struct timer_list *kping_timer; /* kernel ping timer */ -#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) +#define WDOG_KEEP_ON 5 /* Is 'keep on' feature set? */ + struct timer_list *keepon_timer;/* 'keep on' timer */ +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT [-- Attachment #2: watchdog_dev.patch --] [-- Type: text/plain, Size: 5983 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..3546f03 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { .ops = &stmp3xxx_wdt_ops, .min_timeout = 1, .max_timeout = STMP3XXX_MAX_TIMEOUT, - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, }; static int stmp3xxx_wdt_probe(struct platform_device *pdev) diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index d4fae2c..f16955d 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,8 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ -#include <linux/slab.h> /* for kernel ping timer (dynamic) */ -#include <linux/jiffies.h> /* for kernel ping timer */ +#include <linux/slab.h> /* for 'keep on' timer (dynamic) */ +#include <linux/jiffies.h> /* for 'keep on' timer */ #include "watchdog_core.h" @@ -279,32 +279,27 @@ out_ioctl: return err; } -/* kernel ping feature */ -static void watchdog_kping_timer_cb(unsigned long data) +/* 'keep on' feature */ +static void watchdog_keepon_timer_cb(unsigned long data) { struct watchdog_device *wdd = (struct watchdog_device *)data; - if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { - watchdog_ping(wdd); - /* call next ping a second or 0.5s before hardware timeout */ - mod_timer(wdd->kping_timer, wdd->timeout > 1 ? - jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); - } + watchdog_ping(wdd); + /* call next ping half the timeout value */ + mod_timer(wdd->keepon_timer, + jiffies + wdd->timeout * (HZ/2)); } -static void watchdog_kping_start(struct watchdog_device *wdd) +static void watchdog_keepon_start(struct watchdog_device *wdd) { - if (wdd->ops->ref) - wdd->ops->ref(wdd); watchdog_start(wdd); + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code below: */ watchdog_ping(wdd); - mod_timer(wdd->kping_timer, jiffies + HZ/2); + mod_timer(wdd->keepon_timer, jiffies + HZ/2); } -static void watchdog_kping_stop(struct watchdog_device *wdd) +static void watchdog_keepon_stop(struct watchdog_device *wdd) { - del_timer_sync(wdd->kping_timer); - if (wdd->ops->unref) - wdd->ops->unref(wdd); + del_timer_sync(wdd->keepon_timer); } /* @@ -460,8 +455,8 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; - if (test_bit(WDOG_KERNEL_PING, &wdd->status)) - watchdog_kping_stop(wdd); + if (test_bit(WDOG_KEEP_ON, &wdd->status)) + watchdog_keepon_stop(wdd); err = watchdog_start(wdd); if (err < 0) @@ -506,8 +501,8 @@ static int watchdog_release(struct inode *inode, struct file *file) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || !(wdd->info->options & WDIOF_MAGICCLOSE)) { - if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { - watchdog_kping_start(wdd); + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { + watchdog_keepon_start(wdd); err = 0; } else err = watchdog_stop(wdd); @@ -562,14 +557,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; - if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { if (!try_module_get(watchdog->ops->owner)) return -ENODEV; - watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); - if (!watchdog->kping_timer) + watchdog->keepon_timer = + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); + if (!watchdog->keepon_timer) return -ENOMEM; - setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); - watchdog_kping_start(watchdog); + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, + (unsigned long)watchdog); + watchdog_keepon_start(watchdog); } if (watchdog->id == 0) { @@ -624,11 +621,11 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) old_wdd = NULL; } - if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { - watchdog_kping_stop(watchdog); - watchdog_stop(watchdog); - kfree(watchdog->kping_timer); - watchdog->kping_timer = NULL; + if (test_bit(WDOG_KEEP_ON, &watchdog->status) || + watchdog->keepon_timer) { + watchdog_keepon_stop(watchdog); + kfree(watchdog->keepon_timer); + watchdog->keepon_timer = NULL; watchdog_stop(watchdog); module_put(watchdog->ops->owner); } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 8cf9148..57b552a 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,7 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> -#include <linux/timer.h> /* for kernel ping timer */ +#include <linux/timer.h> /* for 'keep on' timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -96,9 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ - struct timer_list *kping_timer; /* kernel ping timer */ -#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) +#define WDOG_KEEP_ON 5 /* Is 'keep on' feature set? */ + struct timer_list *keepon_timer;/* 'keep on' timer */ +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-08 1:14 ` watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature Janusz Użycki @ 2014-09-08 1:18 ` Janusz Użycki 2014-09-08 3:24 ` Guenter Roeck 2014-09-08 3:16 ` Guenter Roeck 1 sibling, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-08 1:18 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog; +Cc: Wim Van Sebroeck [-- Attachment #1: Type: text/plain, Size: 10040 bytes --] I diffed to bad git's HEAD, sorry. I resend the patch. Janusz W dniu 2014-09-08 03:14, Janusz Użycki pisze: > W dniu 2014-09-07 19:18, Guenter Roeck pisze: >> On 09/04/2014 08:47 AM, Janusz Użycki wrote: >>> Some applications require to start watchdog before userspace >>> software. This patch enables such feature. Only WATCHDOG_KERNEL_PING >>> flag is necessary to enable it (attached example for >>> stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when >>> userspace software closed watchdog using the magic character. The >>> features improves kernel's reliable if hardware watchdog is available. >>> * Can you comment the proposed patch? >>> * Shoud dynamic or static timer_list be used (small structure...)? > dynamic or static timer? > >>> * I also added wdd->ops->ref/unref calls but I'm afraid that even >>> original code is buggy in watchdog_dev.c. Is any driver that uses >>> the pointers? In my opinion watchdog_open() should call >>> wdd->ops->ref() before watchdog_start() and watchdog_release() >>> should call wdd->ops->unref() before module_put(). Otherwise fault >>> is possible if watchdog module is unloaded. >>> * I noticed that current watchdog core does not support >>> suspend/resume case. If you want to use suspend without the patch >>> you need to close a watchdog in userspace using the magic character >>> before suspend command. With the patch you must to use >>> WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is >>> there any other method to suspend with watchdog? > Can kernel suspend a started (stoppable) watchdog? It dissapeared in > 3.x. Now userland reaction seems to be required. > >> I like the basic idea. Couple of comments. > Thanks. Below a fixed patch code. > >> Please read and follow Documentation/SubmittingPatches. >> Long lines as above are discouraged, as is sending patches >> as attachment. As you can see, the patch disappears in the >> reply, making it very hard to comment on it. > fixed. > I also attached the patch file - I don't trust email client in text > formatting (tabs) > >> I would suggest to just modify the timer to half the timeout value, >> and then just ping the watchdog unconditionally whenever the callback >> runs. The timer should stop when the watchdog is opened, so there >> should not be a need to check its open status in the callback. > you are right, fixed > >> I don't think there is a need to manipulate the driver reference >> count when the kernel timer starts and stops. You already run >> module_get and module_put during registration / unregistration. > removed > >> I would not call this "kernel ping". We'll need to find a better >> name. Proposals welcome. Something indicating the status, ie something >> indicating that the wdt is always running and can not be stopped. > "keep on" proposed (I changed the subject also) > >> You would not update the status in the affected driver(s) by >> setting the flag in the probe function. You can use the status >> flag initialization for that purpose. > fixed as example > >> Also, we'll need to know if >> the driver you changed is always affected or only in your system. >> Either case, driver patches need to be submitted separately. > I used to add similar code to different wachdog drivers for embedded > boards. This time I work under quite new kernel 3.14 and therefore I > finally decided to submit a patch. > The feature should be generally used for all hardware watchdogs. > Unfortunately some drivers uses own timers, eg. to increase timeout > period. Another exception is softdog. > >> We'll need to tie this functionality with parallel efforts >> to add similar code into individual drivers. There is one patch >> along that line pending right now [1], > I saw it and agree - they can cooperate > >> and I think there is similar >> support in other drivers. dw_wdt is one example where the wdt can >> not be stopped after it was started, of_xilinx_wdt is another. > So it is similar to at91sam9g20 as I remember but Atmel's watchdog > driver uses constant hardware timeout stretched by the driver. > >> Given that, there are two use states to consider: WDT is always running, >> and WDT can not be stopped after it was started once. We should cover >> both cases. > and third: ability to stop watchdog (if possible) for suspend > >> The feature should have DT support from the beginning if possible, >> though it should be added as a separate patch in case there is >> a hiccup with the DT folks. > Can you give more details? > >> It might be worthwhile exploring if the same mechanism can be used >> to augment user space pinging with kernel ping if the maximum timeout >> is too short to be handled by user space alone. That should be a >> separate patch, but we need to keep this use case in mind. > I agree - unification is welcomed. > > Janusz >> >> Guenter >> >> --- >> [1] http://patchwork.roeck-us.net/patch/1890/ > Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..3546f03 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { .ops = &stmp3xxx_wdt_ops, .min_timeout = 1, .max_timeout = STMP3XXX_MAX_TIMEOUT, - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, }; static int stmp3xxx_wdt_probe(struct platform_device *pdev) diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..f16955d 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/slab.h> /* for 'keep on' timer (dynamic) */ +#include <linux/jiffies.h> /* for 'keep on' timer */ #include "watchdog_core.h" @@ -277,6 +279,29 @@ out_ioctl: return err; } +/* 'keep on' feature */ +static void watchdog_keepon_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + watchdog_ping(wdd); + /* call next ping half the timeout value */ + mod_timer(wdd->keepon_timer, + jiffies + wdd->timeout * (HZ/2)); +} + +static void watchdog_keepon_start(struct watchdog_device *wdd) +{ + watchdog_start(wdd); + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code below: */ + watchdog_ping(wdd); + mod_timer(wdd->keepon_timer, jiffies + HZ/2); +} + +static void watchdog_keepon_stop(struct watchdog_device *wdd) +{ + del_timer_sync(wdd->keepon_timer); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +455,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KEEP_ON, &wdd->status)) + watchdog_keepon_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +500,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { + watchdog_keepon_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +557,18 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + watchdog->keepon_timer = + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); + if (!watchdog->keepon_timer) + return -ENOMEM; + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, + (unsigned long)watchdog); + watchdog_keepon_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -575,6 +620,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KEEP_ON, &watchdog->status) || + watchdog->keepon_timer) { + watchdog_keepon_stop(watchdog); + kfree(watchdog->keepon_timer); + watchdog->keepon_timer = NULL; + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..57b552a 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for 'keep on' timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,9 @@ 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_KEEP_ON 5 /* Is 'keep on' feature set? */ + struct timer_list *keepon_timer;/* 'keep on' timer */ +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT [-- Attachment #2: watchdog_dev.patch --] [-- Type: text/plain, Size: 4688 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..3546f03 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { .ops = &stmp3xxx_wdt_ops, .min_timeout = 1, .max_timeout = STMP3XXX_MAX_TIMEOUT, - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, }; static int stmp3xxx_wdt_probe(struct platform_device *pdev) diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..f16955d 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,8 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/slab.h> /* for 'keep on' timer (dynamic) */ +#include <linux/jiffies.h> /* for 'keep on' timer */ #include "watchdog_core.h" @@ -277,6 +279,29 @@ out_ioctl: return err; } +/* 'keep on' feature */ +static void watchdog_keepon_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + watchdog_ping(wdd); + /* call next ping half the timeout value */ + mod_timer(wdd->keepon_timer, + jiffies + wdd->timeout * (HZ/2)); +} + +static void watchdog_keepon_start(struct watchdog_device *wdd) +{ + watchdog_start(wdd); + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code below: */ + watchdog_ping(wdd); + mod_timer(wdd->keepon_timer, jiffies + HZ/2); +} + +static void watchdog_keepon_stop(struct watchdog_device *wdd) +{ + del_timer_sync(wdd->keepon_timer); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +455,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KEEP_ON, &wdd->status)) + watchdog_keepon_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +500,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { + watchdog_keepon_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +557,18 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + watchdog->keepon_timer = + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); + if (!watchdog->keepon_timer) + return -ENOMEM; + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, + (unsigned long)watchdog); + watchdog_keepon_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -575,6 +620,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KEEP_ON, &watchdog->status) || + watchdog->keepon_timer) { + watchdog_keepon_stop(watchdog); + kfree(watchdog->keepon_timer); + watchdog->keepon_timer = NULL; + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..57b552a 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for 'keep on' timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,9 @@ 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_KEEP_ON 5 /* Is 'keep on' feature set? */ + struct timer_list *keepon_timer;/* 'keep on' timer */ +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) }; #ifdef CONFIG_WATCHDOG_NOWAYOUT ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-08 1:18 ` Janusz Użycki @ 2014-09-08 3:24 ` Guenter Roeck 0 siblings, 0 replies; 23+ messages in thread From: Guenter Roeck @ 2014-09-08 3:24 UTC (permalink / raw) To: Janusz Użycki, linux-watchdog; +Cc: Wim Van Sebroeck On 09/07/2014 06:18 PM, Janusz Użycki wrote: > I diffed to bad git's HEAD, sorry. I resend the patch. > \ Ok, more comments inline. Guenter > Janusz > > W dniu 2014-09-08 03:14, Janusz Użycki pisze: >> W dniu 2014-09-07 19:18, Guenter Roeck pisze: >>> On 09/04/2014 08:47 AM, Janusz Użycki wrote: >>>> Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. >>>> * Can you comment the proposed patch? >>>> * Shoud dynamic or static timer_list be used (small structure...)? >> dynamic or static timer? >> >>>> * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. >>>> * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? >> Can kernel suspend a started (stoppable) watchdog? It dissapeared in 3.x. Now userland reaction seems to be required. >> >>> I like the basic idea. Couple of comments. >> Thanks. Below a fixed patch code. >> >>> Please read and follow Documentation/SubmittingPatches. >>> Long lines as above are discouraged, as is sending patches >>> as attachment. As you can see, the patch disappears in the >>> reply, making it very hard to comment on it. >> fixed. >> I also attached the patch file - I don't trust email client in text formatting (tabs) >> >>> I would suggest to just modify the timer to half the timeout value, >>> and then just ping the watchdog unconditionally whenever the callback >>> runs. The timer should stop when the watchdog is opened, so there >>> should not be a need to check its open status in the callback. >> you are right, fixed >> >>> I don't think there is a need to manipulate the driver reference >>> count when the kernel timer starts and stops. You already run >>> module_get and module_put during registration / unregistration. >> removed >> >>> I would not call this "kernel ping". We'll need to find a better >>> name. Proposals welcome. Something indicating the status, ie something >>> indicating that the wdt is always running and can not be stopped. >> "keep on" proposed (I changed the subject also) >> >>> You would not update the status in the affected driver(s) by >>> setting the flag in the probe function. You can use the status >>> flag initialization for that purpose. >> fixed as example >> >>> Also, we'll need to know if >>> the driver you changed is always affected or only in your system. >>> Either case, driver patches need to be submitted separately. >> I used to add similar code to different wachdog drivers for embedded boards. This time I work under quite new kernel 3.14 and therefore I finally decided to submit a patch. >> The feature should be generally used for all hardware watchdogs. Unfortunately some drivers uses own timers, eg. to increase timeout period. Another exception is softdog. >> >>> We'll need to tie this functionality with parallel efforts >>> to add similar code into individual drivers. There is one patch >>> along that line pending right now [1], >> I saw it and agree - they can cooperate >> >>> and I think there is similar >>> support in other drivers. dw_wdt is one example where the wdt can >>> not be stopped after it was started, of_xilinx_wdt is another. >> So it is similar to at91sam9g20 as I remember but Atmel's watchdog driver uses constant hardware timeout stretched by the driver. >> >>> Given that, there are two use states to consider: WDT is always running, >>> and WDT can not be stopped after it was started once. We should cover >>> both cases. >> and third: ability to stop watchdog (if possible) for suspend >> >>> The feature should have DT support from the beginning if possible, >>> though it should be added as a separate patch in case there is >>> a hiccup with the DT folks. >> Can you give more details? >> >>> It might be worthwhile exploring if the same mechanism can be used >>> to augment user space pinging with kernel ping if the maximum timeout >>> is too short to be handled by user space alone. That should be a >>> separate patch, but we need to keep this use case in mind. >> I agree - unification is welcomed. >> >> Janusz >>> >>> Guenter >>> >>> --- >>> [1] http://patchwork.roeck-us.net/patch/1890/ >> > > Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> > > diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > index b4d6b34..3546f03 100644 > --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { > .ops = &stmp3xxx_wdt_ops, > .min_timeout = 1, > .max_timeout = STMP3XXX_MAX_TIMEOUT, > - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, > + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, > }; > > static int stmp3xxx_wdt_probe(struct platform_device *pdev) > diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c > index 6aaefba..f16955d 100644 > --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c > +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c > @@ -41,6 +41,8 @@ > #include <linux/miscdevice.h> /* For handling misc devices */ > #include <linux/init.h> /* For __init/__exit/... */ > #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ > +#include <linux/slab.h> /* for 'keep on' timer (dynamic) */ > +#include <linux/jiffies.h> /* for 'keep on' timer */ > > #include "watchdog_core.h" > > @@ -277,6 +279,29 @@ out_ioctl: > return err; > } > > +/* 'keep on' feature */ > +static void watchdog_keepon_timer_cb(unsigned long data) > +{ > + struct watchdog_device *wdd = (struct watchdog_device *)data; > + watchdog_ping(wdd); I don't think this works. It bails out if the watchdog is not active, and active means opened from user space. Correct me if I am wrong. > + /* call next ping half the timeout value */ > + mod_timer(wdd->keepon_timer, > + jiffies + wdd->timeout * (HZ/2)); > +} > + > +static void watchdog_keepon_start(struct watchdog_device *wdd) > +{ > + watchdog_start(wdd); > + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code below: */ > + watchdog_ping(wdd); > + mod_timer(wdd->keepon_timer, jiffies + HZ/2); > +} > + > +static void watchdog_keepon_stop(struct watchdog_device *wdd) > +{ > + del_timer_sync(wdd->keepon_timer); > +} > + > /* > * watchdog_write: writes to the watchdog. > * @file: file from VFS > @@ -430,6 +455,9 @@ static int watchdog_open(struct inode *inode, struct file *file) > if (!try_module_get(wdd->ops->owner)) > goto out; > > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) > + watchdog_keepon_stop(wdd); > + > err = watchdog_start(wdd); > if (err < 0) > goto out_mod; > @@ -472,8 +500,13 @@ static int watchdog_release(struct inode *inode, struct file *file) > if (!test_bit(WDOG_ACTIVE, &wdd->status)) > err = 0; > else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > - !(wdd->info->options & WDIOF_MAGICCLOSE)) > - err = watchdog_stop(wdd); > + !(wdd->info->options & WDIOF_MAGICCLOSE)) { > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + watchdog_keepon_start(wdd); > + err = 0; > + } else > + err = watchdog_stop(wdd); > + } > > /* If the watchdog was not stopped, send a keepalive ping */ > if (err < 0) { > @@ -524,6 +557,18 @@ int watchdog_dev_register(struct watchdog_device *watchdog) > { > int err, devno; > > + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { > + if (!try_module_get(watchdog->ops->owner)) > + return -ENODEV; > + watchdog->keepon_timer = > + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); > + if (!watchdog->keepon_timer) > + return -ENOMEM; This really suggests it should be static, to avoid such complexity and potential errors. Also, you leak at least the module reference on errors (here and later). > + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, > + (unsigned long)watchdog); > + watchdog_keepon_start(watchdog); By setting the timer here prior to completing registration you risk leaking the timer on error. Also, there is strictly speaking no guarantee that the timer doesn't fire before registration is complete, so this results in a race condition. > + } > + > if (watchdog->id == 0) { > old_wdd = watchdog; > watchdog_miscdev.parent = watchdog->parent; > @@ -575,6 +620,15 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) > misc_deregister(&watchdog_miscdev); > old_wdd = NULL; > } > + > + if (test_bit(WDOG_KEEP_ON, &watchdog->status) || > + watchdog->keepon_timer) { > + watchdog_keepon_stop(watchdog); > + kfree(watchdog->keepon_timer); > + watchdog->keepon_timer = NULL; > + watchdog_stop(watchdog); > + module_put(watchdog->ops->owner); > + } > return 0; > } > > diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h > index 2a3038e..57b552a 100644 > --- a/linux-3.14.17/include/linux/watchdog.h > +++ b/linux-3.14.17/include/linux/watchdog.h > @@ -12,6 +12,7 @@ > #include <linux/bitops.h> > #include <linux/device.h> > #include <linux/cdev.h> > +#include <linux/timer.h> /* for 'keep on' timer */ > #include <uapi/linux/watchdog.h> > > struct watchdog_ops; > @@ -95,6 +96,9 @@ 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_KEEP_ON 5 /* Is 'keep on' feature set? */ > + struct timer_list *keepon_timer;/* 'keep on' timer */ > +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) > }; > > #ifdef CONFIG_WATCHDOG_NOWAYOUT > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-08 1:14 ` watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature Janusz Użycki 2014-09-08 1:18 ` Janusz Użycki @ 2014-09-08 3:16 ` Guenter Roeck 2014-09-08 12:14 ` Janusz Użycki 2014-09-17 11:09 ` Janusz Użycki 1 sibling, 2 replies; 23+ messages in thread From: Guenter Roeck @ 2014-09-08 3:16 UTC (permalink / raw) To: Janusz Użycki, linux-watchdog; +Cc: Wim Van Sebroeck On 09/07/2014 06:14 PM, Janusz Użycki wrote: > W dniu 2014-09-07 19:18, Guenter Roeck pisze: >> On 09/04/2014 08:47 AM, Janusz Użycki wrote: >>> Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available. >>> * Can you comment the proposed patch? >>> * Shoud dynamic or static timer_list be used (small structure...)? > dynamic or static timer? > I am fine with dynamic, though I would probably make it static. It is not as if there are dozens or hundreds of watchdogs in the system where it would make much of a difference. The code itself is already much larger than the size of the data structure. >>> * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded. >>> * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog? > Can kernel suspend a started (stoppable) watchdog? It dissapeared in 3.x. Now userland reaction seems to be required. > Really ? I see a number of watchdog drivers implementing it. >> I like the basic idea. Couple of comments. > Thanks. Below a fixed patch code. > Uuh ... please no patches on top of patches. >> Please read and follow Documentation/SubmittingPatches. >> Long lines as above are discouraged, as is sending patches >> as attachment. As you can see, the patch disappears in the >> reply, making it very hard to comment on it. > fixed. > I also attached the patch file - I don't trust email client in text formatting (tabs) > You'll have to sort that out. Try using git send-email, for example. >> I would suggest to just modify the timer to half the timeout value, >> and then just ping the watchdog unconditionally whenever the callback >> runs. The timer should stop when the watchdog is opened, so there >> should not be a need to check its open status in the callback. > you are right, fixed > >> I don't think there is a need to manipulate the driver reference >> count when the kernel timer starts and stops. You already run >> module_get and module_put during registration / unregistration. > removed > >> I would not call this "kernel ping". We'll need to find a better >> name. Proposals welcome. Something indicating the status, ie something >> indicating that the wdt is always running and can not be stopped. > "keep on" proposed (I changed the subject also) > Hmm ... but that isn't right either. It should reflect that the watchdog is always active / running. >> You would not update the status in the affected driver(s) by >> setting the flag in the probe function. You can use the status >> flag initialization for that purpose. > fixed as example > >> Also, we'll need to know if >> the driver you changed is always affected or only in your system. >> Either case, driver patches need to be submitted separately. > I used to add similar code to different wachdog drivers for embedded boards. This time I work under quite new kernel 3.14 and therefore I finally decided to submit a patch. > The feature should be generally used for all hardware watchdogs. Unfortunately some drivers uses own timers, eg. to increase timeout period. Another exception is softdog. > You still have trouble with your line length. Makes it hard for me (and everyone else) to read your text. >> We'll need to tie this functionality with parallel efforts >> to add similar code into individual drivers. There is one patch >> along that line pending right now [1], > I saw it and agree - they can cooperate > >> and I think there is similar >> support in other drivers. dw_wdt is one example where the wdt can >> not be stopped after it was started, of_xilinx_wdt is another. > So it is similar to at91sam9g20 as I remember but Atmel's watchdog driver uses constant hardware timeout stretched by the driver. > >> Given that, there are two use states to consider: WDT is always running, >> and WDT can not be stopped after it was started once. We should cover >> both cases. > and third: ability to stop watchdog (if possible) for suspend Today that is handled by individual drivers. I have no idea though how that would or could be handled if the watchdog can not be stopped at all. Seems to be a contradiction to me. Anyway, you'll probably need some code which suspends pinging the watchdog in suspend. But that assumes that the watchdog can be stopped after all, at least during suspend. > >> The feature should have DT support from the beginning if possible, >> though it should be added as a separate patch in case there is >> a hiccup with the DT folks. > Can you give more details? > We'll have to determine bindings which are acceptable for DT. >> It might be worthwhile exploring if the same mechanism can be used >> to augment user space pinging with kernel ping if the maximum timeout >> is too short to be handled by user space alone. That should be a >> separate patch, but we need to keep this use case in mind. > I agree - unification is welcomed. > > Janusz >> >> Guenter >> >> --- >> [1] http://patchwork.roeck-us.net/patch/1890/ > > Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> > > diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > index b4d6b34..3546f03 100644 > --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { > .ops = &stmp3xxx_wdt_ops, > .min_timeout = 1, > .max_timeout = STMP3XXX_MAX_TIMEOUT, > - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, > + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, Again, this should be a separate patch. WATCHDOG_ALWAYS_ACTIVE, maybe ? > }; > > static int stmp3xxx_wdt_probe(struct platform_device *pdev) > diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c > index d4fae2c..f16955d 100644 > --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c > +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c > @@ -41,8 +41,8 @@ > #include <linux/miscdevice.h> /* For handling misc devices */ > #include <linux/init.h> /* For __init/__exit/... */ > #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ > -#include <linux/slab.h> /* for kernel ping timer (dynamic) */ > -#include <linux/jiffies.h> /* for kernel ping timer */ > +#include <linux/slab.h> /* for 'keep on' timer (dynamic) */ > +#include <linux/jiffies.h> /* for 'keep on' timer */ > > #include "watchdog_core.h" > > @@ -279,32 +279,27 @@ out_ioctl: > return err; > } > > -/* kernel ping feature */ > -static void watchdog_kping_timer_cb(unsigned long data) > +/* 'keep on' feature */ > +static void watchdog_keepon_timer_cb(unsigned long data) No problem calling this 'ping'. That describes what the function is doing. 'k' in 'kping' is redundant, though - presumably all kernel code runs in the kernel ;-). > { > struct watchdog_device *wdd = (struct watchdog_device *)data; > - if (test_bit(WDOG_KERNEL_PING, &wdd->status) && !test_bit(WDOG_DEV_OPEN, &wdd->status)) { > - watchdog_ping(wdd); > - /* call next ping a second or 0.5s before hardware timeout */ > - mod_timer(wdd->kping_timer, wdd->timeout > 1 ? > - jiffies + (wdd->timeout - 1) * HZ : jiffies + HZ/2); > - } > + watchdog_ping(wdd); > + /* call next ping half the timeout value */ > + mod_timer(wdd->keepon_timer, > + jiffies + wdd->timeout * (HZ/2)); Watch out for coding style (spaces befor and after operators). Better use something like jiffies + msecs_to_jiffies(wdd->timeout * 1000) > } > > -static void watchdog_kping_start(struct watchdog_device *wdd) > +static void watchdog_keepon_start(struct watchdog_device *wdd) > { > - if (wdd->ops->ref) > - wdd->ops->ref(wdd); > watchdog_start(wdd); > + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code below: */ > watchdog_ping(wdd); > - mod_timer(wdd->kping_timer, jiffies + HZ/2); > + mod_timer(wdd->keepon_timer, jiffies + HZ/2); Why only 500ms timeout here ? > } > > -static void watchdog_kping_stop(struct watchdog_device *wdd) > +static void watchdog_keepon_stop(struct watchdog_device *wdd) > { > - del_timer_sync(wdd->kping_timer); > - if (wdd->ops->unref) > - wdd->ops->unref(wdd); > + del_timer_sync(wdd->keepon_timer); > } > > /* > @@ -460,8 +455,8 @@ static int watchdog_open(struct inode *inode, struct file *file) > if (!try_module_get(wdd->ops->owner)) > goto out; > > - if (test_bit(WDOG_KERNEL_PING, &wdd->status)) > - watchdog_kping_stop(wdd); > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) > + watchdog_keepon_stop(wdd); > > err = watchdog_start(wdd); > if (err < 0) > @@ -506,8 +501,8 @@ static int watchdog_release(struct inode *inode, struct file *file) > err = 0; > else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > !(wdd->info->options & WDIOF_MAGICCLOSE)) { > - if (test_bit(WDOG_KERNEL_PING, &wdd->status)) { > - watchdog_kping_start(wdd); > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + watchdog_keepon_start(wdd); > err = 0; > } else > err = watchdog_stop(wdd); > @@ -562,14 +557,16 @@ int watchdog_dev_register(struct watchdog_device *watchdog) > { > int err, devno; > > - if (test_bit(WDOG_KERNEL_PING, &watchdog->status)) { > + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { > if (!try_module_get(watchdog->ops->owner)) > return -ENODEV; > - watchdog->kping_timer = kzalloc(sizeof(*watchdog->kping_timer), GFP_KERNEL); > - if (!watchdog->kping_timer) > + watchdog->keepon_timer = > + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); > + if (!watchdog->keepon_timer) > return -ENOMEM; > - setup_timer(watchdog->kping_timer, watchdog_kping_timer_cb, (unsigned long)watchdog); > - watchdog_kping_start(watchdog); > + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, > + (unsigned long)watchdog); > + watchdog_keepon_start(watchdog); > } > > if (watchdog->id == 0) { > @@ -624,11 +621,11 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) > old_wdd = NULL; > } > > - if (test_bit(WDOG_KERNEL_PING, &watchdog->status) || watchdog->kping_timer) { > - watchdog_kping_stop(watchdog); > - watchdog_stop(watchdog); > - kfree(watchdog->kping_timer); > - watchdog->kping_timer = NULL; > + if (test_bit(WDOG_KEEP_ON, &watchdog->status) || > + watchdog->keepon_timer) { > + watchdog_keepon_stop(watchdog); > + kfree(watchdog->keepon_timer); > + watchdog->keepon_timer = NULL; > watchdog_stop(watchdog); > module_put(watchdog->ops->owner); > } > diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h > index 8cf9148..57b552a 100644 > --- a/linux-3.14.17/include/linux/watchdog.h > +++ b/linux-3.14.17/include/linux/watchdog.h > @@ -12,7 +12,7 @@ > #include <linux/bitops.h> > #include <linux/device.h> > #include <linux/cdev.h> > -#include <linux/timer.h> /* for kernel ping timer */ > +#include <linux/timer.h> /* for 'keep on' timer */ > #include <uapi/linux/watchdog.h> > > struct watchdog_ops; > @@ -96,9 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device not opened by userland */ > - struct timer_list *kping_timer; /* kernel ping timer */ > -#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) > +#define WDOG_KEEP_ON 5 /* Is 'keep on' feature set? */ > + struct timer_list *keepon_timer;/* 'keep on' timer */ > +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) Move this define out of the struct, please. > }; > > #ifdef CONFIG_WATCHDOG_NOWAYOUT > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-08 3:16 ` Guenter Roeck @ 2014-09-08 12:14 ` Janusz Użycki 2014-09-10 17:24 ` Janusz Użycki 2014-09-17 11:09 ` Janusz Użycki 1 sibling, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-08 12:14 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog; +Cc: Wim Van Sebroeck [-- Attachment #1: Type: text/plain, Size: 12316 bytes --] W dniu 2014-09-08 05:16, Guenter Roeck pisze: >>>> * Shoud dynamic or static timer_list be used (small structure...)? >> dynamic or static timer? > I am fine with dynamic, though I would probably make it static. > It is not as if there are dozens or hundreds of watchdogs > in the system where it would make much of a difference. > The code itself is already much larger than the size of > the data structure. ok, switched to static >> Can kernel suspend a started (stoppable) watchdog? It dissapeared in >> 3.x. Now userland reaction seems to be required. > Really ? I see a number of watchdog drivers implementing it. I thought it was discarded and PM in other watchdog drivers is deprecated when I didn't find it for not so old stmp3xxx_rtc_wdt.c. For suspend/resume drivers call own stop/start functions and use watchdog_active() - good but it could be likely unified. Some drivers duplicate active flag, eg. omap_wdt.c: omap_wdt_users. >> I also attached the patch file - I don't trust email client in text >> formatting (tabs) > You'll have to sort that out. Try using git send-email, for example. I found Thunderbird is accepted but I'm not sure. >>> I would not call this "kernel ping". We'll need to find a better >>> name. Proposals welcome. Something indicating the status, ie something >>> indicating that the wdt is always running and can not be stopped. >> "keep on" proposed (I changed the subject also) > > Hmm ... but that isn't right either. It should reflect that the watchdog > is always active / running. I can't fully agree here. "Always active / running" means for me hardware watchdog's attribute. stmp3xxx allows to stop the watchdog and it can be stopped even the feature is set. This case happens on suspend for example. Therefore "always running" / WATCHDOG_ALWAYS_ACTIVE seems to be confusing. > You still have trouble with your line length. Makes it hard for > me (and everyone else) to read your text. Clear. Now I split lines manually. >>> Given that, there are two use states to consider: WDT is always >>> running, >>> and WDT can not be stopped after it was started once. We should cover >>> both cases. >> and third: ability to stop watchdog (if possible) for suspend > > Today that is handled by individual drivers. I have no idea though how > that would or could be handled if the watchdog can not be stopped at all. > Seems to be a contradiction to me. > > Anyway, you'll probably need some code which suspends pinging the > watchdog in suspend. But that assumes that the watchdog can be stopped > after all, at least during suspend. If watchdog can't be stopped it is rather impossible to suspend machine without a reset. Maybe long timeout value has a sense then but it depends on hardware, is complex, and requires RTC wake up event or watchdog pre-timeout interrupt support. >>> The feature should have DT support from the beginning if possible, >>> though it should be added as a separate patch in case there is >>> a hiccup with the DT folks. >> Can you give more details? >> > We'll have to determine bindings which are acceptable for DT. separate patch - next step >> +/* 'keep on' feature */ >> +static void watchdog_keepon_timer_cb(unsigned long data) > > No problem calling this 'ping'. That describes what the function > is doing. 'k' in 'kping' is redundant, though - presumably all > kernel code runs in the kernel ;-). right :) >> + /* call next ping half the timeout value */ >> + mod_timer(wdd->keepon_timer, >> + jiffies + wdd->timeout * (HZ/2)); > > Watch out for coding style (spaces befor and after operators). > Better use something like > jiffies + msecs_to_jiffies(wdd->timeout * 1000) fixed >> +static void watchdog_keepon_start(struct watchdog_device *wdd) >> { >> watchdog_start(wdd); >> + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code >> below: */ >> watchdog_ping(wdd); >> + mod_timer(wdd->keepon_timer, jiffies + HZ/2); > Why only 500ms timeout here ? fixed by calling timer callback function >> struct watchdog_ops; >> @@ -96,9 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if device >> not opened by userland */ >> - struct timer_list *kping_timer; /* kernel ping timer */ >> -#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) >> +#define WDOG_KEEP_ON 5 /* Is 'keep on' feature set? */ >> + struct timer_list *keepon_timer;/* 'keep on' timer */ >> +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) > > Move this define out of the struct, please. done >> +/* 'keep on' feature */ >> +static void watchdog_keepon_timer_cb(unsigned long data) >> +{ >> + struct watchdog_device *wdd = (struct watchdog_device *)data; >> + watchdog_ping(wdd); > > I don't think this works. It bails out if the watchdog is not > active, and active means opened from user space. Correct me > if I am wrong. It works (tested) because watchdog is activated in watchdog_keepon_start() >> + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { >> + if (!try_module_get(watchdog->ops->owner)) >> + return -ENODEV; >> + watchdog->keepon_timer = >> + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); >> + if (!watchdog->keepon_timer) >> + return -ENOMEM; > This really suggests it should be static, to avoid such complexity > and potential errors. Also, you leak at least the module reference > on errors (here and later). changed above >> + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, >> + (unsigned long)watchdog); >> + watchdog_keepon_start(watchdog); > > By setting the timer here prior to completing registration you risk > leaking > the timer on error. Also, there is strictly speaking no guarantee that > the > timer doesn't fire before registration is complete, so this results > in a race condition. Is there race condition really issue? I wanted to start the timer before userspace access is possible. I rather see not handled error path for the ping timer. I fixed it but I guess it requires more common code. Below the fixed patch. Janusz Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..3546f03 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { .ops = &stmp3xxx_wdt_ops, .min_timeout = 1, .max_timeout = STMP3XXX_MAX_TIMEOUT, - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, }; static int stmp3xxx_wdt_probe(struct platform_device *pdev) diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..51a65f6 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,7 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/jiffies.h> /* for ping timer */ #include "watchdog_core.h" @@ -277,6 +278,27 @@ out_ioctl: return err; } +/* 'keep on' feature */ +static void watchdog_ping_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + watchdog_ping(wdd); + /* call next ping half the timeout value */ + mod_timer(&wdd->ping_timer, + jiffies + msecs_to_jiffies(wdd->timeout * 500)); +} + +static void watchdog_keepon_start(struct watchdog_device *wdd) +{ + watchdog_start(wdd); + watchdog_ping_timer_cb((unsigned long)wdd); +} + +static void watchdog_keepon_stop(struct watchdog_device *wdd) +{ + del_timer_sync(&wdd->ping_timer); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +452,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KEEP_ON, &wdd->status)) + watchdog_keepon_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +497,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { + watchdog_keepon_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +554,14 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + setup_timer(&watchdog->ping_timer, watchdog_ping_timer_cb, + (unsigned long)watchdog); + watchdog_keepon_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -535,6 +573,11 @@ int watchdog_dev_register(struct watchdog_device *watchdog) pr_err("%s: a legacy watchdog module is probably present.\n", watchdog->info->identity); old_wdd = NULL; + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + watchdog_keepon_stop(watchdog); + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return err; } } @@ -553,6 +596,11 @@ int watchdog_dev_register(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + watchdog_keepon_stop(watchdog); + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } } return err; } @@ -575,6 +623,12 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + watchdog_keepon_stop(watchdog); + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..650e0d5 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for ping timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,8 @@ 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_KEEP_ON 5 /* Is 'keep on' feature set? */ + struct timer_list ping_timer; /* timer to keep on hardware ping */ }; #ifdef CONFIG_WATCHDOG_NOWAYOUT @@ -104,6 +107,8 @@ struct watchdog_device { #define WATCHDOG_NOWAYOUT 0 #define WATCHDOG_NOWAYOUT_INIT_STATUS 0 #endif +/* other proposal: WATCHDOG_ALWAYS_ACTIVE */ +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) /* Use the following function to check whether or not the watchdog is active */ static inline bool watchdog_active(struct watchdog_device *wdd) [-- Attachment #2: watchdog_dev.patch --] [-- Type: text/plain, Size: 5341 bytes --] Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c index b4d6b34..3546f03 100644 --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { .ops = &stmp3xxx_wdt_ops, .min_timeout = 1, .max_timeout = STMP3XXX_MAX_TIMEOUT, - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, }; static int stmp3xxx_wdt_probe(struct platform_device *pdev) diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 6aaefba..51a65f6 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -41,6 +41,7 @@ #include <linux/miscdevice.h> /* For handling misc devices */ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ +#include <linux/jiffies.h> /* for ping timer */ #include "watchdog_core.h" @@ -277,6 +278,27 @@ out_ioctl: return err; } +/* 'keep on' feature */ +static void watchdog_ping_timer_cb(unsigned long data) +{ + struct watchdog_device *wdd = (struct watchdog_device *)data; + watchdog_ping(wdd); + /* call next ping half the timeout value */ + mod_timer(&wdd->ping_timer, + jiffies + msecs_to_jiffies(wdd->timeout * 500)); +} + +static void watchdog_keepon_start(struct watchdog_device *wdd) +{ + watchdog_start(wdd); + watchdog_ping_timer_cb((unsigned long)wdd); +} + +static void watchdog_keepon_stop(struct watchdog_device *wdd) +{ + del_timer_sync(&wdd->ping_timer); +} + /* * watchdog_write: writes to the watchdog. * @file: file from VFS @@ -430,6 +452,9 @@ static int watchdog_open(struct inode *inode, struct file *file) if (!try_module_get(wdd->ops->owner)) goto out; + if (test_bit(WDOG_KEEP_ON, &wdd->status)) + watchdog_keepon_stop(wdd); + err = watchdog_start(wdd); if (err < 0) goto out_mod; @@ -472,8 +497,13 @@ static int watchdog_release(struct inode *inode, struct file *file) if (!test_bit(WDOG_ACTIVE, &wdd->status)) err = 0; else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || - !(wdd->info->options & WDIOF_MAGICCLOSE)) - err = watchdog_stop(wdd); + !(wdd->info->options & WDIOF_MAGICCLOSE)) { + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { + watchdog_keepon_start(wdd); + err = 0; + } else + err = watchdog_stop(wdd); + } /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { @@ -524,6 +554,14 @@ int watchdog_dev_register(struct watchdog_device *watchdog) { int err, devno; + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + if (!try_module_get(watchdog->ops->owner)) + return -ENODEV; + setup_timer(&watchdog->ping_timer, watchdog_ping_timer_cb, + (unsigned long)watchdog); + watchdog_keepon_start(watchdog); + } + if (watchdog->id == 0) { old_wdd = watchdog; watchdog_miscdev.parent = watchdog->parent; @@ -535,6 +573,11 @@ int watchdog_dev_register(struct watchdog_device *watchdog) pr_err("%s: a legacy watchdog module is probably present.\n", watchdog->info->identity); old_wdd = NULL; + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + watchdog_keepon_stop(watchdog); + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return err; } } @@ -553,6 +596,11 @@ int watchdog_dev_register(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + watchdog_keepon_stop(watchdog); + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } } return err; } @@ -575,6 +623,12 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog) misc_deregister(&watchdog_miscdev); old_wdd = NULL; } + + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { + watchdog_keepon_stop(watchdog); + watchdog_stop(watchdog); + module_put(watchdog->ops->owner); + } return 0; } diff --git a/linux-3.14.17/include/linux/watchdog.h b/linux-3.14.17/include/linux/watchdog.h index 2a3038e..650e0d5 100644 --- a/linux-3.14.17/include/linux/watchdog.h +++ b/linux-3.14.17/include/linux/watchdog.h @@ -12,6 +12,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/cdev.h> +#include <linux/timer.h> /* for ping timer */ #include <uapi/linux/watchdog.h> struct watchdog_ops; @@ -95,6 +96,8 @@ 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_KEEP_ON 5 /* Is 'keep on' feature set? */ + struct timer_list ping_timer; /* timer to keep on hardware ping */ }; #ifdef CONFIG_WATCHDOG_NOWAYOUT @@ -104,6 +107,8 @@ struct watchdog_device { #define WATCHDOG_NOWAYOUT 0 #define WATCHDOG_NOWAYOUT_INIT_STATUS 0 #endif +/* other proposal: WATCHDOG_ALWAYS_ACTIVE */ +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) /* Use the following function to check whether or not the watchdog is active */ static inline bool watchdog_active(struct watchdog_device *wdd) ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-08 12:14 ` Janusz Użycki @ 2014-09-10 17:24 ` Janusz Użycki 2014-09-11 10:47 ` Janusz Użycki 0 siblings, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-10 17:24 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog; +Cc: Wim Van Sebroeck To store relation to the thread: http://www.spinics.net/lists/linux-watchdog/msg05013.html I propose to add to the patch rootfs's boot time parameter. If this time is not zero after the time timer stops pinging. The parameter is a time when userland is ready to maintain a watchdog alone if it supports wachdog at all. The parameter seems to be common for all watchdog drivers and could be defined by kernel's cmdline because the time concerns a specific machine, board/kernel and rootfs. Some userlands open /dev/watchdog only in critical moments and after they use magic close. Therefore the patch continues watchdog's pinging in kernel's space. For the case boot time parameters should be zero. It is just idea. best regards Janusz W dniu 2014-09-08 14:14, Janusz Użycki pisze: > W dniu 2014-09-08 05:16, Guenter Roeck pisze: >>>>> * Shoud dynamic or static timer_list be used (small structure...)? >>> dynamic or static timer? >> I am fine with dynamic, though I would probably make it static. >> It is not as if there are dozens or hundreds of watchdogs >> in the system where it would make much of a difference. >> The code itself is already much larger than the size of >> the data structure. > ok, switched to static > >>> Can kernel suspend a started (stoppable) watchdog? It dissapeared in >>> 3.x. Now userland reaction seems to be required. >> Really ? I see a number of watchdog drivers implementing it. > I thought it was discarded and PM in other watchdog drivers is deprecated > when I didn't find it for not so old stmp3xxx_rtc_wdt.c. > For suspend/resume drivers call own stop/start functions and use > watchdog_active() - good > but it could be likely unified. > Some drivers duplicate active flag, eg. omap_wdt.c: omap_wdt_users. > >>> I also attached the patch file - I don't trust email client in text >>> formatting (tabs) >> You'll have to sort that out. Try using git send-email, for example. > I found Thunderbird is accepted but I'm not sure. > >>>> I would not call this "kernel ping". We'll need to find a better >>>> name. Proposals welcome. Something indicating the status, ie something >>>> indicating that the wdt is always running and can not be stopped. >>> "keep on" proposed (I changed the subject also) >> >> Hmm ... but that isn't right either. It should reflect that the watchdog >> is always active / running. > I can't fully agree here. > "Always active / running" means for me hardware watchdog's attribute. > stmp3xxx allows to stop the watchdog and it can be stopped even the > feature is set. > This case happens on suspend for example. > Therefore "always running" / WATCHDOG_ALWAYS_ACTIVE seems to be > confusing. > >> You still have trouble with your line length. Makes it hard for >> me (and everyone else) to read your text. > Clear. Now I split lines manually. > >>>> Given that, there are two use states to consider: WDT is always >>>> running, >>>> and WDT can not be stopped after it was started once. We should cover >>>> both cases. >>> and third: ability to stop watchdog (if possible) for suspend >> >> Today that is handled by individual drivers. I have no idea though how >> that would or could be handled if the watchdog can not be stopped at >> all. >> Seems to be a contradiction to me. >> >> Anyway, you'll probably need some code which suspends pinging the >> watchdog in suspend. But that assumes that the watchdog can be stopped >> after all, at least during suspend. > If watchdog can't be stopped it is rather impossible to suspend > machine without a reset. > Maybe long timeout value has a sense then but it depends on hardware, > is complex, > and requires RTC wake up event or watchdog pre-timeout interrupt support. > >>>> The feature should have DT support from the beginning if possible, >>>> though it should be added as a separate patch in case there is >>>> a hiccup with the DT folks. >>> Can you give more details? >>> >> We'll have to determine bindings which are acceptable for DT. > separate patch - next step > >>> +/* 'keep on' feature */ >>> +static void watchdog_keepon_timer_cb(unsigned long data) >> >> No problem calling this 'ping'. That describes what the function >> is doing. 'k' in 'kping' is redundant, though - presumably all >> kernel code runs in the kernel ;-). > right :) > >>> + /* call next ping half the timeout value */ >>> + mod_timer(wdd->keepon_timer, >>> + jiffies + wdd->timeout * (HZ/2)); >> >> Watch out for coding style (spaces befor and after operators). >> Better use something like >> jiffies + msecs_to_jiffies(wdd->timeout * 1000) > fixed > >>> +static void watchdog_keepon_start(struct watchdog_device *wdd) >>> { >>> watchdog_start(wdd); >>> + /* watchdog_keepon_timer_cb((unsigned long)wdd); or the code >>> below: */ >>> watchdog_ping(wdd); >>> + mod_timer(wdd->keepon_timer, jiffies + HZ/2); >> Why only 500ms timeout here ? > fixed by calling timer callback function > >>> struct watchdog_ops; >>> @@ -96,9 +96,9 @@ 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_KERNEL_PING 5 /* Ping is done by kernel if >>> device not opened by userland */ >>> - struct timer_list *kping_timer; /* kernel ping timer */ >>> -#define WATCHDOG_KERNEL_PING (1 << WDOG_KERNEL_PING) >>> +#define WDOG_KEEP_ON 5 /* Is 'keep on' feature set? */ >>> + struct timer_list *keepon_timer;/* 'keep on' timer */ >>> +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) >> >> Move this define out of the struct, please. > done > >>> +/* 'keep on' feature */ >>> +static void watchdog_keepon_timer_cb(unsigned long data) >>> +{ >>> + struct watchdog_device *wdd = (struct watchdog_device *)data; >>> + watchdog_ping(wdd); >> >> I don't think this works. It bails out if the watchdog is not >> active, and active means opened from user space. Correct me >> if I am wrong. > It works (tested) because watchdog is activated in > watchdog_keepon_start() > >>> + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { >>> + if (!try_module_get(watchdog->ops->owner)) >>> + return -ENODEV; >>> + watchdog->keepon_timer = >>> + kzalloc(sizeof(*watchdog->keepon_timer), GFP_KERNEL); >>> + if (!watchdog->keepon_timer) >>> + return -ENOMEM; >> This really suggests it should be static, to avoid such complexity >> and potential errors. Also, you leak at least the module reference >> on errors (here and later). > changed above > >>> + setup_timer(watchdog->keepon_timer, watchdog_keepon_timer_cb, >>> + (unsigned long)watchdog); >>> + watchdog_keepon_start(watchdog); >> >> By setting the timer here prior to completing registration you risk >> leaking >> the timer on error. Also, there is strictly speaking no guarantee >> that the >> timer doesn't fire before registration is complete, so this results >> in a race condition. > Is there race condition really issue? > I wanted to start the timer before userspace access is possible. > I rather see not handled error path for the ping timer. > I fixed it but I guess it requires more common code. > > Below the fixed patch. > Janusz > > Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> > > diff --git a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > index b4d6b34..3546f03 100644 > --- a/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > +++ b/linux-3.14.17/drivers/watchdog/stmp3xxx_rtc_wdt.c > @@ -67,7 +67,7 @@ static struct watchdog_device stmp3xxx_wdd = { > .ops = &stmp3xxx_wdt_ops, > .min_timeout = 1, > .max_timeout = STMP3XXX_MAX_TIMEOUT, > - .status = WATCHDOG_NOWAYOUT_INIT_STATUS, > + .status = WATCHDOG_NOWAYOUT_INIT_STATUS | WATCHDOG_KEEP_ON, > }; > > static int stmp3xxx_wdt_probe(struct platform_device *pdev) > diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c > b/linux-3.14.17/drivers/watchdog/watchdog_dev.c > index 6aaefba..51a65f6 100644 > --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c > +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c > @@ -41,6 +41,7 @@ > #include <linux/miscdevice.h> /* For handling misc devices */ > #include <linux/init.h> /* For __init/__exit/... */ > #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ > +#include <linux/jiffies.h> /* for ping timer */ > > #include "watchdog_core.h" > > @@ -277,6 +278,27 @@ out_ioctl: > return err; > } > > +/* 'keep on' feature */ > +static void watchdog_ping_timer_cb(unsigned long data) > +{ > + struct watchdog_device *wdd = (struct watchdog_device *)data; > + watchdog_ping(wdd); > + /* call next ping half the timeout value */ > + mod_timer(&wdd->ping_timer, > + jiffies + msecs_to_jiffies(wdd->timeout * 500)); > +} > + > +static void watchdog_keepon_start(struct watchdog_device *wdd) > +{ > + watchdog_start(wdd); > + watchdog_ping_timer_cb((unsigned long)wdd); > +} > + > +static void watchdog_keepon_stop(struct watchdog_device *wdd) > +{ > + del_timer_sync(&wdd->ping_timer); > +} > + > /* > * watchdog_write: writes to the watchdog. > * @file: file from VFS > @@ -430,6 +452,9 @@ static int watchdog_open(struct inode *inode, > struct file *file) > if (!try_module_get(wdd->ops->owner)) > goto out; > > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) > + watchdog_keepon_stop(wdd); > + > err = watchdog_start(wdd); > if (err < 0) > goto out_mod; > @@ -472,8 +497,13 @@ static int watchdog_release(struct inode *inode, > struct file *file) > if (!test_bit(WDOG_ACTIVE, &wdd->status)) > err = 0; > else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > - !(wdd->info->options & WDIOF_MAGICCLOSE)) > - err = watchdog_stop(wdd); > + !(wdd->info->options & WDIOF_MAGICCLOSE)) { > + if (test_bit(WDOG_KEEP_ON, &wdd->status)) { > + watchdog_keepon_start(wdd); > + err = 0; > + } else > + err = watchdog_stop(wdd); > + } > > /* If the watchdog was not stopped, send a keepalive ping */ > if (err < 0) { > @@ -524,6 +554,14 @@ int watchdog_dev_register(struct watchdog_device > *watchdog) > { > int err, devno; > > + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { > + if (!try_module_get(watchdog->ops->owner)) > + return -ENODEV; > + setup_timer(&watchdog->ping_timer, watchdog_ping_timer_cb, > + (unsigned long)watchdog); > + watchdog_keepon_start(watchdog); > + } > + > if (watchdog->id == 0) { > old_wdd = watchdog; > watchdog_miscdev.parent = watchdog->parent; > @@ -535,6 +573,11 @@ int watchdog_dev_register(struct watchdog_device > *watchdog) > pr_err("%s: a legacy watchdog module is probably > present.\n", > watchdog->info->identity); > old_wdd = NULL; > + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { > + watchdog_keepon_stop(watchdog); > + watchdog_stop(watchdog); > + module_put(watchdog->ops->owner); > + } > return err; > } > } > @@ -553,6 +596,11 @@ int watchdog_dev_register(struct watchdog_device > *watchdog) > misc_deregister(&watchdog_miscdev); > old_wdd = NULL; > } > + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { > + watchdog_keepon_stop(watchdog); > + watchdog_stop(watchdog); > + module_put(watchdog->ops->owner); > + } > } > return err; > } > @@ -575,6 +623,12 @@ int watchdog_dev_unregister(struct > watchdog_device *watchdog) > misc_deregister(&watchdog_miscdev); > old_wdd = NULL; > } > + > + if (test_bit(WDOG_KEEP_ON, &watchdog->status)) { > + watchdog_keepon_stop(watchdog); > + watchdog_stop(watchdog); > + module_put(watchdog->ops->owner); > + } > return 0; > } > > diff --git a/linux-3.14.17/include/linux/watchdog.h > b/linux-3.14.17/include/linux/watchdog.h > index 2a3038e..650e0d5 100644 > --- a/linux-3.14.17/include/linux/watchdog.h > +++ b/linux-3.14.17/include/linux/watchdog.h > @@ -12,6 +12,7 @@ > #include <linux/bitops.h> > #include <linux/device.h> > #include <linux/cdev.h> > +#include <linux/timer.h> /* for ping timer */ > #include <uapi/linux/watchdog.h> > > struct watchdog_ops; > @@ -95,6 +96,8 @@ 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_KEEP_ON 5 /* Is 'keep on' feature set? */ > + struct timer_list ping_timer; /* timer to keep on hardware > ping */ > }; > > #ifdef CONFIG_WATCHDOG_NOWAYOUT > @@ -104,6 +107,8 @@ struct watchdog_device { > #define WATCHDOG_NOWAYOUT 0 > #define WATCHDOG_NOWAYOUT_INIT_STATUS 0 > #endif > +/* other proposal: WATCHDOG_ALWAYS_ACTIVE */ > +#define WATCHDOG_KEEP_ON (1 << WDOG_KEEP_ON) > > /* Use the following function to check whether or not the watchdog is > active */ > static inline bool watchdog_active(struct watchdog_device *wdd) > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-10 17:24 ` Janusz Użycki @ 2014-09-11 10:47 ` Janusz Użycki 0 siblings, 0 replies; 23+ messages in thread From: Janusz Użycki @ 2014-09-11 10:47 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog; +Cc: Wim Van Sebroeck W dniu 2014-09-10 19:24, Janusz Użycki pisze: > To store relation to the thread: > http://www.spinics.net/lists/linux-watchdog/msg05013.html > > I propose to add to the patch rootfs's boot time parameter. > If this time is not zero after the time timer stops pinging. > The parameter is a time when userland is ready to maintain > a watchdog alone if it supports wachdog at all. > The parameter seems to be common for all watchdog drivers > and could be defined by kernel's cmdline > because the time concerns a specific machine, board/kernel > and rootfs. > Some userlands open /dev/watchdog only in critical moments > and after they use magic close. Therefore the patch continues > watchdog's pinging in kernel's space. For the case boot time > parameters should be zero. > It is just idea. Below is implementation proposal for the idea. best regards Janusz Subject: [PATCH] watchdog: boottime protection feature (requires 'keep on') Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> --- linux-3.14.17/drivers/watchdog/watchdog_core.c | 4 ++++ linux-3.14.17/drivers/watchdog/watchdog_dev.c | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/linux-3.14.17/drivers/watchdog/watchdog_core.c b/linux-3.14.17/drivers/watchdog/watchdog_core.c index cec9b55..13f3d3a 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_core.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_core.c @@ -43,6 +43,10 @@ static DEFINE_IDA(watchdog_ida); static struct class *watchdog_class; +unsigned int boottime = 0; +module_param(boottime, uint, 0); +MODULE_PARM_DESC(boottime, "After the boot time active watchdog(s) have to be maintained by userland. 0 disables the protection."); + static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) { /* diff --git a/linux-3.14.17/drivers/watchdog/watchdog_dev.c b/linux-3.14.17/drivers/watchdog/watchdog_dev.c index 51a65f6..4b975da 100644 --- a/linux-3.14.17/drivers/watchdog/watchdog_dev.c +++ b/linux-3.14.17/drivers/watchdog/watchdog_dev.c @@ -42,6 +42,7 @@ #include <linux/init.h> /* For __init/__exit/... */ #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ #include <linux/jiffies.h> /* for ping timer */ +#include <linux/time.h> /* for uptime */ #include "watchdog_core.h" @@ -282,6 +283,16 @@ out_ioctl: static void watchdog_ping_timer_cb(unsigned long data) { struct watchdog_device *wdd = (struct watchdog_device *)data; + /* After the boot time active watchdog(s) have to be maintained by userland. + * The protection works only if boot time is non-zero + * and keep on feature is enabled */ + extern unsigned int boottime; + if (boottime) { + struct timespec uptime; + get_monotonic_boottime(&uptime); + if (uptime.tv_sec > boottime) + return; + } watchdog_ping(wdd); /* call next ping half the timeout value */ mod_timer(&wdd->ping_timer, -- 1.7.11.3 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2014-09-08 3:16 ` Guenter Roeck 2014-09-08 12:14 ` Janusz Użycki @ 2014-09-17 11:09 ` Janusz Użycki 2014-09-18 11:07 ` watchdog's pm support preffered implementation Janusz Użycki 1 sibling, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-17 11:09 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog; +Cc: Wim Van Sebroeck Hi Guenter. W dniu 2014-09-08 05:16, Guenter Roeck pisze: > >>>> * I also added wdd->ops->ref/unref calls but I'm afraid that even >>>> original code is buggy in watchdog_dev.c. Is any driver that uses >>>> the pointers? In my opinion watchdog_open() should call >>>> wdd->ops->ref() before watchdog_start() and watchdog_release() >>>> should call wdd->ops->unref() before module_put(). Otherwise fault >>>> is possible if watchdog module is unloaded. >>>> * I noticed that current watchdog core does not support >>>> suspend/resume case. If you want to use suspend without the patch >>>> you need to close a watchdog in userspace using the magic character >>>> before suspend command. With the patch you must to use >>>> WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is >>>> there any other method to suspend with watchdog? >> Can kernel suspend a started (stoppable) watchdog? It dissapeared in >> 3.x. Now userland reaction seems to be required. >> > Really ? I see a number of watchdog drivers implementing it. > Three things about always active/running/enabled watchdogs: - stop returns an error - pm/suspend/resume are not supported - if watchdog is running/active keep-on like feature should be enabled - on register watchdog specific driver should check if watchdog was enabled before Always active/running/enabled is related to watchdog attribute and seems have direct influence on specific driver code. Is there "never stop" software option needed for stoppable watchdogs? Abore are just notes. Now I'm working under keep-on and stmp3xxx driver only. best regards Janusz ^ permalink raw reply [flat|nested] 23+ messages in thread
* watchdog's pm support preffered implementation 2014-09-17 11:09 ` Janusz Użycki @ 2014-09-18 11:07 ` Janusz Użycki 2014-09-18 21:40 ` Janusz Użycki 0 siblings, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-18 11:07 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog Hi, I work under suspend/resume PM support for stmp3xxx driver. I checked other drivers: grep suspend watchdog/* | grep static watchdog/at32ap700x_wdt.c:static int at32_wdt_suspend(struct platform_device *pdev, pm_message_t message) watchdog/at91rm9200_wdt.c:static int at91wdt_suspend(struct platform_device *pdev, pm_message_t message) watchdog/bfin_wdt.c:static int bfin_wdt_suspend(struct platform_device *pdev, pm_message_t state) watchdog/coh901327_wdt.c:static int coh901327_suspend(struct platform_device *pdev, pm_message_t state) watchdog/dw_wdt.c:static int dw_wdt_suspend(struct device *dev) watchdog/dw_wdt.c:static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume); watchdog/kempld_wdt.c:static int kempld_wdt_suspend(struct platform_device *pdev, watchdog/ks8695_wdt.c:static int ks8695wdt_suspend(struct platform_device *pdev, pm_message_t message) watchdog/omap_wdt.c:static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state) watchdog/s3c2410_wdt.c:static int s3c2410wdt_suspend(struct device *dev) watchdog/s3c2410_wdt.c:static SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops, s3c2410wdt_suspend, watchdog/sirfsoc_wdt.c:static int sirfsoc_wdt_suspend(struct device *dev) watchdog/sp805_wdt.c:static int __maybe_unused sp805_wdt_suspend(struct device *dev) watchdog/sp805_wdt.c:static SIMPLE_DEV_PM_OPS(sp805_wdt_dev_pm_ops, sp805_wdt_suspend, watchdog/stmp3xxx_rtc_wdt.c:static int stmp3xxx_wdt_suspend(struct platform_device *pdev, watchdog/twl4030_wdt.c:static int twl4030_wdt_suspend(struct platform_device *pdev, pm_message_t state) watchdog/ux500_wdt.c:static int ux500_wdt_suspend(struct platform_device *pdev, watchdog/xen_wdt.c:static int xen_wdt_suspend(struct platform_device *dev, pm_message_t state) Most of them use suspend() with struct platform_device and CONFIG_PM. Only sp805, sirfsoc, s3c2410, dw drivers use suspend() with struct device and SIMPLE_DEV_PM_OPS. Which implementation is preffered today? best regards Janusz P.S. Patch on the moment: Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> --- drivers/watchdog/stmp3xxx_rtc_wdt.c | 41 +++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c index 3546f03..12060ab 100644 --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -74,6 +74,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev) { int ret; + platform_set_drvdata(pdev, &stmp3xxx_wdd); watchdog_set_drvdata(&stmp3xxx_wdd, &pdev->dev); stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); @@ -95,12 +96,52 @@ static int stmp3xxx_wdt_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM +/* There is no conflict with rtc/rtc-stmp3xxx.c parent + * because modified registers in PM functions are different */ +static int stmp3xxx_wdt_suspend(struct platform_device *pdev, + pm_message_t state) +{ + struct watchdog_device *wdd = platform_get_drvdata(pdev); + + /* Is keep-on/ping timer suspended before? + * or additional driver-specific flag must be added + * to block watchdog ping in the timer? + * or disable WATCHDOG_KEEP_ON before wdt_stop + * and restore it in resume? */ + if (watchdog_active(wdd)) { + dev_info(wdd->dev, "%s: wdt was active\n", __func__); + return wdt_stop(wdd); + } + /* should we use pm_runtime like omap_wdt.c does? */ + + return 0; +} + +static int stmp3xxx_wdt_resume(struct platform_device *pdev) +{ + struct watchdog_device *wdd = platform_get_drvdata(pdev); + + if (watchdog_active(wdd)) { + dev_info(wdd->dev, "%s: wdt was active\n", __func__); + return wdt_start(wdd); + } + + return 0; +} +#else +#define stmp3xxx_wdt_suspend NULL +#define stmp3xxx_wdt_resume NULL +#endif + static struct platform_driver stmp3xxx_wdt_driver = { .driver = { .name = "stmp3xxx_rtc_wdt", }, .probe = stmp3xxx_wdt_probe, .remove = stmp3xxx_wdt_remove, + .suspend = stmp3xxx_wdt_suspend, + .resume = stmp3xxx_wdt_resume, }; module_platform_driver(stmp3xxx_wdt_driver); ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: watchdog's pm support preffered implementation 2014-09-18 11:07 ` watchdog's pm support preffered implementation Janusz Użycki @ 2014-09-18 21:40 ` Janusz Użycki 2014-09-18 22:02 ` Janusz Użycki 0 siblings, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-18 21:40 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog, Wim Van Sebroeck Hi again, This is the second implementation. Which do you prefer? Subject: [PATCH] stmp3xxx_rtc_wdt: Add suspend/resume PM support Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> --- drivers/watchdog/stmp3xxx_rtc_wdt.c | 39 +++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c index 3546f03..1946277 100644 --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -95,9 +95,48 @@ static int stmp3xxx_wdt_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM_SLEEP +/* There is no conflict with rtc/rtc-stmp3xxx.c parent + * because modified registers in PM functions are different */ +static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev) +{ + struct watchdog_device *wdd = dev_get_drvdata(dev); + + /* Is keep-on/ping timer suspended before? + * or additional driver-specific flag must be added + * to block watchdog ping in the timer? + * or disable WATCHDOG_KEEP_ON before wdt_stop + * and restore it in resume? */ + if (watchdog_active(wdd)) { + dev_info(wdd->dev, "%s: wdt was active\n", __func__); + return wdt_stop(wdd); + } + /* should we use pm_runtime like omap_wdt.c does? */ + + return 0; +} + +static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev) +{ + struct watchdog_device *wdd = dev_get_drvdata(dev); + + if (watchdog_active(wdd)) { + dev_info(wdd->dev, "%s: wdt was active\n", __func__); + return wdt_start(wdd); + } + + return 0; +} +#endif /* CONFIG_PM_SLEEP */ + +static SIMPLE_DEV_PM_OPS(stmp3xxx_wdt_pm_ops, + stmp3xxx_wdt_suspend, stmp3xxx_wdt_resume); + static struct platform_driver stmp3xxx_wdt_driver = { .driver = { .name = "stmp3xxx_rtc_wdt", + .owner = THIS_MODULE, + .pm = &stmp3xxx_wdt_pm_ops, }, .probe = stmp3xxx_wdt_probe, .remove = stmp3xxx_wdt_remove, -- 1.7.11.3 W dniu 2014-09-18 13:07, Janusz Użycki pisze: > > I work under suspend/resume PM support for stmp3xxx driver. > > Most of them use suspend() with struct platform_device and CONFIG_PM. > Only sp805, sirfsoc, s3c2410, dw drivers use suspend() with struct device > and SIMPLE_DEV_PM_OPS. > Which implementation is preffered today? > > best regards > Janusz > > P.S. Patch on the moment: > Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> > --- > drivers/watchdog/stmp3xxx_rtc_wdt.c | 41 +++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c > b/drivers/watchdog/stmp3xxx_rtc_wdt.c > index 3546f03..12060ab 100644 > --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c > +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c > @@ -74,6 +74,7 @@ static int stmp3xxx_wdt_probe(struct platform_device > *pdev) > { > int ret; > > + platform_set_drvdata(pdev, &stmp3xxx_wdd); > watchdog_set_drvdata(&stmp3xxx_wdd, &pdev->dev); > > stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, > STMP3XXX_MAX_TIMEOUT); > @@ -95,12 +96,52 @@ static int stmp3xxx_wdt_remove(struct > platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM > +/* There is no conflict with rtc/rtc-stmp3xxx.c parent > + * because modified registers in PM functions are different */ > +static int stmp3xxx_wdt_suspend(struct platform_device *pdev, > + pm_message_t state) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + > + /* Is keep-on/ping timer suspended before? > + * or additional driver-specific flag must be added > + * to block watchdog ping in the timer? > + * or disable WATCHDOG_KEEP_ON before wdt_stop > + * and restore it in resume? */ > + if (watchdog_active(wdd)) { > + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > + return wdt_stop(wdd); > + } > + /* should we use pm_runtime like omap_wdt.c does? */ > + > + return 0; > +} > + > +static int stmp3xxx_wdt_resume(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd = platform_get_drvdata(pdev); > + > + if (watchdog_active(wdd)) { > + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > + return wdt_start(wdd); > + } > + > + return 0; > +} > +#else > +#define stmp3xxx_wdt_suspend NULL > +#define stmp3xxx_wdt_resume NULL > +#endif > + > static struct platform_driver stmp3xxx_wdt_driver = { > .driver = { > .name = "stmp3xxx_rtc_wdt", > }, > .probe = stmp3xxx_wdt_probe, > .remove = stmp3xxx_wdt_remove, > + .suspend = stmp3xxx_wdt_suspend, > + .resume = stmp3xxx_wdt_resume, > }; > module_platform_driver(stmp3xxx_wdt_driver); > ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: watchdog's pm support preffered implementation 2014-09-18 21:40 ` Janusz Użycki @ 2014-09-18 22:02 ` Janusz Użycki 2014-09-19 3:11 ` Guenter Roeck 0 siblings, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-18 22:02 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog, Wim Van Sebroeck Small fix below in the second implementation. best regards Janusz W dniu 2014-09-18 23:40, Janusz Użycki pisze: > Hi again, > > This is the second implementation. Which do you prefer? > > Subject: [PATCH] stmp3xxx_rtc_wdt: Add suspend/resume PM support > > > Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> > --- > drivers/watchdog/stmp3xxx_rtc_wdt.c | 39 +++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c > b/drivers/watchdog/stmp3xxx_rtc_wdt.c > index 3546f03..1946277 100644 > --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c > +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c > @@ -95,9 +95,48 @@ static int stmp3xxx_wdt_remove(struct > platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > +/* There is no conflict with rtc/rtc-stmp3xxx.c parent > + * because modified registers in PM functions are different */ > +static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev) > +{ > + struct watchdog_device *wdd = dev_get_drvdata(dev); drvdata is NULL, too fast, + struct watchdog_device *wdd = &stmp3xxx_wdd; > + > + /* Is keep-on/ping timer suspended before? > + * or additional driver-specific flag must be added > + * to block watchdog ping in the timer? > + * or disable WATCHDOG_KEEP_ON before wdt_stop > + * and restore it in resume? */ > + if (watchdog_active(wdd)) { > + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > + return wdt_stop(wdd); > + } > + /* should we use pm_runtime like omap_wdt.c does? */ > + > + return 0; > +} > + > +static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev) > +{ > + struct watchdog_device *wdd = dev_get_drvdata(dev); + struct watchdog_device *wdd = &stmp3xxx_wdd; > + > + if (watchdog_active(wdd)) { > + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > + return wdt_start(wdd); > + } > + > + return 0; > +} > +#endif /* CONFIG_PM_SLEEP */ > + > +static SIMPLE_DEV_PM_OPS(stmp3xxx_wdt_pm_ops, > + stmp3xxx_wdt_suspend, stmp3xxx_wdt_resume); > + > static struct platform_driver stmp3xxx_wdt_driver = { > .driver = { > .name = "stmp3xxx_rtc_wdt", > + .owner = THIS_MODULE, > + .pm = &stmp3xxx_wdt_pm_ops, > }, > .probe = stmp3xxx_wdt_probe, > .remove = stmp3xxx_wdt_remove, > -- > 1.7.11.3 > > > W dniu 2014-09-18 13:07, Janusz Użycki pisze: >> >> I work under suspend/resume PM support for stmp3xxx driver. >> >> Most of them use suspend() with struct platform_device and CONFIG_PM. >> Only sp805, sirfsoc, s3c2410, dw drivers use suspend() with struct >> device >> and SIMPLE_DEV_PM_OPS. >> Which implementation is preffered today? >> >> best regards >> Janusz >> >> P.S. Patch on the moment: >> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> >> --- >> drivers/watchdog/stmp3xxx_rtc_wdt.c | 41 +++++++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> >> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c >> b/drivers/watchdog/stmp3xxx_rtc_wdt.c >> index 3546f03..12060ab 100644 >> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c >> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c >> @@ -74,6 +74,7 @@ static int stmp3xxx_wdt_probe(struct >> platform_device *pdev) >> { >> int ret; >> >> + platform_set_drvdata(pdev, &stmp3xxx_wdd); >> watchdog_set_drvdata(&stmp3xxx_wdd, &pdev->dev); >> >> stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, >> STMP3XXX_MAX_TIMEOUT); >> @@ -95,12 +96,52 @@ static int stmp3xxx_wdt_remove(struct >> platform_device *pdev) >> return 0; >> } >> >> +#ifdef CONFIG_PM >> +/* There is no conflict with rtc/rtc-stmp3xxx.c parent >> + * because modified registers in PM functions are different */ >> +static int stmp3xxx_wdt_suspend(struct platform_device *pdev, >> + pm_message_t state) >> +{ >> + struct watchdog_device *wdd = platform_get_drvdata(pdev); >> + >> + /* Is keep-on/ping timer suspended before? >> + * or additional driver-specific flag must be added >> + * to block watchdog ping in the timer? >> + * or disable WATCHDOG_KEEP_ON before wdt_stop >> + * and restore it in resume? */ >> + if (watchdog_active(wdd)) { >> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); >> + return wdt_stop(wdd); >> + } >> + /* should we use pm_runtime like omap_wdt.c does? */ >> + >> + return 0; >> +} >> + >> +static int stmp3xxx_wdt_resume(struct platform_device *pdev) >> +{ >> + struct watchdog_device *wdd = platform_get_drvdata(pdev); >> + >> + if (watchdog_active(wdd)) { >> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); >> + return wdt_start(wdd); >> + } >> + >> + return 0; >> +} >> +#else >> +#define stmp3xxx_wdt_suspend NULL >> +#define stmp3xxx_wdt_resume NULL >> +#endif >> + >> static struct platform_driver stmp3xxx_wdt_driver = { >> .driver = { >> .name = "stmp3xxx_rtc_wdt", >> }, >> .probe = stmp3xxx_wdt_probe, >> .remove = stmp3xxx_wdt_remove, >> + .suspend = stmp3xxx_wdt_suspend, >> + .resume = stmp3xxx_wdt_resume, >> }; >> module_platform_driver(stmp3xxx_wdt_driver); >> > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog's pm support preffered implementation 2014-09-18 22:02 ` Janusz Użycki @ 2014-09-19 3:11 ` Guenter Roeck 2014-09-19 9:46 ` Janusz Użycki 0 siblings, 1 reply; 23+ messages in thread From: Guenter Roeck @ 2014-09-19 3:11 UTC (permalink / raw) To: Janusz Użycki, linux-watchdog, Wim Van Sebroeck On 09/18/2014 03:02 PM, Janusz Użycki wrote: > Small fix below in the second implementation. > > best regards > Janusz > > W dniu 2014-09-18 23:40, Janusz Użycki pisze: >> Hi again, >> >> This is the second implementation. Which do you prefer? >> This one >> Subject: [PATCH] stmp3xxx_rtc_wdt: Add suspend/resume PM support >> >> >> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> >> --- >> drivers/watchdog/stmp3xxx_rtc_wdt.c | 39 +++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c >> index 3546f03..1946277 100644 >> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c >> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c >> @@ -95,9 +95,48 @@ static int stmp3xxx_wdt_remove(struct platform_device *pdev) >> return 0; >> } >> >> +#ifdef CONFIG_PM_SLEEP >> +/* There is no conflict with rtc/rtc-stmp3xxx.c parent >> + * because modified registers in PM functions are different */ Coding style, and what does this comment mean ? To me it is just confusing. >> +static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev) >> +{ >> + struct watchdog_device *wdd = dev_get_drvdata(dev); > > drvdata is NULL, too fast, > + struct watchdog_device *wdd = &stmp3xxx_wdd; > >> + >> + /* Is keep-on/ping timer suspended before? >> + * or additional driver-specific flag must be added >> + * to block watchdog ping in the timer? >> + * or disable WATCHDOG_KEEP_ON before wdt_stop >> + * and restore it in resume? */ You'll have to answer those questions. >> + if (watchdog_active(wdd)) { >> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); Does this message add any value ? >> + return wdt_stop(wdd); >> + } >> + /* should we use pm_runtime like omap_wdt.c does? */ Isn't that what you do here ? >> + >> + return 0; >> +} >> + >> +static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev) Does the __maybe_unused really apply ? >> +{ >> + struct watchdog_device *wdd = dev_get_drvdata(dev); > + struct watchdog_device *wdd = &stmp3xxx_wdd; One of those lines needs to go. Do you have indentation problems ? Do you use space for indentations, maybe ? > >> + >> + if (watchdog_active(wdd)) { >> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); Does this message add any value ? >> + return wdt_start(wdd); >> + } >> + >> + return 0; >> +} >> +#endif /* CONFIG_PM_SLEEP */ >> + >> +static SIMPLE_DEV_PM_OPS(stmp3xxx_wdt_pm_ops, >> + stmp3xxx_wdt_suspend, stmp3xxx_wdt_resume); Please align second line with ( >> + >> static struct platform_driver stmp3xxx_wdt_driver = { >> .driver = { >> .name = "stmp3xxx_rtc_wdt", >> + .owner = THIS_MODULE, Is this needed ? I have seen the .owner assignment being removed in other drivers recently. >> + .pm = &stmp3xxx_wdt_pm_ops, >> }, >> .probe = stmp3xxx_wdt_probe, >> .remove = stmp3xxx_wdt_remove, >> -- >> 1.7.11.3 >> >> >> W dniu 2014-09-18 13:07, Janusz Użycki pisze: >>> >>> I work under suspend/resume PM support for stmp3xxx driver. >>> >>> Most of them use suspend() with struct platform_device and CONFIG_PM. >>> Only sp805, sirfsoc, s3c2410, dw drivers use suspend() with struct device >>> and SIMPLE_DEV_PM_OPS. >>> Which implementation is preffered today? >>> >>> best regards >>> Janusz >>> >>> P.S. Patch on the moment: >>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> >>> --- >>> drivers/watchdog/stmp3xxx_rtc_wdt.c | 41 +++++++++++++++++++++++ >>> 1 file changed, 41 insertions(+) >>> >>> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> index 3546f03..12060ab 100644 >>> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> @@ -74,6 +74,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev) >>> { >>> int ret; >>> >>> + platform_set_drvdata(pdev, &stmp3xxx_wdd); >>> watchdog_set_drvdata(&stmp3xxx_wdd, &pdev->dev); >>> >>> stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); >>> @@ -95,12 +96,52 @@ static int stmp3xxx_wdt_remove(struct platform_device *pdev) >>> return 0; >>> } >>> >>> +#ifdef CONFIG_PM >>> +/* There is no conflict with rtc/rtc-stmp3xxx.c parent >>> + * because modified registers in PM functions are different */ >>> +static int stmp3xxx_wdt_suspend(struct platform_device *pdev, >>> + pm_message_t state) >>> +{ >>> + struct watchdog_device *wdd = platform_get_drvdata(pdev); >>> + >>> + /* Is keep-on/ping timer suspended before? >>> + * or additional driver-specific flag must be added >>> + * to block watchdog ping in the timer? >>> + * or disable WATCHDOG_KEEP_ON before wdt_stop >>> + * and restore it in resume? */ >>> + if (watchdog_active(wdd)) { >>> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); >>> + return wdt_stop(wdd); >>> + } >>> + /* should we use pm_runtime like omap_wdt.c does? */ >>> + >>> + return 0; >>> +} >>> + >>> +static int stmp3xxx_wdt_resume(struct platform_device *pdev) >>> +{ >>> + struct watchdog_device *wdd = platform_get_drvdata(pdev); >>> + >>> + if (watchdog_active(wdd)) { >>> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); >>> + return wdt_start(wdd); >>> + } >>> + >>> + return 0; >>> +} >>> +#else >>> +#define stmp3xxx_wdt_suspend NULL >>> +#define stmp3xxx_wdt_resume NULL >>> +#endif >>> + >>> static struct platform_driver stmp3xxx_wdt_driver = { >>> .driver = { >>> .name = "stmp3xxx_rtc_wdt", >>> }, >>> .probe = stmp3xxx_wdt_probe, >>> .remove = stmp3xxx_wdt_remove, >>> + .suspend = stmp3xxx_wdt_suspend, >>> + .resume = stmp3xxx_wdt_resume, >>> }; >>> module_platform_driver(stmp3xxx_wdt_driver); >>> >> > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog's pm support preffered implementation 2014-09-19 3:11 ` Guenter Roeck @ 2014-09-19 9:46 ` Janusz Użycki 2014-09-19 11:23 ` timers vs drivers suspend race Janusz Użycki 2014-09-19 16:21 ` watchdog's pm support preffered implementation Guenter Roeck 0 siblings, 2 replies; 23+ messages in thread From: Janusz Użycki @ 2014-09-19 9:46 UTC (permalink / raw) To: Guenter Roeck, linux-watchdog, Wim Van Sebroeck W dniu 2014-09-19 05:11, Guenter Roeck pisze: > On 09/18/2014 03:02 PM, Janusz Użycki wrote: >> Small fix below in the second implementation. >> >> best regards >> Janusz >> >> W dniu 2014-09-18 23:40, Janusz Użycki pisze: >>> Hi again, >>> >>> This is the second implementation. Which do you prefer? >>> > This one ok > >>> Subject: [PATCH] stmp3xxx_rtc_wdt: Add suspend/resume PM support >>> >>> >>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> >>> --- >>> drivers/watchdog/stmp3xxx_rtc_wdt.c | 39 +++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> b/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> index 3546f03..1946277 100644 >>> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> @@ -95,9 +95,48 @@ static int stmp3xxx_wdt_remove(struct >>> platform_device *pdev) >>> return 0; >>> } >>> >>> +#ifdef CONFIG_PM_SLEEP >>> +/* There is no conflict with rtc/rtc-stmp3xxx.c parent >>> + * because modified registers in PM functions are different */ > > Coding style, and what does this comment mean ? To me it is just > confusing. I will move to comment of commit > >>> +static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev) >>> +{ >>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >> >> drvdata is NULL, too fast, >> + struct watchdog_device *wdd = &stmp3xxx_wdd; >> >>> + >>> + /* Is keep-on/ping timer suspended before? >>> + * or additional driver-specific flag must be added >>> + * to block watchdog ping in the timer? >>> + * or disable WATCHDOG_KEEP_ON before wdt_stop >>> + * and restore it in resume? */ > > You'll have to answer those questions. I guess you don't know if timers are stopped before susnder of other drivers? > >>> + if (watchdog_active(wdd)) { >>> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > > Does this message add any value ? > >>> + return wdt_stop(wdd); >>> + } >>> + /* should we use pm_runtime like omap_wdt.c does? */ > > Isn't that what you do here ? I meant pm_runtime_put/get_sync() etc. Maybe it is connected to my question above about timers. > >>> + >>> + return 0; >>> +} >>> + >>> +static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev) > > Does the __maybe_unused really apply ? What do you preffer: __maybe_unused or ifdef CONFIG_PM_SLEEP? I guess the first one because SIMPLE_DEV_PM_OPS/SET_SYSTEM_SLEEP_PM_OPS just uses CONFIG_PM_SLEEP. > >>> +{ >>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >> + struct watchdog_device *wdd = &stmp3xxx_wdd; > > One of those lines needs to go. Sure, I wanted to fix it on the list in fast way. I'll use static stmp3xxx_wdd. > > Do you have indentation problems ? Do you use space for indentations, > maybe ? No, it's probably Thunderbird copy-paste despite text email format. When I finish fixes I will send patches using git: 1. watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2. watchdog: boottime protection feature (requires 'keep on') 3. stmp3xxx_rtc_wdt: WATCHDOG_KEEP_ON enabled 4. stmp3xxx_rtc_wdt: Add suspend/resume PM support > >> >>> + >>> + if (watchdog_active(wdd)) { >>> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > > Does this message add any value ? It was only for debug. Removed. > >>> + return wdt_start(wdd); >>> + } >>> + >>> + return 0; >>> +} >>> +#endif /* CONFIG_PM_SLEEP */ >>> + >>> +static SIMPLE_DEV_PM_OPS(stmp3xxx_wdt_pm_ops, >>> + stmp3xxx_wdt_suspend, stmp3xxx_wdt_resume); > > Please align second line with ( ok > >>> + >>> static struct platform_driver stmp3xxx_wdt_driver = { >>> .driver = { >>> .name = "stmp3xxx_rtc_wdt", >>> + .owner = THIS_MODULE, > > Is this needed ? I have seen the .owner assignment being removed > in other drivers recently. I just noticed that some drivers have the assignment and others not. https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/include/linux/platform_device.h?id=9447057eaff871dd7c63c808de761b8732407169 http://lxr.free-electrons.com/source/include/linux/platform_device.h?v=3.14 "use a macro to avoid include chaining to get THIS_MODULE" You are right. The direction is to remove this. Removed. "owner" history from must have to remove (for others): http://stackoverflow.com/questions/19467150/significance-of-this-module-in-linux-driver http://lists.linaro.org/pipermail/linaro-kernel/2013-July/005457.html https://lkml.org/lkml/2014/9/8/1 best regards Janusz ^ permalink raw reply [flat|nested] 23+ messages in thread
* timers vs drivers suspend race 2014-09-19 9:46 ` Janusz Użycki @ 2014-09-19 11:23 ` Janusz Użycki 2014-09-19 13:44 ` Janusz Użycki 2014-09-19 16:21 ` watchdog's pm support preffered implementation Guenter Roeck 1 sibling, 1 reply; 23+ messages in thread From: Janusz Użycki @ 2014-09-19 11:23 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Guenter Roeck, linux-watchdog, Wim Van Sebroeck, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5986 bytes --] Hi. I have a doubt about suspending of timers. I found the patch (not included to linux-next): https://lkml.org/lkml/2014/7/21/493 but it concerns timers only. I'm adding PM to the watchdog driver (below). The driver is capable to stop the watchdog. However I implemented "keep-on" feature to ping active watchdog if userland doesn't do it. For the feature I used a timer. Now, on wdt_suspend() call I don't know if timers are suspended before the watchdog driver or there is no guarantee (race): wdt_suspend() calls wdt_stop() and after timer's callback will ping watchdog and reenable it. I found also: https://lkml.org/lkml/2007/10/25/9 Here del_timer_sync() is called and it solves the problem. In my case the timer is serviced in watchdog_dev, not in the specific-driver. I can add a flag in the specific-driver to block watchdog's ping before wdt_stop() or export new function to suspend/resume watchdog_dev's timer from the specific-driver. Could you advice is it necessary? best regards Janusz -- Treść oryginalnej wiadomości -- Temat: Re: watchdog's pm support preffered implementation Data: Fri, 19 Sep 2014 11:46:11 +0200 Nadawca: Janusz Użycki <j.uzycki@elproma.com.pl> Adresat: Guenter Roeck <linux@roeck-us.net>, linux-watchdog@vger.kernel.org, Wim Van Sebroeck <wim@iguana.be> W dniu 2014-09-19 05:11, Guenter Roeck pisze: > On 09/18/2014 03:02 PM, Janusz Użycki wrote: >> Small fix below in the second implementation. >> >> best regards >> Janusz >> >> W dniu 2014-09-18 23:40, Janusz Użycki pisze: >>> Hi again, >>> >>> This is the second implementation. Which do you prefer? >>> > This one ok > >>> Subject: [PATCH] stmp3xxx_rtc_wdt: Add suspend/resume PM support >>> >>> >>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> >>> --- >>> drivers/watchdog/stmp3xxx_rtc_wdt.c | 39 +++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> b/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> index 3546f03..1946277 100644 >>> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c >>> @@ -95,9 +95,48 @@ static int stmp3xxx_wdt_remove(struct >>> platform_device *pdev) >>> return 0; >>> } >>> >>> +#ifdef CONFIG_PM_SLEEP >>> +/* There is no conflict with rtc/rtc-stmp3xxx.c parent >>> + * because modified registers in PM functions are different */ > > Coding style, and what does this comment mean ? To me it is just > confusing. I will move to comment of commit > >>> +static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev) >>> +{ >>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >> >> drvdata is NULL, too fast, >> + struct watchdog_device *wdd = &stmp3xxx_wdd; >> >>> + >>> + /* Is keep-on/ping timer suspended before? >>> + * or additional driver-specific flag must be added >>> + * to block watchdog ping in the timer? >>> + * or disable WATCHDOG_KEEP_ON before wdt_stop >>> + * and restore it in resume? */ > > You'll have to answer those questions. I guess you don't know if timers are stopped before susnder of other drivers? > >>> + if (watchdog_active(wdd)) { >>> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > > Does this message add any value ? > >>> + return wdt_stop(wdd); >>> + } >>> + /* should we use pm_runtime like omap_wdt.c does? */ > > Isn't that what you do here ? I meant pm_runtime_put/get_sync() etc. Maybe it is connected to my question above about timers. > >>> + >>> + return 0; >>> +} >>> + >>> +static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev) > > Does the __maybe_unused really apply ? What do you preffer: __maybe_unused or ifdef CONFIG_PM_SLEEP? I guess the first one because SIMPLE_DEV_PM_OPS/SET_SYSTEM_SLEEP_PM_OPS just uses CONFIG_PM_SLEEP. > >>> +{ >>> + struct watchdog_device *wdd = dev_get_drvdata(dev); >> + struct watchdog_device *wdd = &stmp3xxx_wdd; > > One of those lines needs to go. Sure, I wanted to fix it on the list in fast way. I'll use static stmp3xxx_wdd. > > Do you have indentation problems ? Do you use space for indentations, > maybe ? No, it's probably Thunderbird copy-paste despite text email format. When I finish fixes I will send patches using git: 1. watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature 2. watchdog: boottime protection feature (requires 'keep on') 3. stmp3xxx_rtc_wdt: WATCHDOG_KEEP_ON enabled 4. stmp3xxx_rtc_wdt: Add suspend/resume PM support > >> >>> + >>> + if (watchdog_active(wdd)) { >>> + dev_info(wdd->dev, "%s: wdt was active\n", __func__); > > Does this message add any value ? It was only for debug. Removed. > >>> + return wdt_start(wdd); >>> + } >>> + >>> + return 0; >>> +} >>> +#endif /* CONFIG_PM_SLEEP */ >>> + >>> +static SIMPLE_DEV_PM_OPS(stmp3xxx_wdt_pm_ops, >>> + stmp3xxx_wdt_suspend, stmp3xxx_wdt_resume); > > Please align second line with ( ok > >>> + >>> static struct platform_driver stmp3xxx_wdt_driver = { >>> .driver = { >>> .name = "stmp3xxx_rtc_wdt", >>> + .owner = THIS_MODULE, > > Is this needed ? I have seen the .owner assignment being removed > in other drivers recently. I just noticed that some drivers have the assignment and others not. https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/include/linux/platform_device.h?id=9447057eaff871dd7c63c808de761b8732407169 http://lxr.free-electrons.com/source/include/linux/platform_device.h?v=3.14 "use a macro to avoid include chaining to get THIS_MODULE" You are right. The direction is to remove this. Removed. "owner" history from must have to remove (for others): http://stackoverflow.com/questions/19467150/significance-of-this-module-in-linux-driver http://lists.linaro.org/pipermail/linaro-kernel/2013-July/005457.html https://lkml.org/lkml/2014/9/8/1 best regards Janusz [-- Attachment #2: Type: text/html, Size: 9164 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: timers vs drivers suspend race 2014-09-19 11:23 ` timers vs drivers suspend race Janusz Użycki @ 2014-09-19 13:44 ` Janusz Użycki 0 siblings, 0 replies; 23+ messages in thread From: Janusz Użycki @ 2014-09-19 13:44 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Guenter Roeck, linux-watchdog, Wim Van Sebroeck, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1210 bytes --] W dniu 2014-09-19 13:23, Janusz Użycki pisze: > Hi. > > I have a doubt about suspending of timers. > I found the patch (not included to linux-next): > https://lkml.org/lkml/2014/7/21/493 > but it concerns timers only. > > I'm adding PM to the watchdog driver (below). > The driver is capable to stop the watchdog. > However I implemented "keep-on" feature to ping > active watchdog if userland doesn't do it. > For the feature I used a timer. > Now, on wdt_suspend() call I don't know > if timers are suspended before the watchdog driver or > there is no guarantee (race): wdt_suspend() calls wdt_stop() > and after timer's callback will ping watchdog and reenable it. > > I found also: > https://lkml.org/lkml/2007/10/25/9 > Here del_timer_sync() is called and it solves > the problem. > In my case the timer is serviced in watchdog_dev, > not in the specific-driver. > I can add a flag in the specific-driver > to block watchdog's ping before wdt_stop() > or export new function to suspend/resume > watchdog_dev's timer from the specific-driver. > > Could you advice is it necessary? Does changing SIMPLE_DEV_PM_OPS to SET_LATE_SYSTEM_SLEEP_PM_OPS solve the race problem with timer? best regards Janusz [-- Attachment #2: Type: text/html, Size: 2141 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog's pm support preffered implementation 2014-09-19 9:46 ` Janusz Użycki 2014-09-19 11:23 ` timers vs drivers suspend race Janusz Użycki @ 2014-09-19 16:21 ` Guenter Roeck 2014-09-23 12:07 ` Janusz Użycki 1 sibling, 1 reply; 23+ messages in thread From: Guenter Roeck @ 2014-09-19 16:21 UTC (permalink / raw) To: Janusz Użycki; +Cc: linux-watchdog, Wim Van Sebroeck On Fri, Sep 19, 2014 at 11:46:11AM +0200, Janusz Użycki wrote: > > W dniu 2014-09-19 05:11, Guenter Roeck pisze: > >On 09/18/2014 03:02 PM, Janusz Użycki wrote: > >>Small fix below in the second implementation. > >> > >>best regards > >>Janusz > >> > >>W dniu 2014-09-18 23:40, Janusz Użycki pisze: > >>>Hi again, > >>> > >>>This is the second implementation. Which do you prefer? > >>> > >This one > > ok > > > > >>>Subject: [PATCH] stmp3xxx_rtc_wdt: Add suspend/resume PM support > >>> > >>> > >>>Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl> > >>>--- > >>> drivers/watchdog/stmp3xxx_rtc_wdt.c | 39 +++++++++++++++++++++++ > >>> 1 file changed, 39 insertions(+) > >>> > >>>diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c > >>>b/drivers/watchdog/stmp3xxx_rtc_wdt.c > >>>index 3546f03..1946277 100644 > >>>--- a/drivers/watchdog/stmp3xxx_rtc_wdt.c > >>>+++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c > >>>@@ -95,9 +95,48 @@ static int stmp3xxx_wdt_remove(struct > >>>platform_device *pdev) > >>> return 0; > >>> } > >>> > >>>+#ifdef CONFIG_PM_SLEEP > >>>+/* There is no conflict with rtc/rtc-stmp3xxx.c parent > >>>+ * because modified registers in PM functions are different */ > > > >Coding style, and what does this comment mean ? To me it is just > >confusing. > > I will move to comment of commit > > > > >>>+static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev) > >>>+{ > >>>+ struct watchdog_device *wdd = dev_get_drvdata(dev); > >> > >>drvdata is NULL, too fast, > >>+ struct watchdog_device *wdd = &stmp3xxx_wdd; > >> > >>>+ > >>>+ /* Is keep-on/ping timer suspended before? > >>>+ * or additional driver-specific flag must be added > >>>+ * to block watchdog ping in the timer? > >>>+ * or disable WATCHDOG_KEEP_ON before wdt_stop > >>>+ * and restore it in resume? */ > > > >You'll have to answer those questions. > > I guess you don't know if timers are stopped before susnder of other > drivers? > My development methology is on a "need to know" basis. No, I don't know, since I never needed to. If I am in a situation like this I look at other drivers and how they solved the problem. On a side note, I would never assume that there is a race condition unless I can prove that there is one. > > > >>>+ if (watchdog_active(wdd)) { > >>>+ dev_info(wdd->dev, "%s: wdt was active\n", __func__); > > > >Does this message add any value ? > > > >>>+ return wdt_stop(wdd); > >>>+ } > >>>+ /* should we use pm_runtime like omap_wdt.c does? */ > > > >Isn't that what you do here ? > > I meant pm_runtime_put/get_sync() etc. > Maybe it is connected to my question above about timers. > Another need-to-know question. Best would be to check how other drivers use the functions. > > > >>>+ > >>>+ return 0; > >>>+} > >>>+ > >>>+static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev) > > > >Does the __maybe_unused really apply ? > > What do you preffer: __maybe_unused or ifdef CONFIG_PM_SLEEP? > I guess the first one because SIMPLE_DEV_PM_OPS/SET_SYSTEM_SLEEP_PM_OPS > just uses CONFIG_PM_SLEEP. > None of the above ;-). Keep the __maybe_unused; at least that means that the code is compiled even if CONFIG_PM_SLEEP is undefined. Guenter ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: watchdog's pm support preffered implementation 2014-09-19 16:21 ` watchdog's pm support preffered implementation Guenter Roeck @ 2014-09-23 12:07 ` Janusz Użycki 0 siblings, 0 replies; 23+ messages in thread From: Janusz Użycki @ 2014-09-23 12:07 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-watchdog, Wim Van Sebroeck W dniu 2014-09-19 18:21, Guenter Roeck pisze: > On Fri, Sep 19, 2014 at 11:46:11AM +0200, Janusz Użycki wrote: >> W dniu 2014-09-19 05:11, Guenter Roeck pisze: >>>>> + >>>>> + /* Is keep-on/ping timer suspended before? >>>>> + * or additional driver-specific flag must be added >>>>> + * to block watchdog ping in the timer? >>>>> + * or disable WATCHDOG_KEEP_ON before wdt_stop >>>>> + * and restore it in resume? */ >>> You'll have to answer those questions. >> I guess you don't know if timers are stopped before susnder of other >> drivers? >> > My development methology is on a "need to know" basis. No, I don't know, > since I never needed to. If I am in a situation like this I look at other > drivers and how they solved the problem. My methodology is also "need to know" but there was possibility that somebody knows an answer. Other watchdog drivers hadn't such situation. Ping timer was always used for hardware which never stops. I analized and found simple solution: suspend/resume helper. You can find it in patches I sent yesterday. Please about comments. Do you happen to work how to use the ping timer instead of timer in device-specific driver? I can't it on the moment. > On a side note, I would never assume that there is a race condition unless > I can prove that there is one. > I am careful here: https://www.kernel.org/doc/Documentation/stable_kernel_rules.txt "No theoretical race condition issues" >>>>> + return wdt_stop(wdd); >>>>> + } >>>>> + /* should we use pm_runtime like omap_wdt.c does? */ >>> Isn't that what you do here ? >> I meant pm_runtime_put/get_sync() etc. >> Maybe it is connected to my question above about timers. >> > Another need-to-know question. > Best would be to check how other drivers use the functions. I learnt: pm_runtime is rather extention to classic resume/suspend. > >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev) >>> Does the __maybe_unused really apply ? >> What do you preffer: __maybe_unused or ifdef CONFIG_PM_SLEEP? >> I guess the first one because SIMPLE_DEV_PM_OPS/SET_SYSTEM_SLEEP_PM_OPS >> just uses CONFIG_PM_SLEEP. >> > None of the above ;-). Keep the __maybe_unused; at least that means > that the code is compiled even if CONFIG_PM_SLEEP is undefined. sure Janusz ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2014-09-23 12:07 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <S1751381AbaIDOwq/20140904145246Z+988@vger.kernel.org>
2014-09-04 15:47 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space Janusz Użycki
2014-09-04 16:05 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space [proposal] Janusz Użycki
2014-09-04 16:24 ` Janusz Użycki
2014-09-04 17:23 ` Fwd: " Janusz Użycki
2014-09-05 6:47 ` Janusz Użycki
2014-09-07 17:18 ` watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space Guenter Roeck
2014-09-08 1:14 ` watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature Janusz Użycki
2014-09-08 1:18 ` Janusz Użycki
2014-09-08 3:24 ` Guenter Roeck
2014-09-08 3:16 ` Guenter Roeck
2014-09-08 12:14 ` Janusz Użycki
2014-09-10 17:24 ` Janusz Użycki
2014-09-11 10:47 ` Janusz Użycki
2014-09-17 11:09 ` Janusz Użycki
2014-09-18 11:07 ` watchdog's pm support preffered implementation Janusz Użycki
2014-09-18 21:40 ` Janusz Użycki
2014-09-18 22:02 ` Janusz Użycki
2014-09-19 3:11 ` Guenter Roeck
2014-09-19 9:46 ` Janusz Użycki
2014-09-19 11:23 ` timers vs drivers suspend race Janusz Użycki
2014-09-19 13:44 ` Janusz Użycki
2014-09-19 16:21 ` watchdog's pm support preffered implementation Guenter Roeck
2014-09-23 12:07 ` Janusz Użycki
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.