Re: [PATCH] acpi: intel_idle : break dependency between modules

Hi,

I agree that such a dependency between 2 modules is not nice. But your patch will have bad side-effects (see comments embedded below).

On Wednesday, June 27, 2012 11:07:48 AM Daniel Lezcano wrote:

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. In order to make it work, the notifier must be initialized in the right order, acpi then intel_idle. This is done in the Makefile. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org

drivers/Makefile | 3 ++- drivers/acpi/processor_driver.c | 7 ------- drivers/idle/intel_idle.c | 22 ++++++++++++++-------- include/linux/cpuidle.h | 7 ------- 4 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile index 2ba29ff..a2454b8 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -12,8 +12,9 @@ obj-$(CONFIG_PCI) += pci/ obj-$(CONFIG_PARISC) += parisc/ obj-$(CONFIG_RAPIDIO) += rapidio/ obj-y += video/ -obj-y += idle/ +# acpi must come before idle for initialization obj-$(CONFIG_ACPI) += acpi/ +obj-y += idle/

This breaks intel_idle. Loading order defines which one comes first and is used: intel_idle or ACPI processor cpuidle driver. With above, one would get acpi_idle cpuidle driver if both are compiled in, instead of the intel_idle one.

Why exactly is this necessary, couldn't it just work?

obj-$(CONFIG_SFI) += sfi/ # PnP must come after ACPI since it will eventually need to check if acpi # was used and do nothing if so diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 0734086..8648b29 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb, * Initialize missing things */ if (pr->flags.need_hotplug_init) {

• 	struct cpuidle_driver *idle_driver =
  

• 		cpuidle_get_driver();
  

• printk(KERN_INFO "Will online and init hotplugged "
         "CPU: %d\n", pr->id);
  WARN(acpi_processor_start(pr), "Failed to start CPU:"
  	" %d\n", pr->id);
  pr->flags.need_hotplug_init = 0;
  

• 	if (idle_driver && !strcmp(idle_driver->name,
  

• 				   "intel_idle")) {
  

• 		intel_idle_cpu_init(pr->id);
  

• 	}
  

  /* Normal CPU soft online event */ } else { acpi_processor_ppc_has_changed(pr, 0);

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index d0f59c3..4c36039 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu; static struct cpuidle_device __percpu *intel_idle_cpuidle_devices; static int intel_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); +static int intel_idle_cpu_init(int cpu); static struct cpuidle_state *cpuidle_state_table; @@ -302,22 +303,28 @@ static void __setup_broadcast_timer(void *arg) clockevents_notify(reason, &cpu); } -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,

• unsigned long action, void *hcpu)
  

+static int cpu_hotplug_notify(struct notifier_block *n,

• 	      unsigned long action, void *hcpu)
  

{ int hotcpu = (unsigned long)hcpu;

• struct cpuidle_device *dev;

switch (action & 0xf) { case CPU_ONLINE: smp_call_function_single(hotcpu, __setup_broadcast_timer, (void *)true, 1);

• dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
  

• if (!dev->registered)
  

• 	intel_idle_cpu_init(hotcpu);
  

A small comment why this can happen and needs to be done (real hotplugged cpu case) might help here later.

break;

} return NOTIFY_OK; } -static struct notifier_block setup_broadcast_notifier = {

• .notifier_call = setup_broadcast_cpuhp_notify,

+static struct notifier_block cpu_hotplug_notifier = {

• .notifier_call = cpu_hotplug_notify,

}; static void auto_demotion_disable(void *dummy) @@ -407,7 +414,7 @@ static int intel_idle_probe(void) lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE; else { on_each_cpu(__setup_broadcast_timer, (void *)true, 1);

• register_cpu_notifier(&setup_broadcast_notifier);
  

• register_cpu_notifier(&cpu_hotplug_notifier);
  

The notifier always has to be registered now, not only in: if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */ case.

} pr_debug(PREFIX "v" INTEL_IDLE_VERSION @@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)

• allocate, initialize, register cpuidle_devices
• @cpu: cpu/core to initialize

*/ -int intel_idle_cpu_init(int cpu) +static int intel_idle_cpu_init(int cpu) { int cstate; struct cpuidle_device *dev; @@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu) return 0; } -EXPORT_SYMBOL_GPL(intel_idle_cpu_init); static int __init intel_idle_init(void) { @@ -583,7 +589,7 @@ static void __exit intel_idle_exit(void) if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) { on_each_cpu(__setup_broadcast_timer, (void *)false, 1);

• unregister_cpu_notifier(&setup_broadcast_notifier);
  

• unregister_cpu_notifier(&cpu_hotplug_notifier);
  

Same.