[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@xxxxxxxxx>
> >Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> >Cc: Alan Cox <alan@xxxxxxxxxxxxxxxxxxx>
> >Cc: linux-watchdog@xxxxxxxxxxxxxxx
> >Signed-off-by: Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx>
> >---
> > 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@xxxxxxxxxxxxxxxxxxx>");
> >-MODULE_AUTHOR("Wim Van Sebroeck <wim@xxxxxxxxx>");
> >-MODULE_DESCRIPTION("WatchDog Timer Driver Core");
> >-MODULE_LICENSE("GPL");
> >
>
