| Message ID | 1556034515-28792-3-git-send-email-paul.gortmaker@windriver.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | wdt: clean up unused modular infrastructure | expand |
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"); >
[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"); > > >
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
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.
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
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");
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(-)