| Message ID | 1459099777-4962-1-git-send-email-paul.gortmaker@windriver.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
[[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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");
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(-)