[RFC PATCH v2 0/4] watchdog: prevent removing a driver if NOWAYOUT

[RFC PATCH v2 0/4] watchdog: prevent removing a driver if NOWAYOUT

[Wolfram Sang]

So, here is my second approach, now avoiding probe() and targetting the init
call. To avoid boilerplate, I introduced macros similar to module_driver(). It
still feels a little adventurous because of hard-coding '.remove' and
'.driver.suppress_bind_attts' in the macro and trusting various driver types
(like platform and PCI) to follow this structure.

Having all this said, it works nicely on my Renesas Salvator-XS (R-Car M3-N).
No reply from buildbot yet, but it is RFC only, so I'll send it out already. A
git branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/wdt-suppress-attr

Looking forward to comments.

Thanks,

   Wolfram

Wolfram Sang (4):
  watchdog: core: add mechanism to prevent removing if NOWAYOUT
  watchdog: renesas_wdt: avoid removing if NOWAYOUT
  watchdog: core: add module_watchdog_pci_driver()
  watchdog: i6300esb: avoid removing if NOWAYOUT

 drivers/watchdog/i6300esb.c    |  2 +-
 drivers/watchdog/renesas_wdt.c |  2 +-
 include/linux/watchdog.h       | 24 ++++++++++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

-- 
2.11.0

[RFC PATCH v2 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Wolfram Sang]

To prevent removing if NOWAYOUT, we invalidate the .remove function and
suppress the bind/unbind attributes in sysfs. These are driver
capabilities, so we need to set it up at runtime during init. To avoid
boilerplate, introduce module_watchdog_driver() similar to
module_driver(). On top of that, we then build
module_watchdog_platform_driver(). Others may follow, if needed.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/watchdog.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 44985c4a1e86..5768fb6b5cde 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -216,4 +216,24 @@ extern void watchdog_unregister_device(struct watchdog_device *);
 /* devres register variant */
 int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);
 
+#define module_watchdog_driver(__driver, __register, __unregister, __nowayout, ...) \
+static int __init __driver##_init(void) \
+{ \
+	if (__nowayout) { \
+		__driver.driver.suppress_bind_attrs = true; \
+		__driver.remove = NULL; \
+	} \
+	return __register(&(__driver)  ##__VA_ARGS__); \
+} \
+module_init(__driver##_init); \
+static void __exit __driver##_exit(void) \
+{ \
+	__unregister(&(__driver), ##__VA_ARGS__); \
+} \
+module_exit(__driver##_exit)
+
+#define module_watchdog_platform_driver(__platform_driver, __nowayout) \
+	module_watchdog_driver(__platform_driver, platform_driver_register, \
+				platform_driver_unregister, __nowayout)
+
 #endif  /* ifndef _LINUX_WATCHDOG_H */
-- 
2.11.0

Re: [RFC PATCH v2 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Guenter Roeck]

On Tue, Aug 28, 2018 at 09:14:13PM +0200, Wolfram Sang wrote:
> To prevent removing if NOWAYOUT, we invalidate the .remove function and
> suppress the bind/unbind attributes in sysfs. These are driver
> capabilities, so we need to set it up at runtime during init. To avoid
> boilerplate, introduce module_watchdog_driver() similar to
> module_driver(). On top of that, we then build
> module_watchdog_platform_driver(). Others may follow, if needed.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  include/linux/watchdog.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 44985c4a1e86..5768fb6b5cde 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -216,4 +216,24 @@ extern void watchdog_unregister_device(struct watchdog_device *);
>  /* devres register variant */
>  int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);
>  
> +#define module_watchdog_driver(__driver, __register, __unregister, __nowayout, ...) \
> +static int __init __driver##_init(void) \
> +{ \
> +	if (__nowayout) { \
> +		__driver.driver.suppress_bind_attrs = true; \
> +		__driver.remove = NULL; \

Does that really do any good ? If I understand correctly, the only
impact is that the platform driver remove function will believe that
nothing needs to be done on removal. See platform_drv_remove().

> +	} \
> +	return __register(&(__driver)  ##__VA_ARGS__); \
> +} \
> +module_init(__driver##_init); \
> +static void __exit __driver##_exit(void) \
> +{ \
> +	__unregister(&(__driver), ##__VA_ARGS__); \
> +} \
> +module_exit(__driver##_exit)
> +
> +#define module_watchdog_platform_driver(__platform_driver, __nowayout) \
> +	module_watchdog_driver(__platform_driver, platform_driver_register, \
> +				platform_driver_unregister, __nowayout)
> +
>  #endif  /* ifndef _LINUX_WATCHDOG_H */
> -- 
> 2.11.0
> 

Re: [RFC PATCH v2 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

Hi Guenter,

> > +		__driver.remove = NULL; \
> 
> Does that really do any good ? If I understand correctly, the only
> impact is that the platform driver remove function will believe that
> nothing needs to be done on removal. See platform_drv_remove().

This might be biased for my use case. Not calling remove will also leave
the clock for that module enabled. But true, the platform device will be
removed and without proper cleanup, there could be dangling pointers.

In general, what do you think of this approach if we'd left out the
above line?

Thanks,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v2 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Guenter Roeck]

On Tue, Aug 28, 2018 at 10:07:40PM +0200, Wolfram Sang wrote:
> Hi Guenter,
> 
> > > +		__driver.remove = NULL; \
> > 
> > Does that really do any good ? If I understand correctly, the only
> > impact is that the platform driver remove function will believe that
> > nothing needs to be done on removal. See platform_drv_remove().
> 
> This might be biased for my use case. Not calling remove will also leave
> the clock for that module enabled. But true, the platform device will be
> removed and without proper cleanup, there could be dangling pointers.
> 

For many drivers the watchdog device would not be removed. That would
indeed be fatal.

> In general, what do you think of this approach if we'd left out the
> above line?
> 

LGTM without that line.

Thanks,
Guenter

[RFC PATCH v2 2/4] watchdog: renesas_wdt: avoid removing if NOWAYOUT

[Wolfram Sang]

Use the new macro to prevent removing the driver if NOWAYOUT.

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

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index f45cb183fa75..92339c744cce 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -302,7 +302,7 @@ static struct platform_driver rwdt_driver = {
 	.probe = rwdt_probe,
 	.remove = rwdt_remove,
 };
-module_platform_driver(rwdt_driver);
+module_watchdog_platform_driver(rwdt_driver, nowayout);
 
 MODULE_DESCRIPTION("Renesas WDT Watchdog Driver");
 MODULE_LICENSE("GPL v2");
-- 
2.11.0

[RFC PATCH v2 3/4] watchdog: core: add module_watchdog_pci_driver()

[Wolfram Sang]

Allow PCI drivers to prevent removing if NOWAYOUT, too.

Note: This is only a build-tested proof-of-concept!

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/watchdog.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 5768fb6b5cde..07c01e8050e8 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -236,4 +236,8 @@ module_exit(__driver##_exit)
 	module_watchdog_driver(__platform_driver, platform_driver_register, \
 				platform_driver_unregister, __nowayout)
 
+#define module_watchdog_pci_driver(__pci_driver, __nowayout) \
+	module_watchdog_driver(__pci_driver, pci_register_driver, \
+				pci_unregister_driver, __nowayout)
+
 #endif  /* ifndef _LINUX_WATCHDOG_H */
-- 
2.11.0

[RFC PATCH v2 4/4] watchdog: i6300esb: avoid removing if NOWAYOUT

[Wolfram Sang]

Use the new macro to prevent removing the driver if NOWAYOUT.

Note: This is only a build-tested proof-of-concept!

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

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index 950c71a8bb22..939224789e2a 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -356,7 +356,7 @@ static struct pci_driver esb_driver = {
 	.remove         = esb_remove,
 };
 
-module_pci_driver(esb_driver);
+module_watchdog_pci_driver(esb_driver, nowayout);
 
 MODULE_AUTHOR("Ross Biro and David Härdeman");
 MODULE_DESCRIPTION("Watchdog driver for Intel 6300ESB chipsets");
-- 
2.11.0