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/ 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); + 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); }
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); }
return; diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5ab7183..66d7e0d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -213,14 +213,7 @@ struct cpuidle_governor { extern int cpuidle_register_governor(struct cpuidle_governor *gov); extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
-#ifdef CONFIG_INTEL_IDLE -extern int intel_idle_cpu_init(int cpu); #else -static inline int intel_idle_cpu_init(int cpu) { return -1; } -#endif - -#else -static inline int intel_idle_cpu_init(int cpu) { return -1; }
static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;}
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);}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.
On 06/27/2012 03:06 PM, Thomas Renninger wrote:
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?
[...]
I just wanted to keep same order. If it is not necessary, I won't invert the compilation order in the next patch.
-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.
Yes, that makes sense.
[...]
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.
Oops, right.
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.
Thanks for the review !
-- Daniel
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. 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/acpi/processor_driver.c | 7 ------ drivers/idle/intel_idle.c | 41 +++++++++++++++++++++++++------------- include/linux/cpuidle.h | 7 ------ 3 files changed, 27 insertions(+), 28 deletions(-)
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..fe95d54 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,35 @@ 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); + + if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) + smp_call_function_single(hotcpu, __setup_broadcast_timer, + (void *)true, 1); + + /* + * Some systems can hotplug a cpu at runtime after + * the kernel has booted, we have to initialize the + * driver in this case + */ + dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu); + if (!dev->registered) + intel_idle_cpu_init(hotcpu); + 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) @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */ lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE; - else { + else on_each_cpu(__setup_broadcast_timer, (void *)true, 1); - register_cpu_notifier(&setup_broadcast_notifier); - } + + register_cpu_notifier(&cpu_hotplug_notifier);
pr_debug(PREFIX "v" INTEL_IDLE_VERSION " model 0x%X\n", boot_cpu_data.x86_model); @@ -494,7 +508,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 +553,6 @@ int intel_idle_cpu_init(int cpu)
return 0; } -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
static int __init intel_idle_init(void) { @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void) intel_idle_cpuidle_devices_uninit(); cpuidle_unregister_driver(&intel_idle_driver);
- if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) { + + 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);
return; } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5ab7183..66d7e0d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -213,14 +213,7 @@ struct cpuidle_governor { extern int cpuidle_register_governor(struct cpuidle_governor *gov); extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
-#ifdef CONFIG_INTEL_IDLE -extern int intel_idle_cpu_init(int cpu); #else -static inline int intel_idle_cpu_init(int cpu) { return -1; } -#endif - -#else -static inline int intel_idle_cpu_init(int cpu) { return -1; }
static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;}
On 06/28/2012 02:16 PM, 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. 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
Looks good to me.
Regards, Srivatsa S. Bhat
drivers/acpi/processor_driver.c | 7 ------ drivers/idle/intel_idle.c | 41 +++++++++++++++++++++++++------------- include/linux/cpuidle.h | 7 ------ 3 files changed, 27 insertions(+), 28 deletions(-)
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);}diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index d0f59c3..fe95d54 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,35 @@ 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);
if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)smp_call_function_single(hotcpu, __setup_broadcast_timer,(void *)true, 1);/** Some systems can hotplug a cpu at runtime after* the kernel has booted, we have to initialize the* driver in this case*/dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);if (!dev->registered)intel_idle_cpu_init(hotcpu);- 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) @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */ lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
- else {
- else on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
register_cpu_notifier(&setup_broadcast_notifier);- }
register_cpu_notifier(&cpu_hotplug_notifier);
pr_debug(PREFIX "v" INTEL_IDLE_VERSION " model 0x%X\n", boot_cpu_data.x86_model);
@@ -494,7 +508,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 +553,6 @@ int intel_idle_cpu_init(int cpu)
return 0; } -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
static int __init intel_idle_init(void) { @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void) intel_idle_cpuidle_devices_uninit(); cpuidle_unregister_driver(&intel_idle_driver);
- if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
- 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);
return;
} diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5ab7183..66d7e0d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -213,14 +213,7 @@ struct cpuidle_governor { extern int cpuidle_register_governor(struct cpuidle_governor *gov); extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
-#ifdef CONFIG_INTEL_IDLE -extern int intel_idle_cpu_init(int cpu); #else -static inline int intel_idle_cpu_init(int cpu) { return -1; } -#endif
-#else -static inline int intel_idle_cpu_init(int cpu) { return -1; }
static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;}
On 06/28/2012 01:24 PM, Srivatsa S. Bhat wrote:
On 06/28/2012 02:16 PM, 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. 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
Looks good to me.
Thanks for the review Srivatsa.
Shall I consider it as an acked-by ?
-- Daniel
Regards, Srivatsa S. Bhat
drivers/acpi/processor_driver.c | 7 ------ drivers/idle/intel_idle.c | 41 +++++++++++++++++++++++++------------- include/linux/cpuidle.h | 7 ------ 3 files changed, 27 insertions(+), 28 deletions(-)
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);}diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index d0f59c3..fe95d54 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,35 @@ 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);
if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)smp_call_function_single(hotcpu, __setup_broadcast_timer,(void *)true, 1);/** Some systems can hotplug a cpu at runtime after* the kernel has booted, we have to initialize the* driver in this case*/dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);if (!dev->registered)intel_idle_cpu_init(hotcpu);- 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) @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */ lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
- else {
- else on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
register_cpu_notifier(&setup_broadcast_notifier);- }
register_cpu_notifier(&cpu_hotplug_notifier);
pr_debug(PREFIX "v" INTEL_IDLE_VERSION " model 0x%X\n", boot_cpu_data.x86_model);
@@ -494,7 +508,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 +553,6 @@ int intel_idle_cpu_init(int cpu)
return 0; } -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
static int __init intel_idle_init(void) { @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void) intel_idle_cpuidle_devices_uninit(); cpuidle_unregister_driver(&intel_idle_driver);
- if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
- 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);
return;
} diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5ab7183..66d7e0d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -213,14 +213,7 @@ struct cpuidle_governor { extern int cpuidle_register_governor(struct cpuidle_governor *gov); extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
-#ifdef CONFIG_INTEL_IDLE -extern int intel_idle_cpu_init(int cpu); #else -static inline int intel_idle_cpu_init(int cpu) { return -1; } -#endif
-#else -static inline int intel_idle_cpu_init(int cpu) { return -1; }
static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;}
On 06/28/2012 04:57 PM, Daniel Lezcano wrote:
On 06/28/2012 01:24 PM, Srivatsa S. Bhat wrote:
On 06/28/2012 02:16 PM, 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. 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
Looks good to me.
Thanks for the review Srivatsa.
Shall I consider it as an acked-by ?
Sure, go ahead! :-)
Regards, Srivatsa S. Bhat
On Thursday, June 28, 2012, 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. 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
This one looks good to me too.
Acked-by: Rafael J. Wysocki rjw@sisk.pl
Len, are you going to take it?
Rafael
drivers/acpi/processor_driver.c | 7 ------ drivers/idle/intel_idle.c | 41 +++++++++++++++++++++++++------------- include/linux/cpuidle.h | 7 ------ 3 files changed, 27 insertions(+), 28 deletions(-)
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);}diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index d0f59c3..fe95d54 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,35 @@ 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);
if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)smp_call_function_single(hotcpu, __setup_broadcast_timer,(void *)true, 1);/** Some systems can hotplug a cpu at runtime after* the kernel has booted, we have to initialize the* driver in this case*/dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);if (!dev->registered)intel_idle_cpu_init(hotcpu);- 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) @@ -405,10 +419,10 @@ static int intel_idle_probe(void) if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */ lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
- else {
- else on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
register_cpu_notifier(&setup_broadcast_notifier);- }
- register_cpu_notifier(&cpu_hotplug_notifier);
pr_debug(PREFIX "v" INTEL_IDLE_VERSION " model 0x%X\n", boot_cpu_data.x86_model); @@ -494,7 +508,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 +553,6 @@ int intel_idle_cpu_init(int cpu) return 0; } -EXPORT_SYMBOL_GPL(intel_idle_cpu_init); static int __init intel_idle_init(void) { @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void) intel_idle_cpuidle_devices_uninit(); cpuidle_unregister_driver(&intel_idle_driver);
- if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
- 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);
return; } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5ab7183..66d7e0d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -213,14 +213,7 @@ struct cpuidle_governor { extern int cpuidle_register_governor(struct cpuidle_governor *gov); extern void cpuidle_unregister_governor(struct cpuidle_governor *gov); -#ifdef CONFIG_INTEL_IDLE -extern int intel_idle_cpu_init(int cpu); #else -static inline int intel_idle_cpu_init(int cpu) { return -1; } -#endif
-#else -static inline int intel_idle_cpu_init(int cpu) { return -1; } static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;}
On 06/28/2012 09:24 PM, Rafael J. Wysocki wrote:
On Thursday, June 28, 2012, 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. 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
This one looks good to me too.
Acked-by: Rafael J. Wysocki rjw@sisk.pl
Thanks for the review Rafael.
Len, are you going to take it?
Rafael, Len,
After the discussion [1], I put in place a tree at:
ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git #cpuidle-next
I proposed this tree to group the cpuidle modifications and prevent the last minutes conflict when Len will apply them.
I also included the tree into linux-next for wider testing.
If you want I can take this patch even if it is related to acpi also and in the future if you agree, Len or you could pull from there.
Thanks -- Daniel
[1] https://lkml.org/lkml/2012/6/18/113
Rafael
drivers/acpi/processor_driver.c | 7 ------ drivers/idle/intel_idle.c | 41 +++++++++++++++++++++++++------------- include/linux/cpuidle.h | 7 ------ 3 files changed, 27 insertions(+), 28 deletions(-)
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);}diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index d0f59c3..fe95d54 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,35 @@ 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);
if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)smp_call_function_single(hotcpu, __setup_broadcast_timer,(void *)true, 1);/** Some systems can hotplug a cpu at runtime after* the kernel has booted, we have to initialize the* driver in this case*/dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);if (!dev->registered)intel_idle_cpu_init(hotcpu);- 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) @@ -405,10 +419,10 @@ static int intel_idle_probe(void) if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */ lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
- else {
- else on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
register_cpu_notifier(&setup_broadcast_notifier);- }
- register_cpu_notifier(&cpu_hotplug_notifier);
pr_debug(PREFIX "v" INTEL_IDLE_VERSION " model 0x%X\n", boot_cpu_data.x86_model); @@ -494,7 +508,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 +553,6 @@ int intel_idle_cpu_init(int cpu) return 0; } -EXPORT_SYMBOL_GPL(intel_idle_cpu_init); static int __init intel_idle_init(void) { @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void) intel_idle_cpuidle_devices_uninit(); cpuidle_unregister_driver(&intel_idle_driver);
- if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
- 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);
return; } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5ab7183..66d7e0d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -213,14 +213,7 @@ struct cpuidle_governor { extern int cpuidle_register_governor(struct cpuidle_governor *gov); extern void cpuidle_unregister_governor(struct cpuidle_governor *gov); -#ifdef CONFIG_INTEL_IDLE -extern int intel_idle_cpu_init(int cpu); #else -static inline int intel_idle_cpu_init(int cpu) { return -1; } -#endif
-#else -static inline int intel_idle_cpu_init(int cpu) { return -1; } static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;}
On Friday, June 29, 2012, Daniel Lezcano wrote:
On 06/28/2012 09:24 PM, Rafael J. Wysocki wrote:
On Thursday, June 28, 2012, 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. 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
This one looks good to me too.
Acked-by: Rafael J. Wysocki rjw@sisk.pl
Thanks for the review Rafael.
Len, are you going to take it?
Rafael, Len,
After the discussion [1], I put in place a tree at:
ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git #cpuidle-next
I proposed this tree to group the cpuidle modifications and prevent the last minutes conflict when Len will apply them.
I also included the tree into linux-next for wider testing.
I can't speak for Len, but I'm not sure he'll like this.
Anyway, you seem to have the same material as Len in that tree, won't there be any conflicts in linux-next?
Rafael
On 06/30/2012 12:27 AM, Rafael J. Wysocki wrote:
On Friday, June 29, 2012, Daniel Lezcano wrote:
On 06/28/2012 09:24 PM, Rafael J. Wysocki wrote:
On Thursday, June 28, 2012, 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. 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
This one looks good to me too.
Acked-by: Rafael J. Wysocki rjw@sisk.pl
Thanks for the review Rafael.
Len, are you going to take it?
Rafael, Len,
After the discussion [1], I put in place a tree at:
ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git #cpuidle-next
I proposed this tree to group the cpuidle modifications and prevent the last minutes conflict when Len will apply them.
I also included the tree into linux-next for wider testing.
I can't speak for Len, but I'm not sure he'll like this.
I sent the proposition a week ago and Len was Cc'ed. I guess he is very busy but the problem we are facing is there are a lot of pending modifications for cpuidle because of some new architecture (like the big.LITTLE and tegra3). Colin's patchset is one of them.
Anyway, you seem to have the same material as Len in that tree, won't there be any conflicts in linux-next?
At the first glance no, until he merge the patches we sent. As I already said the purpose is to help to consolidate the patches sent for cpuidle by acting as a proxy.
I hope that helps.
Thanks -- Daniel
On 06/27/2012 02:37 PM, 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.
There is a much better way of doing this. See below.
This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.
Nice :)
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/ 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
OK, so all you are trying to do here is ensure that the intel idle related notifier runs _after_ the acpi related one. To do that, you don't have to touch the Makefiles at all! Just use the appropriate priorities for the notifiers (default is 0), to handle the dependency. You can find some examples in kernel/ sched/core.c. And while doing that you might want to add a separate notifier in intel idle rather than piggy back on the existing timer related one (because you are handling a dependency here, which might not apply to timers, or even worse, cause unwanted side-effects, if any).
I'll take a look at the rest of the patch tomorrow. I think Thomas has already pointed out the rest of the issues.
Regards, Srivatsa S. Bhat
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);}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);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);}
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);}
return;
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5ab7183..66d7e0d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -213,14 +213,7 @@ struct cpuidle_governor { extern int cpuidle_register_governor(struct cpuidle_governor *gov); extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
-#ifdef CONFIG_INTEL_IDLE -extern int intel_idle_cpu_init(int cpu); #else -static inline int intel_idle_cpu_init(int cpu) { return -1; } -#endif
-#else -static inline int intel_idle_cpu_init(int cpu) { return -1; }
static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;}
On Wednesday, June 27, 2012 06:16:33 PM Srivatsa S. Bhat wrote:
On 06/27/2012 02:37 PM, 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.
There is a much better way of doing this. See below.
This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.
Nice :)
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/ 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
OK, so all you are trying to do here is ensure that the intel idle related notifier runs _after_ the acpi related one.
I might oversee something, if you have concerns, please point me to it. If it's all about keeping the order of excuting these functions: acpi_processor_start(pr) and intel_idle_cpu_init() There should be no need for it. Intel idle is pretty separated.
Thomas
On 06/28/2012 01:04 PM, Thomas Renninger wrote:
On Wednesday, June 27, 2012 06:16:33 PM Srivatsa S. Bhat wrote:
On 06/27/2012 02:37 PM, 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.
There is a much better way of doing this. See below.
This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.
Nice :)
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/ 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
OK, so all you are trying to do here is ensure that the intel idle related notifier runs _after_ the acpi related one.
I might oversee something, if you have concerns, please point me to it. If it's all about keeping the order of excuting these functions: acpi_processor_start(pr) and intel_idle_cpu_init() There should be no need for it. Intel idle is pretty separated.
I digged through the code a bit, and even I couldn't find any reason why there should be a dependency between the two events.
Regards, Srivatsa S. Bhat
-
Daniel Lezcano -
Rafael J. Wysocki -
Srivatsa S. Bhat -
Thomas Renninger