Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] wdt: clean up unused modular infrastructure
@ 2019-04-23 15:48 Paul Gortmaker
  2019-04-23 15:48 ` [PATCH 1/5] watchdog: rtd119x: drop unused module.h include Paul Gortmaker
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-23 15:48 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, Paul Gortmaker, Benjamin Fair, Linus Walleij,
	Nancy Yuen, openbmc, Patrick Venture, Tali Perry, Tomer Maimon,
	Wim Van Sebroeck

People can embed modular includes and modular exit functions into code
that never use any of it, and they won't get any errors or warnings.

Using modular infrastructure in non-modules might seem harmless, but some
of the downfalls this leads to are:

 (1) it is easy to accidentally write unused module_exit/remove code
 (2) it can be misleading when reading the source, thinking a driver can
     be modular when the Makefile and/or Kconfig prohibit it
 (3) an unused include of the module.h header file will in turn
     include nearly everything else; adding a lot to CPP overhead.
 (4) it gets copied/replicated into other drivers and can spread.

As a data point for #3 above, an empty C file that just includes the
module.h header generates over 750kB of CPP output.  Repeating the same
experiment with init.h and the result is less than 12kB; with export.h
it is only about 1/2kB; with both it still is less than 12kB.

Here, In this series, we do what has been done for other subsystems,
like, net, x86, mfd, iommu....  and audit for uses of modular
infrastructure inside code that currently can't be built as a module.

As always, the option exists for driver authors to convert their code
to tristate, if there is a valid use case for it to be so.  But since
I don't have the context for each driver to know if such a use case
exists, I limit myself to simply removing the unused code in order to
make the driver consistent with the Makefile/Kconfig settings that
control it.

Paul.

---

Cc: Benjamin Fair <benjaminfair@google.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-watchdog@vger.kernel.org
Cc: Nancy Yuen <yuenn@google.com>
Cc: openbmc@lists.ozlabs.org
Cc: Patrick Venture <venture@google.com>
Cc: Tali Perry <tali.perry1@gmail.com>
Cc: Tomer Maimon <tmaimon77@gmail.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>


Paul Gortmaker (5):
  watchdog: rtd119x: drop unused module.h include
  watchdog: watchdog_core: make it explicitly non-modular
  watchdog: npcm: make it explicitly non-modular
  watchdog: intel_scu: make it explicitly non-modular
  watchdog: coh901327: make it explicitly non-modular

 drivers/watchdog/coh901327_wdt.c      | 24 ++++--------------------
 drivers/watchdog/intel_scu_watchdog.c | 18 ------------------
 drivers/watchdog/npcm_wdt.c           | 13 ++++++-------
 drivers/watchdog/rtd119x_wdt.c        |  1 -
 drivers/watchdog/watchdog_core.c      | 15 +--------------
 5 files changed, 11 insertions(+), 60 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/5] watchdog: rtd119x: drop unused module.h include
  2019-04-23 15:48 [PATCH 0/5] wdt: clean up unused modular infrastructure Paul Gortmaker
@ 2019-04-23 15:48 ` Paul Gortmaker
  2019-04-29 16:38   ` Guenter Roeck
  2019-04-23 15:48 ` [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular Paul Gortmaker
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-23 15:48 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck; +Cc: linux-watchdog, Paul Gortmaker

The Kconfig for this driver is:

config RTD119X_WATCHDOG
       bool "Realtek RTD118x/RTD129x watchdog support"

...and hence it doesn't need to include module.h for anything.
There are no other signs of unused modular infrastructure.

Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-watchdog@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/watchdog/rtd119x_wdt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/watchdog/rtd119x_wdt.c b/drivers/watchdog/rtd119x_wdt.c
index d001c17ddfde..a0787a50ac6d 100644
--- a/drivers/watchdog/rtd119x_wdt.c
+++ b/drivers/watchdog/rtd119x_wdt.c
@@ -9,7 +9,6 @@
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
-- 
2.7.4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular
  2019-04-23 15:48 [PATCH 0/5] wdt: clean up unused modular infrastructure Paul Gortmaker
  2019-04-23 15:48 ` [PATCH 1/5] watchdog: rtd119x: drop unused module.h include Paul Gortmaker
@ 2019-04-23 15:48 ` Paul Gortmaker
  2019-04-24  1:22   ` Guenter Roeck
  2019-04-23 15:48 ` [PATCH 3/5] watchdog: npcm: " Paul Gortmaker
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-23 15:48 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, Paul Gortmaker, Wim Van Sebroeck, Alan Cox

The Kconfig currently controlling compilation of this code is:

config WATCHDOG_CORE
       bool "WatchDog Timer Driver Core"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

We replace module.h with export.h since the file does export some
symbols.  We don't add init.h since the file already has that.

We also delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.

Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: linux-watchdog@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/watchdog/watchdog_core.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index eb8fa25f8eb2..f9f88f59d181 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -28,7 +28,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/module.h>	/* For EXPORT_SYMBOL/module stuff/... */
+#include <linux/export.h>	/* For EXPORT_SYMBOL stuff */
 #include <linux/types.h>	/* For standard types */
 #include <linux/errno.h>	/* For the -ENODEV/... values */
 #include <linux/kernel.h>	/* For printk/panic/... */
@@ -359,17 +359,4 @@ static int __init watchdog_init(void)
 	watchdog_deferred_registration();
 	return 0;
 }
-
-static void __exit watchdog_exit(void)
-{
-	watchdog_dev_exit();
-	ida_destroy(&watchdog_ida);
-}
-
 subsys_initcall_sync(watchdog_init);
-module_exit(watchdog_exit);
-
-MODULE_AUTHOR("Alan Cox <alan@lxorguk.ukuu.org.uk>");
-MODULE_AUTHOR("Wim Van Sebroeck <wim@iguana.be>");
-MODULE_DESCRIPTION("WatchDog Timer Driver Core");
-MODULE_LICENSE("GPL");
-- 
2.7.4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 3/5] watchdog: npcm: make it explicitly non-modular
  2019-04-23 15:48 [PATCH 0/5] wdt: clean up unused modular infrastructure Paul Gortmaker
  2019-04-23 15:48 ` [PATCH 1/5] watchdog: rtd119x: drop unused module.h include Paul Gortmaker
  2019-04-23 15:48 ` [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular Paul Gortmaker
@ 2019-04-23 15:48 ` " Paul Gortmaker
  2019-04-29 16:40   ` Guenter Roeck
  2019-04-23 15:48 ` [PATCH 4/5] watchdog: intel_scu: " Paul Gortmaker
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-23 15:48 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, Paul Gortmaker, Avi Fishman, Tomer Maimon,
	Tali Perry, Patrick Venture, Nancy Yuen, Benjamin Fair, openbmc

The Kconfig currently controlling compilation of this code is:

config NPCM7XX_WATCHDOG
       bool "Nuvoton NPCM750 watchdog"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

Since module_platform_driver() uses the same init level priority as
builtin_platform_driver() the init ordering remains unchanged with
this commit.

Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.

We also delete the MODULE_LICENSE tag etc. since all that information
was (or is now) contained at the top of the file in the comments.

Cc: Avi Fishman <avifishman70@gmail.com>
Cc: Tomer Maimon <tmaimon77@gmail.com>
Cc: Tali Perry <tali.perry1@gmail.com>
Cc: Patrick Venture <venture@google.com>
Cc: Nancy Yuen <yuenn@google.com>
Cc: Benjamin Fair <benjaminfair@google.com>
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: openbmc@lists.ozlabs.org
Cc: linux-watchdog@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/watchdog/npcm_wdt.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
index 0d4213652ecc..44d305683ab6 100644
--- a/drivers/watchdog/npcm_wdt.c
+++ b/drivers/watchdog/npcm_wdt.c
@@ -2,11 +2,15 @@
 // Copyright (c) 2018 Nuvoton Technology corporation.
 // Copyright (c) 2018 IBM Corp.
 
+/*
+ * Watchdog driver for NPCM
+ * Author: Joel Stanley
+ */
+
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -237,7 +241,6 @@ static const struct of_device_id npcm_wdt_match[] = {
 	{.compatible = "nuvoton,npcm750-wdt"},
 	{},
 };
-MODULE_DEVICE_TABLE(of, npcm_wdt_match);
 #endif
 
 static struct platform_driver npcm_wdt_driver = {
@@ -247,8 +250,4 @@ static struct platform_driver npcm_wdt_driver = {
 		.of_match_table = of_match_ptr(npcm_wdt_match),
 	},
 };
-module_platform_driver(npcm_wdt_driver);
-
-MODULE_AUTHOR("Joel Stanley");
-MODULE_DESCRIPTION("Watchdog driver for NPCM");
-MODULE_LICENSE("GPL v2");
+builtin_platform_driver(npcm_wdt_driver);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 4/5] watchdog: intel_scu: make it explicitly non-modular
  2019-04-23 15:48 [PATCH 0/5] wdt: clean up unused modular infrastructure Paul Gortmaker
                   ` (2 preceding siblings ...)
  2019-04-23 15:48 ` [PATCH 3/5] watchdog: npcm: " Paul Gortmaker
@ 2019-04-23 15:48 ` " Paul Gortmaker
  2019-04-29 16:37   ` Guenter Roeck
  2019-04-23 15:48 ` [PATCH 5/5] watchdog: coh901327: " Paul Gortmaker
  2019-05-12 15:43 ` [PATCH 0/5] wdt: clean up unused modular infrastructure Avi Fishman
  5 siblings, 1 reply; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-23 15:48 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, Paul Gortmaker, Wim Van Sebroeck

The Kconfig currently controlling compilation of this code is:

config INTEL_SCU_WATCHDOG
       bool "Intel SCU Watchdog for Mobile Platforms"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

We also delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.

We leave the moduleparam.h include since the file does declare some
module parameters, and leaving them as such is currently the easiest
way to remain compatible with existing boot arg use cases.

Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-watchdog@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/watchdog/intel_scu_watchdog.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/watchdog/intel_scu_watchdog.c b/drivers/watchdog/intel_scu_watchdog.c
index 0caab6241eb7..32bc5611f0cc 100644
--- a/drivers/watchdog/intel_scu_watchdog.c
+++ b/drivers/watchdog/intel_scu_watchdog.c
@@ -25,7 +25,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/compiler.h>
-#include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
 #include <linux/types.h>
@@ -545,21 +544,4 @@ static int __init intel_scu_watchdog_init(void)
 	iounmap(watchdog_device.timer_load_count_addr);
 	return ret;
 }
-
-static void __exit intel_scu_watchdog_exit(void)
-{
-
-	misc_deregister(&watchdog_device.miscdev);
-	unregister_reboot_notifier(&watchdog_device.intel_scu_notifier);
-	/* disable the timer */
-	iowrite32(0x00000002, watchdog_device.timer_control_addr);
-	iounmap(watchdog_device.timer_load_count_addr);
-}
-
 late_initcall(intel_scu_watchdog_init);
-module_exit(intel_scu_watchdog_exit);
-
-MODULE_AUTHOR("Intel Corporation");
-MODULE_DESCRIPTION("Intel SCU Watchdog Device Driver");
-MODULE_LICENSE("GPL");
-MODULE_VERSION(WDT_VER);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 5/5] watchdog: coh901327: make it explicitly non-modular
  2019-04-23 15:48 [PATCH 0/5] wdt: clean up unused modular infrastructure Paul Gortmaker
                   ` (3 preceding siblings ...)
  2019-04-23 15:48 ` [PATCH 4/5] watchdog: intel_scu: " Paul Gortmaker
@ 2019-04-23 15:48 ` " Paul Gortmaker
  2019-04-23 21:29   ` Linus Walleij
  2019-04-29 16:37   ` Guenter Roeck
  2019-05-12 15:43 ` [PATCH 0/5] wdt: clean up unused modular infrastructure Avi Fishman
  5 siblings, 2 replies; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-23 15:48 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, Paul Gortmaker, Linus Walleij, Wim Van Sebroeck

The Kconfig currently controlling compilation of this code is:

config COH901327_WATCHDOG
       bool "ST-Ericsson COH 901 327 watchdog"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

We explicitly disallow a driver unbind, since that doesn't have a
sensible use case anyway, and it allows us to drop the ".remove"
code for non-modular drivers.

Since module_platform_driver() uses the same init level priority as
builtin_platform_driver() the init ordering remains unchanged with
this commit.

Also note that MODULE_ALIAS is a no-op for non-modular code.

We also delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.

We replace module.h with moduleparam.h since the file does actually
declare some module parameters (i.e. boot args for non-modules).

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-watchdog@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/watchdog/coh901327_wdt.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
index f29d1edc5bad..5914cc767c87 100644
--- a/drivers/watchdog/coh901327_wdt.c
+++ b/drivers/watchdog/coh901327_wdt.c
@@ -6,7 +6,7 @@
  * Watchdog driver for the ST-Ericsson AB COH 901 327 IP core
  * Author: Linus Walleij <linus.walleij@stericsson.com>
  */
-#include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/mod_devicetable.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
@@ -243,16 +243,6 @@ static struct watchdog_device coh901327_wdt = {
 	.timeout = U300_WDOG_DEFAULT_TIMEOUT,
 };
 
-static int __exit coh901327_remove(struct platform_device *pdev)
-{
-	watchdog_unregister_device(&coh901327_wdt);
-	coh901327_disable();
-	free_irq(irq, pdev);
-	clk_disable_unprepare(clk);
-	clk_put(clk);
-	return 0;
-}
-
 static int __init coh901327_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -408,19 +398,13 @@ static struct platform_driver coh901327_driver = {
 	.driver = {
 		.name	= "coh901327_wdog",
 		.of_match_table = coh901327_dt_match,
+		.suppress_bind_attrs = true,
 	},
-	.remove		= __exit_p(coh901327_remove),
 	.suspend	= coh901327_suspend,
 	.resume		= coh901327_resume,
 };
+builtin_platform_driver_probe(coh901327_driver, coh901327_probe);
 
-module_platform_driver_probe(coh901327_driver, coh901327_probe);
-
-MODULE_AUTHOR("Linus Walleij <linus.walleij@stericsson.com>");
-MODULE_DESCRIPTION("COH 901 327 Watchdog");
-
+/* not really modular, but ... */
 module_param(margin, uint, 0);
 MODULE_PARM_DESC(margin, "Watchdog margin in seconds (default 60s)");
-
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:coh901327-watchdog");
-- 
2.7.4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 5/5] watchdog: coh901327: make it explicitly non-modular
  2019-04-23 15:48 ` [PATCH 5/5] watchdog: coh901327: " Paul Gortmaker
@ 2019-04-23 21:29   ` Linus Walleij
  2019-04-29 16:37   ` Guenter Roeck
  1 sibling, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2019-04-23 21:29 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: Wim Van Sebroeck, Guenter Roeck, LINUXWATCHDOG, Wim Van Sebroeck

On Tue, Apr 23, 2019 at 5:50 PM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:

> The Kconfig currently controlling compilation of this code is:
>
> config COH901327_WATCHDOG
>        bool "ST-Ericsson COH 901 327 watchdog"
>
> ...meaning that it currently is not being built as a module by anyone.
>
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
>
> We explicitly disallow a driver unbind, since that doesn't have a
> sensible use case anyway, and it allows us to drop the ".remove"
> code for non-modular drivers.
>
> Since module_platform_driver() uses the same init level priority as
> builtin_platform_driver() the init ordering remains unchanged with
> this commit.
>
> Also note that MODULE_ALIAS is a no-op for non-modular code.
>
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
>
> We replace module.h with moduleparam.h since the file does actually
> declare some module parameters (i.e. boot args for non-modules).
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-watchdog@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for doing this Paul.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular
  2019-04-23 15:48 ` [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular Paul Gortmaker
@ 2019-04-24  1:22   ` Guenter Roeck
  2019-04-24 15:37     ` Paul Gortmaker
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2019-04-24  1:22 UTC (permalink / raw)
  To: Paul Gortmaker, Wim Van Sebroeck
  Cc: linux-watchdog, Wim Van Sebroeck, Alan Cox

On 4/23/19 8:48 AM, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
> 
> config WATCHDOG_CORE
>         bool "WatchDog Timer Driver Core"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
> 
> We replace module.h with export.h since the file does export some
> symbols.  We don't add init.h since the file already has that.
> 
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
> 

I must admit that I am not at all happy about this change. While not
configurable by default, I used tristate a lot (after enabling it
manually) to test watchdog core code while changing it. It saves a
lot of time to be able to reload the watchdog core without having
to reboot the entire system after each change. Removing the ability
to do that just because it is not enabled in the field and just
to save a few lines of code (and because having modules seems to
have come out of favor lately) does not make sense to me.

I won't NACK the series outright, but I'll leave it up to Wim as
the senior maintainer to decide what he wants to do with it.

Guenter

> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: linux-watchdog@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>   drivers/watchdog/watchdog_core.c | 15 +--------------
>   1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index eb8fa25f8eb2..f9f88f59d181 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -28,7 +28,7 @@
>   
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
> -#include <linux/module.h>	/* For EXPORT_SYMBOL/module stuff/... */
> +#include <linux/export.h>	/* For EXPORT_SYMBOL stuff */
>   #include <linux/types.h>	/* For standard types */
>   #include <linux/errno.h>	/* For the -ENODEV/... values */
>   #include <linux/kernel.h>	/* For printk/panic/... */
> @@ -359,17 +359,4 @@ static int __init watchdog_init(void)
>   	watchdog_deferred_registration();
>   	return 0;
>   }
> -
> -static void __exit watchdog_exit(void)
> -{
> -	watchdog_dev_exit();
> -	ida_destroy(&watchdog_ida);
> -}
> -
>   subsys_initcall_sync(watchdog_init);
> -module_exit(watchdog_exit);
> -
> -MODULE_AUTHOR("Alan Cox <alan@lxorguk.ukuu.org.uk>");
> -MODULE_AUTHOR("Wim Van Sebroeck <wim@iguana.be>");
> -MODULE_DESCRIPTION("WatchDog Timer Driver Core");
> -MODULE_LICENSE("GPL");
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular
  2019-04-24  1:22   ` Guenter Roeck
@ 2019-04-24 15:37     ` Paul Gortmaker
  2019-04-24 21:05       ` Guenter Roeck
  0 siblings, 1 reply; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-24 15:37 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wim Van Sebroeck, linux-watchdog, Wim Van Sebroeck, Alan Cox

[Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular] On 23/04/2019 (Tue 18:22) Guenter Roeck wrote:

> On 4/23/19 8:48 AM, Paul Gortmaker wrote:
> >The Kconfig currently controlling compilation of this code is:
> >
> >config WATCHDOG_CORE
> >        bool "WatchDog Timer Driver Core"
> >
> >...meaning that it currently is not being built as a module by anyone.
> >
> >Lets remove the modular code that is essentially orphaned, so that
> >when reading the driver there is no doubt it is builtin-only.
> >
> >We replace module.h with export.h since the file does export some
> >symbols.  We don't add init.h since the file already has that.
> >
> >We also delete the MODULE_LICENSE tag etc. since all that information
> >is already contained at the top of the file in the comments.
> >
> 
> I must admit that I am not at all happy about this change. While not
> configurable by default, I used tristate a lot (after enabling it
> manually) to test watchdog core code while changing it. It saves a
> lot of time to be able to reload the watchdog core without having
> to reboot the entire system after each change. Removing the ability

I'm confused.  If it is useful, then why not formally make it tristate
so other people can do the same as you do, and nobody is manually making
the change over and over again each time?  I'd support that update.

> to do that just because it is not enabled in the field and just
> to save a few lines of code (and because having modules seems to
> have come out of favor lately) does not make sense to me.

I'd have to say this is a mischaracterization.  Modules are not out of
favour.  A disconnect between the code and Kconfig is out of favour.

Of all the hundred or so(?) of these type patches that have been merged
so far, I have not created a single change with the intent of reduction
in the existing out-of-box mainline support of drivers as modules.

Rather, It is in fact the opposite.  As I said in the 0/5 preamble:

    As always, the option exists for driver authors to convert their
    code to tristate...

...and a lot of drivers are now tristate because the author simply
didn't realize they'd chosen "bool".  We managed to make a new uart
driver get bool ---> tristate conversion just this week, for example.

https://marc.info/?l=linux-serial&m=155602656610079&w=2

> I won't NACK the series outright, but I'll leave it up to Wim as
> the senior maintainer to decide what he wants to do with it.

Yes, the decision is entirely up to you guys, but I just wanted to
clarify once again that this or any one of the other similar changes
are in no way some kind of "attack on modules". Quite the opposite
as you can see in the above thread.

Thanks,
Paul.
--

> 
> Guenter
> 
> >Cc: Wim Van Sebroeck <wim@iguana.be>
> >Cc: Guenter Roeck <linux@roeck-us.net>
> >Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> >Cc: linux-watchdog@vger.kernel.org
> >Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> >---
> >  drivers/watchdog/watchdog_core.c | 15 +--------------
> >  1 file changed, 1 insertion(+), 14 deletions(-)
> >
> >diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> >index eb8fa25f8eb2..f9f88f59d181 100644
> >--- a/drivers/watchdog/watchdog_core.c
> >+++ b/drivers/watchdog/watchdog_core.c
> >@@ -28,7 +28,7 @@
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >-#include <linux/module.h>	/* For EXPORT_SYMBOL/module stuff/... */
> >+#include <linux/export.h>	/* For EXPORT_SYMBOL stuff */
> >  #include <linux/types.h>	/* For standard types */
> >  #include <linux/errno.h>	/* For the -ENODEV/... values */
> >  #include <linux/kernel.h>	/* For printk/panic/... */
> >@@ -359,17 +359,4 @@ static int __init watchdog_init(void)
> >  	watchdog_deferred_registration();
> >  	return 0;
> >  }
> >-
> >-static void __exit watchdog_exit(void)
> >-{
> >-	watchdog_dev_exit();
> >-	ida_destroy(&watchdog_ida);
> >-}
> >-
> >  subsys_initcall_sync(watchdog_init);
> >-module_exit(watchdog_exit);
> >-
> >-MODULE_AUTHOR("Alan Cox <alan@lxorguk.ukuu.org.uk>");
> >-MODULE_AUTHOR("Wim Van Sebroeck <wim@iguana.be>");
> >-MODULE_DESCRIPTION("WatchDog Timer Driver Core");
> >-MODULE_LICENSE("GPL");
> >
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular
  2019-04-24 15:37     ` Paul Gortmaker
@ 2019-04-24 21:05       ` Guenter Roeck
  2019-04-27  9:48         ` Wim Van Sebroeck
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2019-04-24 21:05 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: Wim Van Sebroeck, linux-watchdog, Wim Van Sebroeck, Alan Cox

On Wed, Apr 24, 2019 at 11:37:00AM -0400, Paul Gortmaker wrote:
> [Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular] On 23/04/2019 (Tue 18:22) Guenter Roeck wrote:
> 
> > On 4/23/19 8:48 AM, Paul Gortmaker wrote:
> > >The Kconfig currently controlling compilation of this code is:
> > >
> > >config WATCHDOG_CORE
> > >        bool "WatchDog Timer Driver Core"
> > >
> > >...meaning that it currently is not being built as a module by anyone.
> > >
> > >Lets remove the modular code that is essentially orphaned, so that
> > >when reading the driver there is no doubt it is builtin-only.
> > >
> > >We replace module.h with export.h since the file does export some
> > >symbols.  We don't add init.h since the file already has that.
> > >
> > >We also delete the MODULE_LICENSE tag etc. since all that information
> > >is already contained at the top of the file in the comments.
> > >
> > 
> > I must admit that I am not at all happy about this change. While not
> > configurable by default, I used tristate a lot (after enabling it
> > manually) to test watchdog core code while changing it. It saves a
> > lot of time to be able to reload the watchdog core without having
> > to reboot the entire system after each change. Removing the ability
> 
> I'm confused.  If it is useful, then why not formally make it tristate
> so other people can do the same as you do, and nobody is manually making
> the change over and over again each time?  I'd support that update.
> 
No idea. That precedes my involvement in the watchdog subsystem.
Let's wait for input from Wim. I have a set of patches ready, but it
doesn't make sense to me to submit them if Wim wants to go the non-modular
route.

FWIW, I am fine with the other patches except for the npcm patch, because
several of the other npcm drivers are buildable as module.

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular
  2019-04-24 21:05       ` Guenter Roeck
@ 2019-04-27  9:48         ` Wim Van Sebroeck
  2019-04-29 16:28           ` Guenter Roeck
  0 siblings, 1 reply; 18+ messages in thread
From: Wim Van Sebroeck @ 2019-04-27  9:48 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Paul Gortmaker, Wim Van Sebroeck, linux-watchdog,
	Wim Van Sebroeck, Alan Cox

Hi All,

> On Wed, Apr 24, 2019 at 11:37:00AM -0400, Paul Gortmaker wrote:
> > [Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular] On 23/04/2019 (Tue 18:22) Guenter Roeck wrote:
> > 
> > > On 4/23/19 8:48 AM, Paul Gortmaker wrote:
> > > >The Kconfig currently controlling compilation of this code is:
> > > >
> > > >config WATCHDOG_CORE
> > > >        bool "WatchDog Timer Driver Core"
> > > >
> > > >...meaning that it currently is not being built as a module by anyone.
> > > >
> > > >Lets remove the modular code that is essentially orphaned, so that
> > > >when reading the driver there is no doubt it is builtin-only.
> > > >
> > > >We replace module.h with export.h since the file does export some
> > > >symbols.  We don't add init.h since the file already has that.
> > > >
> > > >We also delete the MODULE_LICENSE tag etc. since all that information
> > > >is already contained at the top of the file in the comments.
> > > >
> > > 
> > > I must admit that I am not at all happy about this change. While not
> > > configurable by default, I used tristate a lot (after enabling it
> > > manually) to test watchdog core code while changing it. It saves a
> > > lot of time to be able to reload the watchdog core without having
> > > to reboot the entire system after each change. Removing the ability
> > 
> > I'm confused.  If it is useful, then why not formally make it tristate
> > so other people can do the same as you do, and nobody is manually making
> > the change over and over again each time?  I'd support that update.
> > 
> No idea. That precedes my involvement in the watchdog subsystem.
> Let's wait for input from Wim. I have a set of patches ready, but it
> doesn't make sense to me to submit them if Wim wants to go the non-modular
> route.
> 
> FWIW, I am fine with the other patches except for the npcm patch, because
> several of the other npcm drivers are buildable as module.

In general: If systems/devices can't handle modules then we should indeed make sure that we clean it up.

For the watchdog core however, I am not in favour of doing that. I also use it as a module when i'm testing.
I originally wanted to make it tristate (so that it can be loaded as a module), but I didn't had a clean way for the following situation:
driver built as part of kernel, but watchdog system build as a module. We should imho avoid that.
So no for this peticular patch and Guenter you can o ahead with another patch to make it tristate.

Kind regards,
Wim.




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular
  2019-04-27  9:48         ` Wim Van Sebroeck
@ 2019-04-29 16:28           ` Guenter Roeck
  0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2019-04-29 16:28 UTC (permalink / raw)
  To: Wim Van Sebroeck
  Cc: Paul Gortmaker, linux-watchdog, Wim Van Sebroeck, Alan Cox

On Sat, Apr 27, 2019 at 11:48:34AM +0200, Wim Van Sebroeck wrote:
> Hi All,
> 
> > On Wed, Apr 24, 2019 at 11:37:00AM -0400, Paul Gortmaker wrote:
> > > [Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular] On 23/04/2019 (Tue 18:22) Guenter Roeck wrote:
> > > 
> > > > On 4/23/19 8:48 AM, Paul Gortmaker wrote:
> > > > >The Kconfig currently controlling compilation of this code is:
> > > > >
> > > > >config WATCHDOG_CORE
> > > > >        bool "WatchDog Timer Driver Core"
> > > > >
> > > > >...meaning that it currently is not being built as a module by anyone.
> > > > >
> > > > >Lets remove the modular code that is essentially orphaned, so that
> > > > >when reading the driver there is no doubt it is builtin-only.
> > > > >
> > > > >We replace module.h with export.h since the file does export some
> > > > >symbols.  We don't add init.h since the file already has that.
> > > > >
> > > > >We also delete the MODULE_LICENSE tag etc. since all that information
> > > > >is already contained at the top of the file in the comments.
> > > > >
> > > > 
> > > > I must admit that I am not at all happy about this change. While not
> > > > configurable by default, I used tristate a lot (after enabling it
> > > > manually) to test watchdog core code while changing it. It saves a
> > > > lot of time to be able to reload the watchdog core without having
> > > > to reboot the entire system after each change. Removing the ability
> > > 
> > > I'm confused.  If it is useful, then why not formally make it tristate
> > > so other people can do the same as you do, and nobody is manually making
> > > the change over and over again each time?  I'd support that update.
> > > 
> > No idea. That precedes my involvement in the watchdog subsystem.
> > Let's wait for input from Wim. I have a set of patches ready, but it
> > doesn't make sense to me to submit them if Wim wants to go the non-modular
> > route.
> > 
> > FWIW, I am fine with the other patches except for the npcm patch, because
> > several of the other npcm drivers are buildable as module.
> 
> In general: If systems/devices can't handle modules then we should indeed make sure that we clean it up.
> 
Makes sense.

> For the watchdog core however, I am not in favour of doing that. I also use it as a module when i'm testing.
> I originally wanted to make it tristate (so that it can be loaded as a module), but I didn't had a clean way for the following situation:
> driver built as part of kernel, but watchdog system build as a module. We should imho avoid that.
> So no for this peticular patch and Guenter you can o ahead with another patch to make it tristate.
> 

That should be addressed by "select WATCHDOG_CORE" which is used throughout
the kernel. It would be a problem if we had any "depends on WATCHDOG_CORE".
Fortunately, there are no such dependencies. There are a couple of "depends
on WATCHDOG", but they are all "depends on WATCHDOG" followed by "select
WATCHDOG_CORE" as it should be. So we should be fine; any watchdog driver
built into the kernel forces WATCHDOG_CORE to be built into the kernel as
well.

I'll try to clean up my series and send it out this week. It requires more
than one patch since there are some dependencies on the pretimeout code.

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 5/5] watchdog: coh901327: make it explicitly non-modular
  2019-04-23 15:48 ` [PATCH 5/5] watchdog: coh901327: " Paul Gortmaker
  2019-04-23 21:29   ` Linus Walleij
@ 2019-04-29 16:37   ` Guenter Roeck
  1 sibling, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2019-04-29 16:37 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: Wim Van Sebroeck, linux-watchdog, Linus Walleij, Wim Van Sebroeck

On Tue, Apr 23, 2019 at 11:48:35AM -0400, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
> 
> config COH901327_WATCHDOG
>        bool "ST-Ericsson COH 901 327 watchdog"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
> 
> We explicitly disallow a driver unbind, since that doesn't have a
> sensible use case anyway, and it allows us to drop the ".remove"
> code for non-modular drivers.
> 
> Since module_platform_driver() uses the same init level priority as
> builtin_platform_driver() the init ordering remains unchanged with
> this commit.
> 
> Also note that MODULE_ALIAS is a no-op for non-modular code.
> 
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
> 
> We replace module.h with moduleparam.h since the file does actually
> declare some module parameters (i.e. boot args for non-modules).
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-watchdog@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/5] watchdog: intel_scu: make it explicitly non-modular
  2019-04-23 15:48 ` [PATCH 4/5] watchdog: intel_scu: " Paul Gortmaker
@ 2019-04-29 16:37   ` Guenter Roeck
  0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2019-04-29 16:37 UTC (permalink / raw)
  To: Paul Gortmaker; +Cc: Wim Van Sebroeck, linux-watchdog, Wim Van Sebroeck

On Tue, Apr 23, 2019 at 11:48:34AM -0400, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
> 
> config INTEL_SCU_WATCHDOG
>        bool "Intel SCU Watchdog for Mobile Platforms"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
> 
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
> 
> We leave the moduleparam.h include since the file does declare some
> module parameters, and leaving them as such is currently the easiest
> way to remain compatible with existing boot arg use cases.
> 
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-watchdog@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

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

> ---
>  drivers/watchdog/intel_scu_watchdog.c | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/drivers/watchdog/intel_scu_watchdog.c b/drivers/watchdog/intel_scu_watchdog.c
> index 0caab6241eb7..32bc5611f0cc 100644
> --- a/drivers/watchdog/intel_scu_watchdog.c
> +++ b/drivers/watchdog/intel_scu_watchdog.c
> @@ -25,7 +25,6 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/compiler.h>
> -#include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/moduleparam.h>
>  #include <linux/types.h>
> @@ -545,21 +544,4 @@ static int __init intel_scu_watchdog_init(void)
>  	iounmap(watchdog_device.timer_load_count_addr);
>  	return ret;
>  }
> -
> -static void __exit intel_scu_watchdog_exit(void)
> -{
> -
> -	misc_deregister(&watchdog_device.miscdev);
> -	unregister_reboot_notifier(&watchdog_device.intel_scu_notifier);
> -	/* disable the timer */
> -	iowrite32(0x00000002, watchdog_device.timer_control_addr);
> -	iounmap(watchdog_device.timer_load_count_addr);
> -}
> -
>  late_initcall(intel_scu_watchdog_init);
> -module_exit(intel_scu_watchdog_exit);
> -
> -MODULE_AUTHOR("Intel Corporation");
> -MODULE_DESCRIPTION("Intel SCU Watchdog Device Driver");
> -MODULE_LICENSE("GPL");
> -MODULE_VERSION(WDT_VER);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/5] watchdog: rtd119x: drop unused module.h include
  2019-04-23 15:48 ` [PATCH 1/5] watchdog: rtd119x: drop unused module.h include Paul Gortmaker
@ 2019-04-29 16:38   ` Guenter Roeck
  0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2019-04-29 16:38 UTC (permalink / raw)
  To: Paul Gortmaker; +Cc: Wim Van Sebroeck, linux-watchdog

On Tue, Apr 23, 2019 at 11:48:31AM -0400, Paul Gortmaker wrote:
> The Kconfig for this driver is:
> 
> config RTD119X_WATCHDOG
>        bool "Realtek RTD118x/RTD129x watchdog support"
> 
> ...and hence it doesn't need to include module.h for anything.
> There are no other signs of unused modular infrastructure.
> 
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-watchdog@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/5] watchdog: npcm: make it explicitly non-modular
  2019-04-23 15:48 ` [PATCH 3/5] watchdog: npcm: " Paul Gortmaker
@ 2019-04-29 16:40   ` Guenter Roeck
  2019-04-29 18:21     ` Paul Gortmaker
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2019-04-29 16:40 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: Wim Van Sebroeck, linux-watchdog, Avi Fishman, Tomer Maimon,
	Tali Perry, Patrick Venture, Nancy Yuen, Benjamin Fair, openbmc

On Tue, Apr 23, 2019 at 11:48:33AM -0400, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
> 
> config NPCM7XX_WATCHDOG
>        bool "Nuvoton NPCM750 watchdog"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
> 
> Since module_platform_driver() uses the same init level priority as
> builtin_platform_driver() the init ordering remains unchanged with
> this commit.
> 
> Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
> 
> We also delete the MODULE_LICENSE tag etc. since all that information
> was (or is now) contained at the top of the file in the comments.
> 
> Cc: Avi Fishman <avifishman70@gmail.com>
> Cc: Tomer Maimon <tmaimon77@gmail.com>
> Cc: Tali Perry <tali.perry1@gmail.com>
> Cc: Patrick Venture <venture@google.com>
> Cc: Nancy Yuen <yuenn@google.com>
> Cc: Benjamin Fair <benjaminfair@google.com>
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: openbmc@lists.ozlabs.org
> Cc: linux-watchdog@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

I'll send a different patch to make the driver tristate,
to follow the example given by other drivers for the same chipset.

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/5] watchdog: npcm: make it explicitly non-modular
  2019-04-29 16:40   ` Guenter Roeck
@ 2019-04-29 18:21     ` Paul Gortmaker
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Gortmaker @ 2019-04-29 18:21 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wim Van Sebroeck, linux-watchdog, Avi Fishman, Tomer Maimon,
	Tali Perry, Patrick Venture, Nancy Yuen, Benjamin Fair, openbmc

[Re: [PATCH 3/5] watchdog: npcm: make it explicitly non-modular] On 29/04/2019 (Mon 09:40) Guenter Roeck wrote:

> On Tue, Apr 23, 2019 at 11:48:33AM -0400, Paul Gortmaker wrote:
> > The Kconfig currently controlling compilation of this code is:
> > 
> > config NPCM7XX_WATCHDOG
> >        bool "Nuvoton NPCM750 watchdog"
> > 
> > ...meaning that it currently is not being built as a module by anyone.
> > 

[...]

> I'll send a different patch to make the driver tristate,
> to follow the example given by other drivers for the same chipset.

Great, thanks.  I'll drop this patch from my internal queue once I see
it conflict with your tristate conversion in linux-next.

Paul.
--

> 
> Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5] wdt: clean up unused modular infrastructure
  2019-04-23 15:48 [PATCH 0/5] wdt: clean up unused modular infrastructure Paul Gortmaker
                   ` (4 preceding siblings ...)
  2019-04-23 15:48 ` [PATCH 5/5] watchdog: coh901327: " Paul Gortmaker
@ 2019-05-12 15:43 ` Avi Fishman
  5 siblings, 0 replies; 18+ messages in thread
From: Avi Fishman @ 2019-05-12 15:43 UTC (permalink / raw)
  To: Paul Gortmaker
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, Patrick Venture,
	OpenBMC Maillist, Tomer Maimon, Tali Perry, Wim Van Sebroeck,
	Linus Walleij, Benjamin Fair

I saw many drivers that are putting "#ifdef MODULE" around module specific code.
I think it answers all 4 concerns above.
It also covers the case where a developer made the driver available to
be compiled as a module and in the Kconfig didn't make it tristate.
What do you think?

On Thu, May 9, 2019 at 9:17 AM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> People can embed modular includes and modular exit functions into code
> that never use any of it, and they won't get any errors or warnings.
>
> Using modular infrastructure in non-modules might seem harmless, but some
> of the downfalls this leads to are:
>
>  (1) it is easy to accidentally write unused module_exit/remove code
>  (2) it can be misleading when reading the source, thinking a driver can
>      be modular when the Makefile and/or Kconfig prohibit it
>  (3) an unused include of the module.h header file will in turn
>      include nearly everything else; adding a lot to CPP overhead.
>  (4) it gets copied/replicated into other drivers and can spread.
>
> As a data point for #3 above, an empty C file that just includes the
> module.h header generates over 750kB of CPP output.  Repeating the same
> experiment with init.h and the result is less than 12kB; with export.h
> it is only about 1/2kB; with both it still is less than 12kB.
>
> Here, In this series, we do what has been done for other subsystems,
> like, net, x86, mfd, iommu....  and audit for uses of modular
> infrastructure inside code that currently can't be built as a module.
>
> As always, the option exists for driver authors to convert their code
> to tristate, if there is a valid use case for it to be so.  But since
> I don't have the context for each driver to know if such a use case
> exists, I limit myself to simply removing the unused code in order to
> make the driver consistent with the Makefile/Kconfig settings that
> control it.
>
> Paul.
>
> ---
>
> Cc: Benjamin Fair <benjaminfair@google.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-watchdog@vger.kernel.org
> Cc: Nancy Yuen <yuenn@google.com>
> Cc: openbmc@lists.ozlabs.org
> Cc: Patrick Venture <venture@google.com>
> Cc: Tali Perry <tali.perry1@gmail.com>
> Cc: Tomer Maimon <tmaimon77@gmail.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
>
>
> Paul Gortmaker (5):
>   watchdog: rtd119x: drop unused module.h include
>   watchdog: watchdog_core: make it explicitly non-modular
>   watchdog: npcm: make it explicitly non-modular
>   watchdog: intel_scu: make it explicitly non-modular
>   watchdog: coh901327: make it explicitly non-modular
>
>  drivers/watchdog/coh901327_wdt.c      | 24 ++++--------------------
>  drivers/watchdog/intel_scu_watchdog.c | 18 ------------------
>  drivers/watchdog/npcm_wdt.c           | 13 ++++++-------
>  drivers/watchdog/rtd119x_wdt.c        |  1 -
>  drivers/watchdog/watchdog_core.c      | 15 +--------------
>  5 files changed, 11 insertions(+), 60 deletions(-)
>
> --
> 2.7.4
>


-- 
Regards,
Avi

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-23 15:48 [PATCH 0/5] wdt: clean up unused modular infrastructure Paul Gortmaker
2019-04-23 15:48 ` [PATCH 1/5] watchdog: rtd119x: drop unused module.h include Paul Gortmaker
2019-04-29 16:38   ` Guenter Roeck
2019-04-23 15:48 ` [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular Paul Gortmaker
2019-04-24  1:22   ` Guenter Roeck
2019-04-24 15:37     ` Paul Gortmaker
2019-04-24 21:05       ` Guenter Roeck
2019-04-27  9:48         ` Wim Van Sebroeck
2019-04-29 16:28           ` Guenter Roeck
2019-04-23 15:48 ` [PATCH 3/5] watchdog: npcm: " Paul Gortmaker
2019-04-29 16:40   ` Guenter Roeck
2019-04-29 18:21     ` Paul Gortmaker
2019-04-23 15:48 ` [PATCH 4/5] watchdog: intel_scu: " Paul Gortmaker
2019-04-29 16:37   ` Guenter Roeck
2019-04-23 15:48 ` [PATCH 5/5] watchdog: coh901327: " Paul Gortmaker
2019-04-23 21:29   ` Linus Walleij
2019-04-29 16:37   ` Guenter Roeck
2019-05-12 15:43 ` [PATCH 0/5] wdt: clean up unused modular infrastructure Avi Fishman

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox