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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

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.

Thanks,
Lorenzo