Re: [PATCH V2 04/20] cpufreq: Keep a single path for adding managed CPUs

[Saravana Kannan <skannan@codeaurora.org>]

On 02/19/2015 03:32 AM, Viresh Kumar wrote:
> There are two cases when we may try to add already managed CPUs:
The term manages CPUs is a bit unclear/confusing. When I hear managed 
CPUs, I think CPUs that are already marked in policy->cpus and the 
governor has already been started on them.

Can you clarify this commit text please?

> - On boot, the first cpu has marked all policy->cpus managed and so we will find
>    policy for all other policy->cpus later on.
> - When a managed cpu is hotplugged out and later brought back in.
>
> Currently we manage these two with separate paths and checks. While the first
> one is detected by testing cpu against 'policy->cpus', the other one is detected
> by testing cpu against 'policy->related_cpus'.
>
> We can manage both these via a single path and there is no need to do special
> checking for the first one.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/cpufreq.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9da8ed5bdd21..d06d1241a900 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -992,6 +992,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
>   	int ret = 0;
>   	unsigned long flags;
>
> +	/* Is cpu already managed ? */
> +	if (cpumask_test_cpu(cpu, policy->cpus))
> +		return 0;
> +
>   	if (has_target()) {
>   		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
>   		if (ret) {
> @@ -1147,16 +1151,10 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>
>   	pr_debug("adding CPU %u\n", cpu);
>
> -	/* check whether a different CPU already registered this
> -	 * CPU because it is in the same boat. */
> -	policy = cpufreq_cpu_get_raw(cpu);
> -	if (unlikely(policy))
> -		return 0;
> -
>   	if (!down_read_trylock(&cpufreq_rwsem))
>   		return 0;
>
> -	/* Check if this cpu was hot-unplugged earlier and has siblings */
> +	/* Check if this cpu already has a policy to manage it */
>   	read_lock_irqsave(&cpufreq_driver_lock, flags);
>   	for_each_policy(policy) {
>   		if (cpumask_test_cpu(cpu, policy->related_cpus)) {
>

If comment text is fixed,
Acked-by: Saravana Kannan <skannan@codeaurora.org>

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project