Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383493 > unrolled thread

[PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-04-20 17:30 +0200
    Re: [PATCH v2] drivers/idle: make intel_idle.c driver more  explicitly non-modular Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-04-20 20:20 +0200
      Re: [PATCH v2] drivers/idle: make intel_idle.c driver more  explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-04-21 05:20 +0200
        Re: [PATCH v2] drivers/idle: make intel_idle.c driver more  explicitly non-modular Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-04-21 10:10 +0200
          Re: [PATCH v2] drivers/idle: make intel_idle.c driver more  explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-04-21 14:50 +0200
            Re: [PATCH v2] drivers/idle: make intel_idle.c driver more  explicitly non-modular Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-04-21 15:30 +0200
              Re: [PATCH v2] drivers/idle: make intel_idle.c driver more  explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-04-21 19:50 +0200

#1383493 — [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

The Kconfig for this driver is currently declared with:

config INTEL_IDLE
        bool "Cpuidle Driver for Intel Processors"

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

This was done in commit 6ce9cd8669fa1195fdc21643370e34523c7ac988
("intel_idle: disable module support") since "...the module capability
is cauing more trouble than it is worth."

This was done over 5y ago, and Daniel adds that:

    ...the modular support has been removed from almost all the cpuidle
    drivers and the cpuidle framework is no longer assuming driver could
    be unloaded.

    Removing the modular dead code in the driver makes sense as this
    what have been done in the others drivers.

So 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_init translates to device_initcall in the non-modular
case, the init ordering remains unchanged with this commit.  At a
later date we might want to consider whether subsys_init or another
init category seems more appropriate than device_init.

We replace module.h with moduleparam.h 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.

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

Also note that we can't remove intel_idle_cpuidle_devices_uninit() as
that is still used for unwind purposes if the init fails.

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: Len Brown <len.brown@intel.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: rcochran@linutronix.de
Cc: linux-pm@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---

[v2: refresh for recent upstream changes to exit code; add comment from
 Daniel to commit log.  Redo 32/64 build tests on today's linux-next.]

 drivers/idle/intel_idle.c | 41 ++++++++---------------------------------
 1 file changed, 8 insertions(+), 33 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index c96649292b55..f93788fad49a 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -46,8 +46,6 @@
  * to avoid complications with the lapic timer workaround.
  * Have not seen issues with suspend, but may need same workaround here.
  *
- * There is currently no kernel-based automatic probing/loading mechanism
- * if the driver is built as a module.
  */
 
 /* un-comment DEBUG to enable pr_debug() statements */
@@ -60,7 +58,7 @@
 #include <linux/sched.h>
 #include <linux/notifier.h>
 #include <linux/cpu.h>
-#include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <asm/cpu_device_id.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>
@@ -1054,7 +1052,6 @@ static const struct x86_cpu_id intel_idle_ids[] __initconst = {
 	ICPU(0x5c, idle_cpu_bxt),
 	{}
 };
-MODULE_DEVICE_TABLE(x86cpu, intel_idle_ids);
 
 /*
  * intel_idle_probe()
@@ -1415,34 +1412,12 @@ static int __init intel_idle_init(void)
 
 	return 0;
 }
+device_initcall(intel_idle_init);
 
-static void __exit intel_idle_exit(void)
-{
-	struct cpuidle_device *dev;
-	int i;
-
-	cpu_notifier_register_begin();
-
-	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
-		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-	__unregister_cpu_notifier(&cpu_hotplug_notifier);
-
-	for_each_possible_cpu(i) {
-		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
-		cpuidle_unregister_device(dev);
-	}
-
-	cpu_notifier_register_done();
-
-	cpuidle_unregister_driver(&intel_idle_driver);
-	free_percpu(intel_idle_cpuidle_devices);
-}
-
-module_init(intel_idle_init);
-module_exit(intel_idle_exit);
-
+/*
+ * We are not really modular, but we used to support that.  Meaning we also
+ * support "intel_idle.max_cstate=..." at boot and also a read-only export of
+ * it at /sys/module/intel_idle/parameters/max_cstate -- so using module_param
+ * is the easiest way (currently) to continue doing that.
+ */
 module_param(max_cstate, int, 0444);
-
-MODULE_AUTHOR("Len Brown <len.brown@intel.com>");
-MODULE_DESCRIPTION("Cpuidle driver for Intel Hardware v" INTEL_IDLE_VERSION);
-MODULE_LICENSE("GPL");
-- 
2.8.0

[toc] | [next] | [standalone]

#1383621 — Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

On Wed, Apr 20, 2016 at 11:25:06AM -0400, Paul Gortmaker wrote:
> The Kconfig for this driver is currently declared with:
> 
> config INTEL_IDLE
>         bool "Cpuidle Driver for Intel Processors"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> This was done in commit 6ce9cd8669fa1195fdc21643370e34523c7ac988
> ("intel_idle: disable module support") since "...the module capability
> is cauing more trouble than it is worth."
> 
> This was done over 5y ago, and Daniel adds that:
> 
>     ...the modular support has been removed from almost all the cpuidle
>     drivers and the cpuidle framework is no longer assuming driver could
>     be unloaded.
> 
>     Removing the modular dead code in the driver makes sense as this
>     what have been done in the others drivers.
> 
> So 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_init translates to device_initcall in the non-modular
> case, the init ordering remains unchanged with this commit.  At a
> later date we might want to consider whether subsys_init or another
> init category seems more appropriate than device_init.
> 
> We replace module.h with moduleparam.h 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.

What about using __setup() ? so module* disappear from the file.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/block/brd.c#n463
 
> Note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
> 
> Also note that we can't remove intel_idle_cpuidle_devices_uninit() as
> that is still used for unwind purposes if the init fails.
> 
> 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: Len Brown <len.brown@intel.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: rcochran@linutronix.de
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

[toc] | [prev] | [next] | [standalone]

#1383854 — Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

[Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular] On 20/04/2016 (Wed 20:13) Daniel Lezcano wrote:

> On Wed, Apr 20, 2016 at 11:25:06AM -0400, Paul Gortmaker wrote:
> > The Kconfig for this driver is currently declared with:
> > 
> > config INTEL_IDLE
> >         bool "Cpuidle Driver for Intel Processors"
> > 
> > ...meaning that it currently is not being built as a module by anyone.
> > 
> > This was done in commit 6ce9cd8669fa1195fdc21643370e34523c7ac988
> > ("intel_idle: disable module support") since "...the module capability
> > is cauing more trouble than it is worth."
> > 
> > This was done over 5y ago, and Daniel adds that:
> > 
> >     ...the modular support has been removed from almost all the cpuidle
> >     drivers and the cpuidle framework is no longer assuming driver could
> >     be unloaded.
> > 
> >     Removing the modular dead code in the driver makes sense as this
> >     what have been done in the others drivers.
> > 
> > So 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_init translates to device_initcall in the non-modular
> > case, the init ordering remains unchanged with this commit.  At a
> > later date we might want to consider whether subsys_init or another
> > init category seems more appropriate than device_init.
> > 
> > We replace module.h with moduleparam.h 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.
> 
> What about using __setup() ? so module* disappear from the file.

No, it can't be __setup since moduleparam uses an instance of the
filename as a prefix to the boot arg, and __setup does not.  And we
should stay compatible with existing boot arg use cases for people
who have embedded such a setting in their grub config a long time
ago and forgot it.  It would take looking at and likely extending the
early_param macro to provide a syntax compatible instance of what
the module_param currently does if I recall correctly -- hence the
above comment in the commit log.

Paul.
--

> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/block/brd.c#n463
>  
> > Note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
> > 
> > Also note that we can't remove intel_idle_cpuidle_devices_uninit() as
> > that is still used for unwind purposes if the init fails.
> > 
> > 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: Len Brown <len.brown@intel.com>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: rcochran@linutronix.de
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

[toc] | [prev] | [next] | [standalone]

#1383922 — Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

On Wed, Apr 20, 2016 at 11:12:49PM -0400, Paul Gortmaker wrote:

[ ... ]

> > > We replace module.h with moduleparam.h 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.
> > 
> > What about using __setup() ? so module* disappear from the file.
> 
> No, it can't be __setup since moduleparam uses an instance of the
> filename as a prefix to the boot arg, and __setup does not.  And we
> should stay compatible with existing boot arg use cases for people
> who have embedded such a setting in their grub config a long time
> ago and forgot it.  It would take looking at and likely extending the
> early_param macro to provide a syntax compatible instance of what
> the module_param currently does if I recall correctly -- hence the
> above comment in the commit log.

-module_param(max_cstate, int, 0444);
+static int __init max_cstate_param(char *str)
+{
+       max_cstate = simple_strtol(str, NULL, 0);
+       return 1;
+}
+__setup("intel_idle.max_cstate=", max_cstate_param);

[toc] | [prev] | [next] | [standalone]

#1384170 — Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

[Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular] On 21/04/2016 (Thu 10:04) Daniel Lezcano wrote:

> On Wed, Apr 20, 2016 at 11:12:49PM -0400, Paul Gortmaker wrote:
> 
> [ ... ]
> 
> > > > We replace module.h with moduleparam.h 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.
> > > 
> > > What about using __setup() ? so module* disappear from the file.
> > 
> > No, it can't be __setup since moduleparam uses an instance of the
> > filename as a prefix to the boot arg, and __setup does not.  And we
> > should stay compatible with existing boot arg use cases for people
> > who have embedded such a setting in their grub config a long time
> > ago and forgot it.  It would take looking at and likely extending the
> > early_param macro to provide a syntax compatible instance of what
> > the module_param currently does if I recall correctly -- hence the
> > above comment in the commit log.
> 
> -module_param(max_cstate, int, 0444);
> +static int __init max_cstate_param(char *str)
> +{
> +       max_cstate = simple_strtol(str, NULL, 0);
> +       return 1;
> +}
> +__setup("intel_idle.max_cstate=", max_cstate_param);

Yeah, I recall thinking it would be that easy too, but there was
something that happens when you manually insert the dot in there that
breaks processing.  I'd have to re-test to remind myself what failed.

Paul.
--

> 
> 

[toc] | [prev] | [next] | [standalone]

#1384221 — Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

On Thu, Apr 21, 2016 at 08:44:55AM -0400, Paul Gortmaker wrote:
> [Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular] On 21/04/2016 (Thu 10:04) Daniel Lezcano wrote:
> 
> > On Wed, Apr 20, 2016 at 11:12:49PM -0400, Paul Gortmaker wrote:
> > 
> > [ ... ]
> > 
> > > > > We replace module.h with moduleparam.h 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.
> > > > 
> > > > What about using __setup() ? so module* disappear from the file.
> > > 
> > > No, it can't be __setup since moduleparam uses an instance of the
> > > filename as a prefix to the boot arg, and __setup does not.  And we
> > > should stay compatible with existing boot arg use cases for people
> > > who have embedded such a setting in their grub config a long time
> > > ago and forgot it.  It would take looking at and likely extending the
> > > early_param macro to provide a syntax compatible instance of what
> > > the module_param currently does if I recall correctly -- hence the
> > > above comment in the commit log.
> > 
> > -module_param(max_cstate, int, 0444);
> > +static int __init max_cstate_param(char *str)
> > +{
> > +       max_cstate = simple_strtol(str, NULL, 0);
> > +       return 1;
> > +}
> > +__setup("intel_idle.max_cstate=", max_cstate_param);
> 
> Yeah, I recall thinking it would be that easy too, but there was
> something that happens when you manually insert the dot in there that
> breaks processing.  I'd have to re-test to remind myself what failed.

Ok.

I quickly tested this code snippet and, except I missed something, it 
worked.

That said, I looked around and found that using module_param() for 
non-modular is found in several places, so it is common. I don't like to 
find references to modular code when the the caller is not supposed to be 
modular but that's the situation today.

So I will let Len and you decice what to do ;)

Other than that: Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

[toc] | [prev] | [next] | [standalone]

#1384456 — Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular

[Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular] On 21/04/2016 (Thu 15:21) Daniel Lezcano wrote:

> On Thu, Apr 21, 2016 at 08:44:55AM -0400, Paul Gortmaker wrote:
> > [Re: [PATCH v2] drivers/idle: make intel_idle.c driver more explicitly non-modular] On 21/04/2016 (Thu 10:04) Daniel Lezcano wrote:
> > 
> > > On Wed, Apr 20, 2016 at 11:12:49PM -0400, Paul Gortmaker wrote:
> > > 
> > > [ ... ]
> > > 
> > > > > > We replace module.h with moduleparam.h 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.
> > > > > 
> > > > > What about using __setup() ? so module* disappear from the file.
> > > > 
> > > > No, it can't be __setup since moduleparam uses an instance of the
> > > > filename as a prefix to the boot arg, and __setup does not.  And we
> > > > should stay compatible with existing boot arg use cases for people
> > > > who have embedded such a setting in their grub config a long time
> > > > ago and forgot it.  It would take looking at and likely extending the
> > > > early_param macro to provide a syntax compatible instance of what
> > > > the module_param currently does if I recall correctly -- hence the
> > > > above comment in the commit log.
> > > 
> > > -module_param(max_cstate, int, 0444);
> > > +static int __init max_cstate_param(char *str)
> > > +{
> > > +       max_cstate = simple_strtol(str, NULL, 0);
> > > +       return 1;
> > > +}
> > > +__setup("intel_idle.max_cstate=", max_cstate_param);
> > 
> > Yeah, I recall thinking it would be that easy too, but there was
> > something that happens when you manually insert the dot in there that
> > breaks processing.  I'd have to re-test to remind myself what failed.
> 
> Ok.
> 
> I quickly tested this code snippet and, except I missed something, it 
> worked.

Maybe it was when I used early_param in testing that it failed....

> 
> That said, I looked around and found that using module_param() for 
> non-modular is found in several places, so it is common. I don't like to 
> find references to modular code when the the caller is not supposed to be 

Agreed, I wasn't a fan of it either, and had it on my to-do list to
circle back around and revisit them once I'd got all the dead code
removed tree wide, and as you say, there are instances already, so I
figured no real drastic rush needed for that.

> modular but that's the situation today.
> 
> So I will let Len and you decice what to do ;)
> 
> Other than that: Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks for the input on this.  It is Len's subsystem, so he gets the final
say and not me.  :)

Paul.
--

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web