[PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock

[PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock

[Doug Anderson]

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;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v2 2/2] watchdog: dw_wdt: keepalive the watchdog at write time

[Doug Anderson]

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

Re: [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock

[Guenter Roeck]

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>

Re: [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock

[Jisheng Zhang]

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>

Re: [PATCH v2 1/2] watchdog: dw_wdt: No need for a spinlock

[Dmitry Torokhov]

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

Re: [PATCH v2 2/2] watchdog: dw_wdt: keepalive the watchdog at write time

[Dmitry Torokhov]

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