* [PATCH v2 2/2] watchdog: dw_wdt: keepalive the watchdog at write time
2015-05-08 4:27 [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock Doug Anderson
@ 2015-05-08 4:27 ` Doug Anderson
2015-05-08 5:24 ` Dmitry Torokhov
2015-05-08 4:51 ` [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock Guenter Roeck
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2015-05-08 4:27 UTC (permalink / raw)
To: wim, linux
Cc: jszhang, Dmitry Torokhov, Doug Anderson, linux-watchdog,
linux-kernel
If you've got code that does this in a tight loop
1. Open watchdog
2. Send 'expect close'
3. Close watchdog
...you'll eventually trigger a watchdog reset. You can reproduce this
by using daisydog (1) and running:
while true; do daisydog -c > /dev/null; done
The problem is that each time you write to the watchdog for 'expect
close' it moves the timer .5 seconds out. The timer thus never fires
and never pats the watchdog for you.
1: http://git.chromium.org/gitweb/?p=chromiumos/third_party/daisydog.git
Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Jisheng Zhang <jszhang@marvell.com>
---
Changes in v2: None
drivers/watchdog/dw_wdt.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index a284abd..6ea0634 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -215,6 +215,7 @@ static ssize_t dw_wdt_write(struct file *filp, const char __user *buf,
}
dw_wdt_set_next_heartbeat();
+ dw_wdt_keepalive();
mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
return len;
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/2] watchdog: dw_wdt: keepalive the watchdog at write time
2015-05-08 4:27 ` [PATCH v2 2/2] watchdog: dw_wdt: keepalive the watchdog at write time Doug Anderson
@ 2015-05-08 5:24 ` Dmitry Torokhov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2015-05-08 5:24 UTC (permalink / raw)
To: Doug Anderson; +Cc: wim, linux, jszhang, linux-watchdog, linux-kernel
On Thu, May 07, 2015 at 09:27:45PM -0700, Doug Anderson wrote:
> If you've got code that does this in a tight loop
> 1. Open watchdog
> 2. Send 'expect close'
> 3. Close watchdog
> ...you'll eventually trigger a watchdog reset. You can reproduce this
> by using daisydog (1) and running:
> while true; do daisydog -c > /dev/null; done
>
> The problem is that each time you write to the watchdog for 'expect
> close' it moves the timer .5 seconds out. The timer thus never fires
> and never pats the watchdog for you.
>
> 1: http://git.chromium.org/gitweb/?p=chromiumos/third_party/daisydog.git
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Jisheng Zhang <jszhang@marvell.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> Changes in v2: None
>
> drivers/watchdog/dw_wdt.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index a284abd..6ea0634 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -215,6 +215,7 @@ static ssize_t dw_wdt_write(struct file *filp, const char __user *buf,
> }
>
> dw_wdt_set_next_heartbeat();
> + dw_wdt_keepalive();
> mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
>
> return len;
> --
> 2.2.0.rc0.207.ga3a616c
>
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock
2015-05-08 4:27 [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock Doug Anderson
2015-05-08 4:27 ` [PATCH v2 2/2] watchdog: dw_wdt: keepalive the watchdog at write time Doug Anderson
@ 2015-05-08 4:51 ` Guenter Roeck
2015-05-08 5:01 ` Jisheng Zhang
2015-05-08 5:23 ` Dmitry Torokhov
3 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2015-05-08 4:51 UTC (permalink / raw)
To: Doug Anderson, wim; +Cc: jszhang, Dmitry Torokhov, linux-watchdog, linux-kernel
On 05/07/2015 09:27 PM, Doug Anderson wrote:
> Right now the dw_wdt uses a spinlock to protect dw_wdt_open(). The
> problem is that while holding the spinlock we call:
> -> dw_wdt_set_top()
> -> dw_wdt_top_in_seconds()
> -> clk_get_rate()
> -> clk_prepare_lock()
> -> mutex_lock()
>
> Locking a mutex while holding a spinlock is not allowed and leads to
> warnings like "BUG: spinlock wrong CPU on CPU#1", among other
> problems.
>
> There's no reason to use a spinlock. Only dw_wdt_open() was protected
> and the test_and_set_bit() at the start of that function protects us
> anyway.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock
2015-05-08 4:27 [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock Doug Anderson
2015-05-08 4:27 ` [PATCH v2 2/2] watchdog: dw_wdt: keepalive the watchdog at write time Doug Anderson
2015-05-08 4:51 ` [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock Guenter Roeck
@ 2015-05-08 5:01 ` Jisheng Zhang
2015-05-08 5:23 ` Dmitry Torokhov
3 siblings, 0 replies; 6+ messages in thread
From: Jisheng Zhang @ 2015-05-08 5:01 UTC (permalink / raw)
To: Doug Anderson, wim@iguana.be, linux@roeck-us.net
Cc: Dmitry Torokhov, linux-watchdog@vger.kernel.org,
linux-kernel@vger.kernel.org
On Thu, 7 May 2015 21:27:44 -0700
Doug Anderson <dianders@chromium.org> wrote:
> Right now the dw_wdt uses a spinlock to protect dw_wdt_open(). The
> problem is that while holding the spinlock we call:
> -> dw_wdt_set_top()
> -> dw_wdt_top_in_seconds()
> -> clk_get_rate()
> -> clk_prepare_lock()
> -> mutex_lock()
>
> Locking a mutex while holding a spinlock is not allowed and leads to
> warnings like "BUG: spinlock wrong CPU on CPU#1", among other
> problems.
>
> There's no reason to use a spinlock. Only dw_wdt_open() was protected
> and the test_and_set_bit() at the start of that function protects us
> anyway.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Don't switch to mutex; just don't use spinlock at all as per Dmitry
>
> drivers/watchdog/dw_wdt.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index d0bb949..a284abd 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -35,7 +35,6 @@
> #include <linux/pm.h>
> #include <linux/platform_device.h>
> #include <linux/reboot.h>
> -#include <linux/spinlock.h>
> #include <linux/timer.h>
> #include <linux/uaccess.h>
> #include <linux/watchdog.h>
> @@ -61,7 +60,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> #define WDT_TIMEOUT (HZ / 2)
>
> static struct {
> - spinlock_t lock;
> void __iomem *regs;
> struct clk *clk;
> unsigned long in_use;
> @@ -177,7 +175,6 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
> /* Make sure we don't get unloaded. */
> __module_get(THIS_MODULE);
>
> - spin_lock(&dw_wdt.lock);
> if (!dw_wdt_is_enabled()) {
> /*
> * The watchdog is not currently enabled. Set the timeout to
> @@ -190,8 +187,6 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
>
> dw_wdt_set_next_heartbeat();
>
> - spin_unlock(&dw_wdt.lock);
> -
> return nonseekable_open(inode, filp);
> }
>
> @@ -348,8 +343,6 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - spin_lock_init(&dw_wdt.lock);
> -
> ret = misc_register(&dw_wdt_miscdev);
> if (ret)
> goto out_disable_clk;
Tested-by: Jisheng Zhang <jszhang@marvell.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock
2015-05-08 4:27 [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock Doug Anderson
` (2 preceding siblings ...)
2015-05-08 5:01 ` Jisheng Zhang
@ 2015-05-08 5:23 ` Dmitry Torokhov
3 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2015-05-08 5:23 UTC (permalink / raw)
To: Doug Anderson; +Cc: wim, linux, jszhang, linux-watchdog, linux-kernel
On Thu, May 07, 2015 at 09:27:44PM -0700, Doug Anderson wrote:
> Right now the dw_wdt uses a spinlock to protect dw_wdt_open(). The
> problem is that while holding the spinlock we call:
> -> dw_wdt_set_top()
> -> dw_wdt_top_in_seconds()
> -> clk_get_rate()
> -> clk_prepare_lock()
> -> mutex_lock()
>
> Locking a mutex while holding a spinlock is not allowed and leads to
> warnings like "BUG: spinlock wrong CPU on CPU#1", among other
> problems.
>
> There's no reason to use a spinlock. Only dw_wdt_open() was protected
> and the test_and_set_bit() at the start of that function protects us
> anyway.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Don't switch to mutex; just don't use spinlock at all as per Dmitry
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> drivers/watchdog/dw_wdt.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index d0bb949..a284abd 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -35,7 +35,6 @@
> #include <linux/pm.h>
> #include <linux/platform_device.h>
> #include <linux/reboot.h>
> -#include <linux/spinlock.h>
> #include <linux/timer.h>
> #include <linux/uaccess.h>
> #include <linux/watchdog.h>
> @@ -61,7 +60,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> #define WDT_TIMEOUT (HZ / 2)
>
> static struct {
> - spinlock_t lock;
> void __iomem *regs;
> struct clk *clk;
> unsigned long in_use;
> @@ -177,7 +175,6 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
> /* Make sure we don't get unloaded. */
> __module_get(THIS_MODULE);
>
> - spin_lock(&dw_wdt.lock);
> if (!dw_wdt_is_enabled()) {
> /*
> * The watchdog is not currently enabled. Set the timeout to
> @@ -190,8 +187,6 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
>
> dw_wdt_set_next_heartbeat();
>
> - spin_unlock(&dw_wdt.lock);
> -
> return nonseekable_open(inode, filp);
> }
>
> @@ -348,8 +343,6 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - spin_lock_init(&dw_wdt.lock);
> -
> ret = misc_register(&dw_wdt_miscdev);
> if (ret)
> goto out_disable_clk;
> --
> 2.2.0.rc0.207.ga3a616c
>
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread