[PATCH 0/2] watchdog: fix OOPS when using stop_on_unregister and use it for R-Car

[PATCH 0/2] watchdog: fix OOPS when using stop_on_unregister and use it for R-Car

[Wolfram Sang]

I wanted to activate 'stop_on_unregister' for the Renesas R-Car driver (see
patch 2) and stumbled over an OOPS when using it. So, patch 1 addresses this
problem to the best of my knowledge. Looking forward to comments.

Happy hacking,

   Wolfram


Wolfram Sang (2):
  watchdog: core: fix null pointer dereference when releasing cdev
  watchdog: renesas_wdt: stop when unregistering

 drivers/watchdog/renesas_wdt.c  |  1 +
 drivers/watchdog/watchdog_dev.c | 10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH 1/2] watchdog: core: fix null pointer dereference when releasing cdev

[Wolfram Sang]

watchdog_stop() calls watchdog_update_worker() which needs a valid
wdd->wd_data pointer. So, when unregistering the cdev, clear the
pointers after we call watchdog_stop(), not before.

Fixes: bb292ac1c602 ("watchdog: Introduce watchdog_stop_on_unregister helper")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/watchdog_dev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ffbdc4642ea5..f6c24b22b37c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1019,16 +1019,16 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
 		old_wd_data = NULL;
 	}
 
-	mutex_lock(&wd_data->lock);
-	wd_data->wdd = NULL;
-	wdd->wd_data = NULL;
-	mutex_unlock(&wd_data->lock);
-
 	if (watchdog_active(wdd) &&
 	    test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
 		watchdog_stop(wdd);
 	}
 
+	mutex_lock(&wd_data->lock);
+	wd_data->wdd = NULL;
+	wdd->wd_data = NULL;
+	mutex_unlock(&wd_data->lock);
+
 	hrtimer_cancel(&wd_data->timer);
 	kthread_cancel_work_sync(&wd_data->work);
 
-- 
2.11.0

Re: [PATCH 1/2] watchdog: core: fix null pointer dereference when releasing cdev

[Guenter Roeck]

On 08/28/2018 03:13 AM, Wolfram Sang wrote:
> watchdog_stop() calls watchdog_update_worker() which needs a valid
> wdd->wd_data pointer. So, when unregistering the cdev, clear the
> pointers after we call watchdog_stop(), not before.
> 
> Fixes: bb292ac1c602 ("watchdog: Introduce watchdog_stop_on_unregister helper")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

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

> ---
>   drivers/watchdog/watchdog_dev.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index ffbdc4642ea5..f6c24b22b37c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1019,16 +1019,16 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>   		old_wd_data = NULL;
>   	}
>   
> -	mutex_lock(&wd_data->lock);
> -	wd_data->wdd = NULL;
> -	wdd->wd_data = NULL;
> -	mutex_unlock(&wd_data->lock);
> -
>   	if (watchdog_active(wdd) &&
>   	    test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
>   		watchdog_stop(wdd);
>   	}
>   
> +	mutex_lock(&wd_data->lock);
> +	wd_data->wdd = NULL;
> +	wdd->wd_data = NULL;
> +	mutex_unlock(&wd_data->lock);
> +
>   	hrtimer_cancel(&wd_data->timer);
>   	kthread_cancel_work_sync(&wd_data->work);
>   
> 

RE: [PATCH 1/2] watchdog: core: fix null pointer dereference when releasing cdev

[Fabrizio Castro]

> Subject: [PATCH 1/2] watchdog: core: fix null pointer dereference when releasing cdev
>
> watchdog_stop() calls watchdog_update_worker() which needs a valid
> wdd->wd_data pointer. So, when unregistering the cdev, clear the
> pointers after we call watchdog_stop(), not before.
>
> Fixes: bb292ac1c602 ("watchdog: Introduce watchdog_stop_on_unregister helper")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> ---
>  drivers/watchdog/watchdog_dev.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index ffbdc4642ea5..f6c24b22b37c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1019,16 +1019,16 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>  old_wd_data = NULL;
>  }
>
> -mutex_lock(&wd_data->lock);
> -wd_data->wdd = NULL;
> -wdd->wd_data = NULL;
> -mutex_unlock(&wd_data->lock);
> -
>  if (watchdog_active(wdd) &&
>      test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
>  watchdog_stop(wdd);
>  }
>
> +mutex_lock(&wd_data->lock);
> +wd_data->wdd = NULL;
> +wdd->wd_data = NULL;
> +mutex_unlock(&wd_data->lock);
> +
>  hrtimer_cancel(&wd_data->timer);
>  kthread_cancel_work_sync(&wd_data->work);
>
> --
> 2.11.0




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

[PATCH 2/2] watchdog: renesas_wdt: stop when unregistering

[Wolfram Sang]

We want to go into a sane state when unregistering. Currently, it
happens that the watchdog stops when unbinding because of RuntimePM
stopping the core clock. When rebinding, the core clock gets reactivated
and the watchdog fires even though it hasn't been opened by userspace
yet. Strange scenario, yes, but sane state is much preferred anyhow.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/renesas_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 88d81feba4e6..f45cb183fa75 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -234,6 +234,7 @@ static int rwdt_probe(struct platform_device *pdev)
 	watchdog_set_drvdata(&priv->wdev, priv);
 	watchdog_set_nowayout(&priv->wdev, nowayout);
 	watchdog_set_restart_priority(&priv->wdev, 0);
+	watchdog_stop_on_unregister(&priv->wdev);
 
 	/* This overrides the default timeout only if DT configuration was found */
 	ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
-- 
2.11.0

Re: [PATCH 2/2] watchdog: renesas_wdt: stop when unregistering

[Guenter Roeck]

On 08/28/2018 03:13 AM, Wolfram Sang wrote:
> We want to go into a sane state when unregistering. Currently, it
> happens that the watchdog stops when unbinding because of RuntimePM
> stopping the core clock. When rebinding, the core clock gets reactivated
> and the watchdog fires even though it hasn't been opened by userspace
> yet. Strange scenario, yes, but sane state is much preferred anyhow.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

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

> ---
>   drivers/watchdog/renesas_wdt.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 88d81feba4e6..f45cb183fa75 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -234,6 +234,7 @@ static int rwdt_probe(struct platform_device *pdev)
>   	watchdog_set_drvdata(&priv->wdev, priv);
>   	watchdog_set_nowayout(&priv->wdev, nowayout);
>   	watchdog_set_restart_priority(&priv->wdev, 0);
> +	watchdog_stop_on_unregister(&priv->wdev);
>   
>   	/* This overrides the default timeout only if DT configuration was found */
>   	ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
> 

RE: [PATCH 2/2] watchdog: renesas_wdt: stop when unregistering

[Fabrizio Castro]

> Subject: [PATCH 2/2] watchdog: renesas_wdt: stop when unregistering
>
> We want to go into a sane state when unregistering. Currently, it
> happens that the watchdog stops when unbinding because of RuntimePM
> stopping the core clock. When rebinding, the core clock gets reactivated
> and the watchdog fires even though it hasn't been opened by userspace
> yet. Strange scenario, yes, but sane state is much preferred anyhow.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> ---
>  drivers/watchdog/renesas_wdt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 88d81feba4e6..f45cb183fa75 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -234,6 +234,7 @@ static int rwdt_probe(struct platform_device *pdev)
>  watchdog_set_drvdata(&priv->wdev, priv);
>  watchdog_set_nowayout(&priv->wdev, nowayout);
>  watchdog_set_restart_priority(&priv->wdev, 0);
> +watchdog_stop_on_unregister(&priv->wdev);
>
>  /* This overrides the default timeout only if DT configuration was found */
>  ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
> --
> 2.11.0




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.