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

From: Paul Gortmaker
Date: Mon Apr 04 2016 - 15:55:59 EST


[[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@xxxxxxxxxx>
> Cc: Len Brown <len.brown@xxxxxxxxx>
> Cc: linux-pm@xxxxxxxxxxxxxxx
> Signed-off-by: Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>");
> -MODULE_DESCRIPTION("Cpuidle driver for Intel Hardware v" INTEL_IDLE_VERSION);
> -MODULE_LICENSE("GPL");
> --
> 2.6.1
>