Re: [PATCH 4/4] cpuidle - support multiple drivers

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

On 10/07/2012 11:26 PM, Rafael J. Wysocki wrote:
> On Tuesday 25 of September 2012 00:43:54 Daniel Lezcano wrote:
>> With the tegra3 and the big.LITTLE [1] new architectures, several cpus
>> with different characteristics (latencies and states) can co-exists on the
>> system.
>>
>> The cpuidle framework has the limitation of handling only identical cpus.
>>
>> This patch removes this limitation by introducing the multiple driver support
>> for cpuidle.
>>
>> This option is configurable at compile time and should be enabled for the
>> architectures mentioned above. So there is no impact for the other platforms
>> if the option is disabled. The option defaults to 'n'. Note the multiple drivers
>> support is also compatible with the existing drivers, even if just one driver is
>> needed, all the cpu will be tied to this driver using an extra small chunk of
>> processor memory.
>>
>> The multiple driver support use a per-cpu driver pointer instead of a global
>> variable and the accessor to this variable are done from a cpu context.

Thanks Rafael for the review.

I took into account all your remarks for the V2.

[ cut ]

>> +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv)
>> +{
>> +	int ret = 0;
>> +	int i, cpu;
>> +
>> +	for_each_present_cpu(cpu) {
>> +		ret = __cpuidle_register_driver(drv, cpu);
>> +		if (!ret)
>> +			continue;
>> +		for (i = cpu - 1; i >= 0; i--)
> I wonder if this is going to work in all cases.  For example, is there any
> guarantee that the CPU numbers start from 0 and that there are no gaps?

AFAICS, the cpumask.h is not assuming the cpu numbers start from zero
and they are contiguous.

I will fix this reverse loop, thanks for spotting this.

[ cut ]

>>  void cpuidle_unregister_driver(struct cpuidle_driver *drv)
>>  {
>>  	spin_lock(&cpuidle_driver_lock);
>> -	__cpuidle_unregister_driver(drv);
>> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
>> +	__cpuidle_unregister_all_cpu_driver(drv);
>> +#else
>> +	__cpuidle_unregister_driver(drv, smp_processor_id());
>> +#endif
> I'm slightly cautious about using smp_processor_id() above.
> get_cpu()/put_cpu() is the preferred way of doing this nowadays (although
> this particular code is under the spinlock, so it should be OK).
yes, get_cpu does preempt_disable() and smp_processor_id()
As spin_lock does also preempt_disable() that should be ok.
But I changed the code to use the preferred way.

Thanks
-- Daniel


-- 
 <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


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html