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

[RFC PATCH v3 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
'.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).
build bot is happy. 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

Changes since V2:
	* keep the remove callback valid
	* remove 'if' from macro

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       | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.11.0

[RFC PATCH v3 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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 44985c4a1e86..c8ecbc53c807 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -216,4 +216,21 @@ 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) \
+{ \
+	__driver.driver.suppress_bind_attrs = !!(__nowayout); \
+	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 v3 1/4] watchdog: core: add mechanism to prevent removing if NOWAYOUT

[Guenter Roeck]

On Wed, Aug 29, 2018 at 09:42:38AM +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 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 44985c4a1e86..c8ecbc53c807 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -216,4 +216,21 @@ 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, ...) \

Another option might be to add another argument to this macro. Something like:

#define module_watchdog_driver(__wdriver, __driver, __register, __unregister, __nowayout, ...) \
static int __init __wdriver##_init(void) \
{ \
	__driver->suppress_bind_attrs = !!(__nowayout); \
	return __register(&(__wdriver)  ##__VA_ARGS__); \
} \
module_init(__wdriver##_init); \
static void __exit __wdriver##_exit(void) \
{ \
	__unregister(&(__wdriver), ##__VA_ARGS__); \
} \
module_exit(__wdriver##_exit)

#define module_watchdog_platform_driver(__platform_driver, __nowayout) \
	module_watchdog_driver(__platform_driver, &__platform_driver->driver, \
				platform_driver_register, \
				platform_driver_unregister, __nowayout)

This would avoid the dependency of having a ".driver" structure in each
supported bus driver structure.

Would that make sense ?

Guenter

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

[Jerry Hoemann]

On Wed, Aug 29, 2018 at 09:42:38AM +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 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 44985c4a1e86..c8ecbc53c807 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -216,4 +216,21 @@ 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) \
> +{ \
> +	__driver.driver.suppress_bind_attrs = !!(__nowayout); \
> +	return __register(&(__driver)  ##__VA_ARGS__); \
> +} \

Sorry, I'm not familiar w/ this level of detail of the driver interface.

Is the intent of the proposed changes to prevent the unload of
a module if it was loaded with the "nowayout" parameter?

If so, I thought the WD "nowayout" semantic was only supposed to be in effect
after /dev/watchdog was opened.  The proposed change looks like it makes the
"nowayout" semantic take effect before /dev/watchdog is opened.


Thanks


> +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

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

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

[Geert Uytterhoeven]

Hi Jerry,

On Wed, Aug 29, 2018 at 10:55 PM Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Wed, Aug 29, 2018 at 09:42:38AM +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 | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> > index 44985c4a1e86..c8ecbc53c807 100644
> > --- a/include/linux/watchdog.h
> > +++ b/include/linux/watchdog.h
> > @@ -216,4 +216,21 @@ 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) \
> > +{ \
> > +     __driver.driver.suppress_bind_attrs = !!(__nowayout); \
> > +     return __register(&(__driver)  ##__VA_ARGS__); \
> > +} \
>
> Sorry, I'm not familiar w/ this level of detail of the driver interface.
>
> Is the intent of the proposed changes to prevent the unload of
> a module if it was loaded with the "nowayout" parameter?
>
> If so, I thought the WD "nowayout" semantic was only supposed to be in effect
> after /dev/watchdog was opened.  The proposed change looks like it makes the
> "nowayout" semantic take effect before /dev/watchdog is opened.

Thanks, that's IMHO a very valid point.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Wolfram Sang]

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


> > Is the intent of the proposed changes to prevent the unload of
> > a module if it was loaded with the "nowayout" parameter?
> >
> > If so, I thought the WD "nowayout" semantic was only supposed to be in effect
> > after /dev/watchdog was opened.  The proposed change looks like it makes the
> > "nowayout" semantic take effect before /dev/watchdog is opened.
> 
> Thanks, that's IMHO a very valid point.

I agree, thanks for this input.


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

[RFC PATCH v3 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 v3 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 c8ecbc53c807..7f097d6f94d4 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -233,4 +233,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 v3 4/4] watchdog: i6300esb: 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/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