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

Groups > linux.kernel > #1370844 > unrolled thread

Re: [PATCH] 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

  Re: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-04-04 22:00 +0200
    Re: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular rcochran@linutronix.de - 2016-04-05 05:20 +0200
      RE: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular "Brown, Len" <len.brown@intel.com> - 2016-04-05 06:30 +0200
        Re: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular rcochran@linutronix.de - 2016-04-05 06:40 +0200
          RE: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular "Brown, Len" <len.brown@intel.com> - 2016-04-05 08:00 +0200
            Re: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular rcochran@linutronix.de - 2016-04-05 09:40 +0200
              Re: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-04-07 19:00 +0200
      Re: [PATCH] drivers/idle: make intel_idle.c driver more explicitly  non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-04-05 20:30 +0200

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

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

[toc] | [next] | [standalone]

#1371156

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

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

#1371184

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.

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

#1371188

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

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

#1371235

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

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

#1371283

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

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

#1373574

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

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

#1371874

[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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web