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

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

[Paul Gortmaker]

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."

Since this was done over 5y ago, it is safe to say there is no big desire
to overcome the issues with modular versions.  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 <lenb@kernel.org>
Cc: Len Brown <len.brown@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/idle/intel_idle.c | 35 ++++++++---------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index ba947df5a8c7..d1b5a821664d 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>
@@ -948,7 +946,6 @@ static const struct x86_cpu_id intel_idle_ids[] __initconst = {
 	ICPU(0x57, idle_cpu_knl),
 	{}
 };
-MODULE_DEVICE_TABLE(x86cpu, intel_idle_ids);
 
 /*
  * intel_idle_probe()
@@ -1247,28 +1244,12 @@ static int __init intel_idle_init(void)
 
 	return 0;
 }
+device_initcall(intel_idle_init);
 
-static void __exit intel_idle_exit(void)
-{
-	intel_idle_cpuidle_devices_uninit();
-	cpuidle_unregister_driver(&intel_idle_driver);
-
-	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);
-
-	cpu_notifier_register_done();
-
-	return;
-}
-
-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.6.1

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

[Paul Gortmaker]

[[PATCH] drivers/idle: make intel_idle.c driver more explicitly non-modular] On 27/03/2016 (Sun 13:29) 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."
> 
> Since this was done over 5y ago, it is safe to say there is no big desire
> to overcome the issues with modular versions.  So lets remove the modular
> code that is essentially orphaned, so that when reading the driver there
> is no doubt it is builtin-only.

This patch will no longer apply since there were several updates to this
driver by Richard Cochran dated March 29th.   Before I go and refresh
the patch for a v2, is there any objections to the general goal of what
the patch was aiming to achieve -- avoiding use of modular infrastructure
in non-modular code, and not having module_exit code that can't be run?

Thanks,
Paul.
--

> 
> 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 <lenb@kernel.org>
> Cc: Len Brown <len.brown@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  drivers/idle/intel_idle.c | 35 ++++++++---------------------------
>  1 file changed, 8 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index ba947df5a8c7..d1b5a821664d 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>
> @@ -948,7 +946,6 @@ static const struct x86_cpu_id intel_idle_ids[] __initconst = {
>  	ICPU(0x57, idle_cpu_knl),
>  	{}
>  };
> -MODULE_DEVICE_TABLE(x86cpu, intel_idle_ids);
>  
>  /*
>   * intel_idle_probe()
> @@ -1247,28 +1244,12 @@ static int __init intel_idle_init(void)
>  
>  	return 0;
>  }
> +device_initcall(intel_idle_init);
>  
> -static void __exit intel_idle_exit(void)
> -{
> -	intel_idle_cpuidle_devices_uninit();
> -	cpuidle_unregister_driver(&intel_idle_driver);
> -
> -	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);
> -
> -	cpu_notifier_register_done();
> -
> -	return;
> -}
> -
> -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.6.1
> 

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

[rcochran]

On Mon, Apr 04, 2016 at 03:55:35PM -0400, Paul Gortmaker wrote:
> > This was done in commit 6ce9cd8669fa1195fdc21643370e34523c7ac988
> > ("intel_idle: disable module support") since "...the module capability
> > is cauing more trouble than it is worth."

The reason given in that commit was that "it lost the init race with
ACPI", whatever that means.

> > Since this was done over 5y ago, it is safe to say there is no big desire
> > to overcome the issues with modular versions.  So lets remove the modular
> > code that is essentially orphaned, so that when reading the driver there
> > is no doubt it is builtin-only.

So you want to make the driver non-modular due to lack of desire to
fix it?

> This patch will no longer apply since there were several updates to this
> driver by Richard Cochran dated March 29th.   Before I go and refresh
> the patch for a v2, is there any objections to the general goal of what
> the patch was aiming to achieve -- avoiding use of modular infrastructure
> in non-modular code, and not having module_exit code that can't be run?

On the one hand, the better way is to fix the issues, keeping the
driver's modular form.  That way, by loading and unloading, you can
observe how well it works.  I already started by fixing several bugs
WRT module init/exit.

On the other hand, if there is some technical reason why the modular
form is impossible, then the patch should state it.

Thanks,
Richard

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

[Brown, Len]

The first idle driver to register with cpuidle wins.

intel_idle should always get the opportunity
to probe and register before acpi_idle (processor_idle.c)

When intel_idle was allowed to be modular,
some distros build their kernel and loaded modules
such that acpi_idle could probe first.  In such
a kernel, intel_idle became dead code.

As intel_idle is a small driver, the q	uick fix
was to make it Y/N so that it would always probe
before acpi_idle, no matter how acpi_idle
is build and loaded.

Yes, even though intel_idle is a tiny driver, I think
it would be good to be able to unload it when it doesn't probe.

Today, it appears that acpi_idle (acpi/processor_idle.c)
is compiled Y/N.

thanks,
-Len

ps.

No, I do not believe that cpuidle should bother
supporting changing idle drivers at run-time.

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

[rcochran]

On Tue, Apr 05, 2016 at 04:20:47AM +0000, Brown, Len wrote:
> The first idle driver to register with cpuidle wins.
> 
> intel_idle should always get the opportunity
> to probe and register before acpi_idle (processor_idle.c)
> 
> When intel_idle was allowed to be modular,
> some distros build their kernel and loaded modules
> such that acpi_idle could probe first.  In such
> a kernel, intel_idle became dead code.
> 
> As intel_idle is a small driver, the q	uick fix
> was to make it Y/N so that it would always probe
> before acpi_idle, no matter how acpi_idle
> is build and loaded.
> 
> Yes, even though intel_idle is a tiny driver, I think
> it would be good to be able to unload it when it doesn't probe.

And that means fixing the race with acpi_idle, right?
 
> Today, it appears that acpi_idle (acpi/processor_idle.c)
> is compiled Y/N.

So it, too, needs work?

> No, I do not believe that cpuidle should bother
> supporting changing idle drivers at run-time.

Huh?  But you just said, "it would be good to be able to unload it
when it doesn't probe."

Thanks,
Richard

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

[Brown, Len]

> -----Original Message-----
> From: rcochran@linutronix.de [mailto:rcochran@linutronix.de]
> Sent: Tuesday, April 05, 2016 12:30 AM
> To: Brown, Len
> Cc: Gortmaker, Paul (Wind River); linux-kernel@vger.kernel.org; Len Brown;
> linux-pm@vger.kernel.org
> Subject: Re: [PATCH] drivers/idle: make intel_idle.c driver more
> explicitly non-modular
> 
> On Tue, Apr 05, 2016 at 04:20:47AM +0000, Brown, Len wrote:
> > The first idle driver to register with cpuidle wins.
> >
> > intel_idle should always get the opportunity
> > to probe and register before acpi_idle (processor_idle.c)
> >
> > When intel_idle was allowed to be modular,
> > some distros build their kernel and loaded modules
> > such that acpi_idle could probe first.  In such
> > a kernel, intel_idle became dead code.
> >
> > As intel_idle is a small driver, the q	uick fix
> > was to make it Y/N so that it would always probe
> > before acpi_idle, no matter how acpi_idle
> > is build and loaded.
> >
> > Yes, even though intel_idle is a tiny driver, I think
> > it would be good to be able to unload it when it doesn't probe.
> 
> And that means fixing the race with acpi_idle, right?
> 
> > Today, it appears that acpi_idle (acpi/processor_idle.c)
> > is compiled Y/N.
> 
> So it, too, needs work?

"needs" is somewhat subjective.
Some may argue that this driver is so small,
that an effort to save memory might be more effectively
directed elsewhere.  But if the goal is to save the memory
consumed by this driver when the driver doesn't probe,
then yes, it would have to be made modular.

I don't remember what ACPI dependency made it non-modular.
ACPI has some tricky initialization ordering issues
that are BIOS dependent, and sometimes out of our control.

> > No, I do not believe that cpuidle should bother
> > supporting changing idle drivers at run-time.
> 
> Huh?  But you just said, "it would be good to be able to unload it
> when it doesn't probe."

being able to switch the registered driver at run-time
does not require the driver to be modular.

cheers,
-Len

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

[rcochran]

On Tue, Apr 05, 2016 at 05:53:47AM +0000, Brown, Len wrote:
> > On Tue, Apr 05, 2016 at 04:20:47AM +0000, Brown, Len wrote:
> > > No, I do not believe that cpuidle should bother
> > > supporting changing idle drivers at run-time.
> > 
> > Huh?  But you just said, "it would be good to be able to unload it
> > when it doesn't probe."
> 
> being able to switch the registered driver at run-time
> does not require the driver to be modular.

Uh, right, but you don't think that the cpuidle core should allow
changing drivers.  If it doesn't allowing changing drivers, then there
would be just one choice, compiled in, and thus none of the drivers
should be modular.

Or what do you mean?

Confused,
Richard

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

[Daniel Lezcano]

On Tue, Apr 05, 2016 at 09:33:12AM +0200, rcochran@linutronix.de wrote:
> On Tue, Apr 05, 2016 at 05:53:47AM +0000, Brown, Len wrote:
> > > On Tue, Apr 05, 2016 at 04:20:47AM +0000, Brown, Len wrote:
> > > > No, I do not believe that cpuidle should bother
> > > > supporting changing idle drivers at run-time.
> > > 
> > > Huh?  But you just said, "it would be good to be able to unload it
> > > when it doesn't probe."
> > 
> > being able to switch the registered driver at run-time
> > does not require the driver to be modular.
> 
> Uh, right, but you don't think that the cpuidle core should allow
> changing drivers.  If it doesn't allowing changing drivers, then there
> would be just one choice, compiled in, and thus none of the drivers
> should be modular.

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

The cpuidle drivers are very arch/platform specific and the acpi vs
something is an exception and could be handled differently than
converting to modular again.

I don't see the point on removing a cpuidle driver at runtime to
something else.

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

  -- Daniel

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

[Paul Gortmaker]

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

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

[Daniel Lezcano]

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>

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

[Paul Gortmaker]

[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>

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

[Daniel Lezcano]

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

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

[Paul Gortmaker]

[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.
--

> 
> 

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

[Daniel Lezcano]

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>

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

[Paul Gortmaker]

[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.
--

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

[Len Brown]

Applied, thanks!


Len Brown, Intel Open Source Technology Center

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

[Paul Gortmaker]

[Re: [PATCH] drivers/idle: make intel_idle.c driver more explicitly non-modular] On 05/04/2016 (Tue 05:11) rcochran@linutronix.de wrote:

> On Mon, Apr 04, 2016 at 03:55:35PM -0400, Paul Gortmaker wrote:
> > > This was done in commit 6ce9cd8669fa1195fdc21643370e34523c7ac988
> > > ("intel_idle: disable module support") since "...the module capability
> > > is cauing more trouble than it is worth."
> 
> The reason given in that commit was that "it lost the init race with
> ACPI", whatever that means.
> 
> > > Since this was done over 5y ago, it is safe to say there is no big desire
> > > to overcome the issues with modular versions.  So lets remove the modular
> > > code that is essentially orphaned, so that when reading the driver there
> > > is no doubt it is builtin-only.
> 
> So you want to make the driver non-modular due to lack of desire to
> fix it?
> 
> > This patch will no longer apply since there were several updates to this
> > driver by Richard Cochran dated March 29th.   Before I go and refresh
> > the patch for a v2, is there any objections to the general goal of what
> > the patch was aiming to achieve -- avoiding use of modular infrastructure
> > in non-modular code, and not having module_exit code that can't be run?
> 
> On the one hand, the better way is to fix the issues, keeping the
> driver's modular form.  That way, by loading and unloading, you can
> observe how well it works.  I already started by fixing several bugs
> WRT module init/exit.

So, to be clear, I really don't have a horse in this race.  Be it
built-in or be it modular, I don't care.  But if code pretends to be
modular and is not, and hence has dead code, then I care.  So fixing
this driver to be functionally tristate is just as good as killing
the (currently) unreachable module_exit code IMHO.

It sounds like this discussion will lead us to one of the two, so that
is good.

Paul.
--

> 
> On the other hand, if there is some technical reason why the modular
> form is impossible, then the patch should state it.
> 
> Thanks,
> Richard