Re: [Update][PATCH] cpufreq: Do not hold driver module references for additional policy CPUs

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, August 02, 2013 04:25:58 PM Viresh Kumar wrote:
> On 2 August 2013 10:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> @@ -908,7 +905,8 @@ static int cpufreq_add_policy_cpu(unsign
> >>         unsigned long flags;
> >>
> >>         policy = cpufreq_cpu_get(sibling);
> >
> > This can be skipped completely at this place. Caller of
> > cpufreq_add_policy_cpu() has got the policy pointer with it and so
> > can be passed. I haven't done it earlier as the impression was we need
> > to call cpufreq_cpu_get()..
> 
> And here is the fixup to do this (attached too):

Care to add a changelog?

I'll apply this on top of my $subject patch, then.

Thanks,
Rafael


> ---
>  drivers/cpufreq/cpufreq.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 46e70ae..47f2a6e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -889,21 +889,17 @@ static void cpufreq_init_policy(struct
> cpufreq_policy *policy)
>  }
> 
>  #ifdef CONFIG_HOTPLUG_CPU
> -static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
> -				  struct device *dev, bool frozen)
> +static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
> +				  unsigned int cpu, struct device *dev,
> +				  bool frozen)
>  {
> -	struct cpufreq_policy *policy;
>  	int ret = 0, has_target = !!cpufreq_driver->target;
>  	unsigned long flags;
> 
> -	policy = cpufreq_cpu_get(sibling);
> -	if (WARN_ON_ONCE(!policy))
> -		return -ENODATA;
> -
>  	if (has_target)
>  		__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> 
> -	lock_policy_rwsem_write(sibling);
> +	lock_policy_rwsem_write(policy->cpu);
> 
>  	write_lock_irqsave(&cpufreq_driver_lock, flags);
> 
> @@ -912,7 +908,7 @@ static int cpufreq_add_policy_cpu(unsigned int
> cpu, unsigned int sibling,
>  	per_cpu(cpufreq_cpu_data, cpu) = policy;
>  	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 
> -	unlock_policy_rwsem_write(sibling);
> +	unlock_policy_rwsem_write(policy->cpu);
> 
>  	if (has_target) {
>  		__cpufreq_governor(policy, CPUFREQ_GOV_START);
> @@ -923,7 +919,6 @@ static int cpufreq_add_policy_cpu(unsigned int
> cpu, unsigned int sibling,
>  	if (!frozen)
>  		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> 
> -	cpufreq_cpu_put(policy);
>  	return ret;
>  }
>  #endif
> @@ -1006,8 +1001,7 @@ static int __cpufreq_add_dev(struct device *dev,
> struct subsys_interface *sif,
>  		struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
>  		if (cp && cpumask_test_cpu(cpu, cp->related_cpus)) {
>  			read_unlock_irqrestore(&cpufreq_driver_lock, flags);
> -			return cpufreq_add_policy_cpu(cpu, sibling, dev,
> -						      frozen);
> +			return cpufreq_add_policy_cpu(cp, cpu, dev, frozen);
>  		}
>  	}
>  	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.