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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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);
>> -			}
>>  		/* 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;}
>>
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog