Re: [PATCH 5/5][RFC] cpuidle : add cpuidle_register_states function

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

On 08/10/2012 07:17 PM, Lorenzo Pieralisi wrote:
> Hi Daniel,
> 
> thanks for this patchset.
> 
> On Wed, Jul 25, 2012 at 11:46:02AM +0100, Daniel Lezcano wrote:
>> The tegra3 and big.LITTLE architecture have different cpu latencies.
>> This API allows to specify a different cpu latency for a specific cpu.
>>
>> With the previous patches, we use the per cpuidle device states pointer,
>> this function overrides this pointer.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/cpuidle/cpuidle.c |   17 +++++++++++++++++
>>  include/linux/cpuidle.h   |   10 +++++++---
>>  2 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 199878a..3b21b68 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
>>  
>>  EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
>>  
>> +int cpuidle_register_states(struct cpuidle_device *dev,
>> +			    struct cpuidle_state *states,
>> +			    int state_count)
>> +{
>> +	if (!dev || !states)
>> +		return -EINVAL;
>> +
>> +	if (state_count <= 0)
>> +		return -EINVAL;
>> +
>> +	dev->states = states;
>> +	dev->state_count = state_count;
> 
> Is this function supposed to be called after cpuidle_device registration ?
> I think so since at registration time the dev->states pointers are all
> initialized to point to the driver state array, which is global and not
> really what we want.
> 
> Unless this function is called on the cpu that requires swapping the state
> pointer, I think it is unsafe to register a different state pointer
> without a minimal level of locking (or disabling idle and renabling idle)
> since the update of dev->states and dev->state_count is not atomic.
> Maybe it is implicit but it should be documented somehow to define
> cpuidle_register_states(...) proper usage.

Hi Lorenzo,

Yes, you are right. I will add the cpuidle lock.

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