[PATCH 1/2] watchdog: dw_wdt: Use a mutex, not a spinlock

[PATCH 1/2] watchdog: dw_wdt: Use a mutex, not 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, so switch to a mutex.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/watchdog/dw_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index d0bb949..3fa2f19 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -30,12 +30,12 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/of.h>
 #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 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 #define WDT_TIMEOUT		(HZ / 2)
 
 static struct {
-	spinlock_t		lock;
+	struct mutex		lock;
 	void __iomem		*regs;
 	struct clk		*clk;
 	unsigned long		in_use;
@@ -177,7 +177,7 @@ 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);
+	mutex_lock(&dw_wdt.lock);
 	if (!dw_wdt_is_enabled()) {
 		/*
 		 * The watchdog is not currently enabled. Set the timeout to
@@ -190,7 +190,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
 
 	dw_wdt_set_next_heartbeat();
 
-	spin_unlock(&dw_wdt.lock);
+	mutex_unlock(&dw_wdt.lock);
 
 	return nonseekable_open(inode, filp);
 }
@@ -348,7 +348,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	spin_lock_init(&dw_wdt.lock);
+	mutex_init(&dw_wdt.lock);
 
 	ret = misc_register(&dw_wdt_miscdev);
 	if (ret)
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 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>
---
 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 3fa2f19..ff5d734 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -220,6 +220,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 1/2] watchdog: dw_wdt: Use a mutex, not a spinlock

[Guenter Roeck]

On 05/07/2015 03:09 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, so switch to a mutex.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

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

[Guenter Roeck]

On 05/07/2015 03:09 PM, 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>

Re: [PATCH 1/2] watchdog: dw_wdt: Use a mutex, not a spinlock

[Jisheng Zhang]

On Thu, 7 May 2015 15:09:23 -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, so switch to a mutex.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/watchdog/dw_wdt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index d0bb949..3fa2f19 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -30,12 +30,12 @@
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/mutex.h>
>  #include <linux/notifier.h>
>  #include <linux/of.h>
>  #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 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>  #define WDT_TIMEOUT		(HZ / 2)
>  
>  static struct {
> -	spinlock_t		lock;
> +	struct mutex		lock;
>  	void __iomem		*regs;
>  	struct clk		*clk;
>  	unsigned long		in_use;
> @@ -177,7 +177,7 @@ 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);
> +	mutex_lock(&dw_wdt.lock);
>  	if (!dw_wdt_is_enabled()) {
>  		/*
>  		 * The watchdog is not currently enabled. Set the timeout to
> @@ -190,7 +190,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
>  
>  	dw_wdt_set_next_heartbeat();
>  
> -	spin_unlock(&dw_wdt.lock);
> +	mutex_unlock(&dw_wdt.lock);
>  
>  	return nonseekable_open(inode, filp);
>  }
> @@ -348,7 +348,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	spin_lock_init(&dw_wdt.lock);
> +	mutex_init(&dw_wdt.lock);
>  
>  	ret = misc_register(&dw_wdt_miscdev);
>  	if (ret)

Tested on marvell BG2Q SoC, no issue found. So

Tested-by: Jisheng Zhang <jszhang@marvell.com>

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

[Jisheng Zhang]

On Thu, 7 May 2015 15:09:24 -0700
Doug Anderson <dianders@chromium.org> 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>
> ---
>  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 3fa2f19..ff5d734 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -220,6 +220,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;

Tested-by: Jisheng Zhang <jszhang@marvell.com>

Re: [PATCH 1/2] watchdog: dw_wdt: Use a mutex, not a spinlock

[Doug Anderson]

Hi,

On Thu, May 7, 2015 at 6:47 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 05/07/2015 03:09 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, so switch to a mutex.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

As Dmitry pointed out in another context, and even better fix is to
just remove the spinlock altogether.  I'll send up v2...

-Doug