[PATCH v2] watchdog: core: Make use of devm_register_reboot_notifier()

[PATCH v2] watchdog: core: Make use of devm_register_reboot_notifier()

[Andrey Smirnov]

Save a bit of cleanup code by leveraging newly added
devm_register_reboot_notifier().

Cc: cphealy@gmail.com
Cc: linux-kernel@vger.kernel.org
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Guenter, Andy:

Here's the second version of the patch originally submitted in
[v1]. This time I tried to follow Guenter's suggestion of moving this
bit on initialization in watchdog_dev_register().

If I missed something obvious why this patch wouldn't work, again,
please let me know.

Patch introducting devm_register_reboot_notifier() can be found in
[other-patch]

Thanks,
Andrey Smirnov

[v1] https://lkml.org/lkml/2017/4/4/388
[other-patch] https://lkml.org/lkml/2017/3/20/671


 drivers/watchdog/watchdog_core.c | 35 -----------------------------------
 drivers/watchdog/watchdog_dev.c  | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 74265b2..8a8d952 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -137,25 +137,6 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 }
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
 
-static int watchdog_reboot_notifier(struct notifier_block *nb,
-				    unsigned long code, void *data)
-{
-	struct watchdog_device *wdd = container_of(nb, struct watchdog_device,
-						   reboot_nb);
-
-	if (code == SYS_DOWN || code == SYS_HALT) {
-		if (watchdog_active(wdd)) {
-			int ret;
-
-			ret = wdd->ops->stop(wdd);
-			if (ret)
-				return NOTIFY_BAD;
-		}
-	}
-
-	return NOTIFY_DONE;
-}
-
 static int watchdog_restart_notifier(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
@@ -244,19 +225,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		}
 	}
 
-	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
-		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
-
-		ret = register_reboot_notifier(&wdd->reboot_nb);
-		if (ret) {
-			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
-			       wdd->id, ret);
-			watchdog_dev_unregister(wdd);
-			ida_simple_remove(&watchdog_ida, wdd->id);
-			return ret;
-		}
-	}
-
 	if (wdd->ops->restart) {
 		wdd->restart_nb.notifier_call = watchdog_restart_notifier;
 
@@ -302,9 +270,6 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
 	if (wdd->ops->restart)
 		unregister_restart_handler(&wdd->restart_nb);
 
-	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
-		unregister_reboot_notifier(&wdd->reboot_nb);
-
 	watchdog_dev_unregister(wdd);
 	ida_simple_remove(&watchdog_ida, wdd->id);
 }
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 32930a0..9a52990 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -42,6 +42,7 @@
 #include <linux/miscdevice.h>	/* For handling misc devices */
 #include <linux/module.h>	/* For module stuff/... */
 #include <linux/mutex.h>	/* For mutexes */
+#include <linux/reboot.h>	/* For reboot notifier */
 #include <linux/slab.h>		/* For memory functions */
 #include <linux/types.h>	/* For standard types (like size_t) */
 #include <linux/watchdog.h>	/* For watchdog specific items */
@@ -998,6 +999,25 @@ static struct class watchdog_class = {
 	.dev_groups =	wdt_groups,
 };
 
+static int watchdog_reboot_notifier(struct notifier_block *nb,
+				    unsigned long code, void *data)
+{
+	struct watchdog_device *wdd = container_of(nb, struct watchdog_device,
+						   reboot_nb);
+
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		if (watchdog_active(wdd)) {
+			int ret;
+
+			ret = wdd->ops->stop(wdd);
+			if (ret)
+				return NOTIFY_BAD;
+		}
+	}
+
+	return NOTIFY_DONE;
+}
+
 /*
  *	watchdog_dev_register: register a watchdog device
  *	@wdd: watchdog device
@@ -1031,6 +1051,18 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 	if (ret) {
 		device_destroy(&watchdog_class, devno);
 		watchdog_cdev_unregister(wdd);
+		return ret;
+	}
+
+	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
+		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
+
+		ret = devm_register_reboot_notifier(dev, &wdd->reboot_nb);
+		if (ret) {
+			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
+			       wdd->id, ret);
+			watchdog_dev_unregister(wdd);
+		}
 	}
 
 	return ret;
-- 
2.9.3

Re: [PATCH v2] watchdog: core: Make use of devm_register_reboot_notifier()

[Guenter Roeck]

On 04/11/2017 09:06 AM, Andrey Smirnov wrote:
> Save a bit of cleanup code by leveraging newly added
> devm_register_reboot_notifier().
>
> Cc: cphealy@gmail.com
> Cc: linux-kernel@vger.kernel.org
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

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

> ---
>
> Guenter, Andy:
>
> Here's the second version of the patch originally submitted in
> [v1]. This time I tried to follow Guenter's suggestion of moving this
> bit on initialization in watchdog_dev_register().
>
> If I missed something obvious why this patch wouldn't work, again,
> please let me know.
>
> Patch introducting devm_register_reboot_notifier() can be found in
> [other-patch]
>
> Thanks,
> Andrey Smirnov
>
> [v1] https://lkml.org/lkml/2017/4/4/388
> [other-patch] https://lkml.org/lkml/2017/3/20/671
>
>
>  drivers/watchdog/watchdog_core.c | 35 -----------------------------------
>  drivers/watchdog/watchdog_dev.c  | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 74265b2..8a8d952 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -137,25 +137,6 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>  }
>  EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>
> -static int watchdog_reboot_notifier(struct notifier_block *nb,
> -				    unsigned long code, void *data)
> -{
> -	struct watchdog_device *wdd = container_of(nb, struct watchdog_device,
> -						   reboot_nb);
> -
> -	if (code == SYS_DOWN || code == SYS_HALT) {
> -		if (watchdog_active(wdd)) {
> -			int ret;
> -
> -			ret = wdd->ops->stop(wdd);
> -			if (ret)
> -				return NOTIFY_BAD;
> -		}
> -	}
> -
> -	return NOTIFY_DONE;
> -}
> -
>  static int watchdog_restart_notifier(struct notifier_block *nb,
>  				     unsigned long action, void *data)
>  {
> @@ -244,19 +225,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>
> -	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> -		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> -
> -		ret = register_reboot_notifier(&wdd->reboot_nb);
> -		if (ret) {
> -			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
> -			       wdd->id, ret);
> -			watchdog_dev_unregister(wdd);
> -			ida_simple_remove(&watchdog_ida, wdd->id);
> -			return ret;
> -		}
> -	}
> -
>  	if (wdd->ops->restart) {
>  		wdd->restart_nb.notifier_call = watchdog_restart_notifier;
>
> @@ -302,9 +270,6 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
>  	if (wdd->ops->restart)
>  		unregister_restart_handler(&wdd->restart_nb);
>
> -	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> -		unregister_reboot_notifier(&wdd->reboot_nb);
> -
>  	watchdog_dev_unregister(wdd);
>  	ida_simple_remove(&watchdog_ida, wdd->id);
>  }
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 32930a0..9a52990 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -42,6 +42,7 @@
>  #include <linux/miscdevice.h>	/* For handling misc devices */
>  #include <linux/module.h>	/* For module stuff/... */
>  #include <linux/mutex.h>	/* For mutexes */
> +#include <linux/reboot.h>	/* For reboot notifier */
>  #include <linux/slab.h>		/* For memory functions */
>  #include <linux/types.h>	/* For standard types (like size_t) */
>  #include <linux/watchdog.h>	/* For watchdog specific items */
> @@ -998,6 +999,25 @@ static struct class watchdog_class = {
>  	.dev_groups =	wdt_groups,
>  };
>
> +static int watchdog_reboot_notifier(struct notifier_block *nb,
> +				    unsigned long code, void *data)
> +{
> +	struct watchdog_device *wdd = container_of(nb, struct watchdog_device,
> +						   reboot_nb);
> +
> +	if (code == SYS_DOWN || code == SYS_HALT) {
> +		if (watchdog_active(wdd)) {
> +			int ret;
> +
> +			ret = wdd->ops->stop(wdd);
> +			if (ret)
> +				return NOTIFY_BAD;
> +		}
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>  /*
>   *	watchdog_dev_register: register a watchdog device
>   *	@wdd: watchdog device
> @@ -1031,6 +1051,18 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>  	if (ret) {
>  		device_destroy(&watchdog_class, devno);
>  		watchdog_cdev_unregister(wdd);
> +		return ret;
> +	}
> +
> +	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> +		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> +
> +		ret = devm_register_reboot_notifier(dev, &wdd->reboot_nb);
> +		if (ret) {
> +			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
> +			       wdd->id, ret);
> +			watchdog_dev_unregister(wdd);
> +		}
>  	}
>
>  	return ret;
>

Re: [PATCH v2] watchdog: core: Make use of devm_register_reboot_notifier()

[Andrey Smirnov]

On Sat, Apr 15, 2017 at 11:11 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 04/11/2017 09:06 AM, Andrey Smirnov wrote:
>>
>> Save a bit of cleanup code by leveraging newly added
>> devm_register_reboot_notifier().
>>
>> Cc: cphealy@gmail.com
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Wim Van Sebroeck <wim@iguana.be>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>
>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
>

Andrew,

This patch can't go in via watchdog tree because it depends on
devm_register_reboot_notifier() which is still present only in
linux-next, any change you can pull this into linux-next as well?

Thanks,
Andrey Smirnov


>
>> ---
>>
>> Guenter, Andy:
>>
>> Here's the second version of the patch originally submitted in
>> [v1]. This time I tried to follow Guenter's suggestion of moving this
>> bit on initialization in watchdog_dev_register().
>>
>> If I missed something obvious why this patch wouldn't work, again,
>> please let me know.
>>
>> Patch introducting devm_register_reboot_notifier() can be found in
>> [other-patch]
>>
>> Thanks,
>> Andrey Smirnov
>>
>> [v1] https://lkml.org/lkml/2017/4/4/388
>> [other-patch] https://lkml.org/lkml/2017/3/20/671
>>
>>
>>  drivers/watchdog/watchdog_core.c | 35 -----------------------------------
>>  drivers/watchdog/watchdog_dev.c  | 32 ++++++++++++++++++++++++++++++++
>>  2 files changed, 32 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_core.c
>> b/drivers/watchdog/watchdog_core.c
>> index 74265b2..8a8d952 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -137,25 +137,6 @@ int watchdog_init_timeout(struct watchdog_device
>> *wdd,
>>  }
>>  EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>>
>> -static int watchdog_reboot_notifier(struct notifier_block *nb,
>> -                                   unsigned long code, void *data)
>> -{
>> -       struct watchdog_device *wdd = container_of(nb, struct
>> watchdog_device,
>> -                                                  reboot_nb);
>> -
>> -       if (code == SYS_DOWN || code == SYS_HALT) {
>> -               if (watchdog_active(wdd)) {
>> -                       int ret;
>> -
>> -                       ret = wdd->ops->stop(wdd);
>> -                       if (ret)
>> -                               return NOTIFY_BAD;
>> -               }
>> -       }
>> -
>> -       return NOTIFY_DONE;
>> -}
>> -
>>  static int watchdog_restart_notifier(struct notifier_block *nb,
>>                                      unsigned long action, void *data)
>>  {
>> @@ -244,19 +225,6 @@ static int __watchdog_register_device(struct
>> watchdog_device *wdd)
>>                 }
>>         }
>>
>> -       if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
>> -               wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>> -
>> -               ret = register_reboot_notifier(&wdd->reboot_nb);
>> -               if (ret) {
>> -                       pr_err("watchdog%d: Cannot register reboot
>> notifier (%d)\n",
>> -                              wdd->id, ret);
>> -                       watchdog_dev_unregister(wdd);
>> -                       ida_simple_remove(&watchdog_ida, wdd->id);
>> -                       return ret;
>> -               }
>> -       }
>> -
>>         if (wdd->ops->restart) {
>>                 wdd->restart_nb.notifier_call = watchdog_restart_notifier;
>>
>> @@ -302,9 +270,6 @@ static void __watchdog_unregister_device(struct
>> watchdog_device *wdd)
>>         if (wdd->ops->restart)
>>                 unregister_restart_handler(&wdd->restart_nb);
>>
>> -       if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
>> -               unregister_reboot_notifier(&wdd->reboot_nb);
>> -
>>         watchdog_dev_unregister(wdd);
>>         ida_simple_remove(&watchdog_ida, wdd->id);
>>  }
>> diff --git a/drivers/watchdog/watchdog_dev.c
>> b/drivers/watchdog/watchdog_dev.c
>> index 32930a0..9a52990 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -42,6 +42,7 @@
>>  #include <linux/miscdevice.h>  /* For handling misc devices */
>>  #include <linux/module.h>      /* For module stuff/... */
>>  #include <linux/mutex.h>       /* For mutexes */
>> +#include <linux/reboot.h>      /* For reboot notifier */
>>  #include <linux/slab.h>                /* For memory functions */
>>  #include <linux/types.h>       /* For standard types (like size_t) */
>>  #include <linux/watchdog.h>    /* For watchdog specific items */
>> @@ -998,6 +999,25 @@ static struct class watchdog_class = {
>>         .dev_groups =   wdt_groups,
>>  };
>>
>> +static int watchdog_reboot_notifier(struct notifier_block *nb,
>> +                                   unsigned long code, void *data)
>> +{
>> +       struct watchdog_device *wdd = container_of(nb, struct
>> watchdog_device,
>> +                                                  reboot_nb);
>> +
>> +       if (code == SYS_DOWN || code == SYS_HALT) {
>> +               if (watchdog_active(wdd)) {
>> +                       int ret;
>> +
>> +                       ret = wdd->ops->stop(wdd);
>> +                       if (ret)
>> +                               return NOTIFY_BAD;
>> +               }
>> +       }
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>>  /*
>>   *     watchdog_dev_register: register a watchdog device
>>   *     @wdd: watchdog device
>> @@ -1031,6 +1051,18 @@ int watchdog_dev_register(struct watchdog_device
>> *wdd)
>>         if (ret) {
>>                 device_destroy(&watchdog_class, devno);
>>                 watchdog_cdev_unregister(wdd);
>> +               return ret;
>> +       }
>> +
>> +       if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
>> +               wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>> +
>> +               ret = devm_register_reboot_notifier(dev, &wdd->reboot_nb);
>> +               if (ret) {
>> +                       pr_err("watchdog%d: Cannot register reboot
>> notifier (%d)\n",
>> +                              wdd->id, ret);
>> +                       watchdog_dev_unregister(wdd);
>> +               }
>>         }
>>
>>         return ret;
>>
>

Re: [PATCH v2] watchdog: core: Make use of devm_register_reboot_notifier()

[Andrew Morton]

On Tue, 17 Oct 2017 08:50:12 -0700 Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> On Sat, Apr 15, 2017 at 11:11 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 04/11/2017 09:06 AM, Andrey Smirnov wrote:
> >>
> >> Save a bit of cleanup code by leveraging newly added
> >> devm_register_reboot_notifier().
> >>
> >> Cc: cphealy@gmail.com
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: Wim Van Sebroeck <wim@iguana.be>
> >> Cc: Guenter Roeck <linux@roeck-us.net>
> >> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >
> >
> > Acked-by: Guenter Roeck <linux@roeck-us.net>
> >
> 
> Andrew,
> 
> This patch can't go in via watchdog tree because it depends on
> devm_register_reboot_notifier() which is still present only in
> linux-next, any change you can pull this into linux-next as well?

That's because I've been sitting on
kernel-reboot-add-devm_register_reboot_notifier.patch since March
because it has no users.

This patch adds a user.  Have you identified other sites which
can/should use devm_register_reboot_notifier()?  I guess quite a lot,
so it's a matter of alerting developers to the new interface.  I
wonder how.  A checkpatch rule would do it, but that's new ground for
checkpatch.

Re: [PATCH v2] watchdog: core: Make use of devm_register_reboot_notifier()

[Andrey Smirnov]

On Tue, Oct 17, 2017 at 3:08 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 17 Oct 2017 08:50:12 -0700 Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
>> On Sat, Apr 15, 2017 at 11:11 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On 04/11/2017 09:06 AM, Andrey Smirnov wrote:
>> >>
>> >> Save a bit of cleanup code by leveraging newly added
>> >> devm_register_reboot_notifier().
>> >>
>> >> Cc: cphealy@gmail.com
>> >> Cc: linux-kernel@vger.kernel.org
>> >> Cc: Wim Van Sebroeck <wim@iguana.be>
>> >> Cc: Guenter Roeck <linux@roeck-us.net>
>> >> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>> >> Cc: Andrew Morton <akpm@linux-foundation.org>
>> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> >
>> >
>> > Acked-by: Guenter Roeck <linux@roeck-us.net>
>> >
>>
>> Andrew,
>>
>> This patch can't go in via watchdog tree because it depends on
>> devm_register_reboot_notifier() which is still present only in
>> linux-next, any change you can pull this into linux-next as well?
>
> That's because I've been sitting on
> kernel-reboot-add-devm_register_reboot_notifier.patch since March
> because it has no users.
>

Ah! I suspected that was the case.

> This patch adds a user.  Have you identified other sites which
> can/should use devm_register_reboot_notifier()?  I guess quite a lot,
> so it's a matter of alerting developers to the new interface.  I
> wonder how.  A checkpatch rule would do it, but that's new ground for
> checkpatch.
>

I can't say I looked for it in every nook and cranny, but I did look
at ~80% of the results that grepping for "register_reboot_notifier("
produces and it looked like lion's share of the code calling the
function didn't have a device to tie the registration lifespan to. For
dozen or so cases where there was a device to use, majority had the
call being done in the middle of probe/removal sequence, so it was
hard to tell if using devm_ version would be possible and if it would
give any significant benefits code reduction wise. However, there were
three places that looked like they had a pretty clear case for
conversion:

- drivers/input/misc/pm8941-pwrkey.c looked like a poster child for
the conversion, since the only thing it does in .remov is call
unregister_reboot_notifier()
- drivers/soc/lantiq/gphy.c and drivers/rtc/rtc-m41t80.c both looked
like they can be converted as well

Hope this is helpful.

Please, let me know if there's more action I should take about it.

Thanks,
Andrey Smirnov