Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Gortmaker <paul.gortmaker@windriver.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>,
	<linux-watchdog@vger.kernel.org>,
	Wim Van Sebroeck <wim@iguana.be>,
	Alan Cox <alan@lxorguk.ukuu.org.uk>
Subject: Re: [PATCH 2/5] watchdog: watchdog_core: make it explicitly non-modular
Date: Wed, 24 Apr 2019 11:37:00 -0400
Message-ID: <20190424153700.GT23044@windriver.com> (raw)
In-Reply-To: <439d89d7-bae6-c5c8-e9bf-5477304bc065@roeck-us.net>

[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");
> >
> 

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190424153700.GT23044@windriver.com \
    --to=paul.gortmaker@windriver.com \
    --cc=alan@lxorguk.ukuu.org.uk \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=wim@iguana.be \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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