Messages in this thread | | | Date | Thu, 12 Sep 2013 12:10:17 +0530 | From | "Srivatsa S. Bhat" <> | Subject | Re: [PATCH 5/5] cpufreq: use correct values of cpus in __cpufreq_remove_dev_finish() |
| |
On 09/12/2013 10:55 AM, Viresh Kumar wrote: > This broke after a recent change "cedb70a cpufreq: Split __cpufreq_remove_dev() > into two parts" from Srivatsa.. > > Consider a scenario where we have two CPUs in a policy (0 & 1) and we are > removing cpu 1. On the call to __cpufreq_remove_dev_prepare() we have cleared 1 > from policy->cpus and now on a call to __cpufreq_remove_dev_finish() we read > cpumask_weight of policy->cpus, which will come as 1 and this code will behave > as if we are removing the last cpu from policy :) > > Fix it by clearing cpu mask in __cpufreq_remove_dev_finish() instead of > __cpufreq_remove_dev_prepare(). >
Oops! Good catch!
That said, your fix doesn't look correct. See below.
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 0e11fcb..b556d46 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1175,12 +1175,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, > policy->governor->name, CPUFREQ_NAME_LEN); > #endif > > - WARN_ON(lock_policy_rwsem_write(cpu)); > + lock_policy_rwsem_read(cpu); > cpus = cpumask_weight(policy->cpus); > - > - if (cpus > 1) > - cpumask_clear_cpu(cpu, policy->cpus); > - unlock_policy_rwsem_write(cpu); > + unlock_policy_rwsem_read(cpu); > > if (cpu != policy->cpu) { > if (!frozen)
Around here, we call cpufreq_nominate_new_policy_cpu(), and if we haven't cleared the CPU by then, there is a chance that it will nominate the same CPU that we are taking offline. So its important to clear the CPU before that point.
> @@ -1222,9 +1219,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev, > return -EINVAL; > } > > - lock_policy_rwsem_read(cpu); > + WARN_ON(lock_policy_rwsem_write(cpu)); > cpus = cpumask_weight(policy->cpus); > - unlock_policy_rwsem_read(cpu); > + > + if (cpus > 1) > + cpumask_clear_cpu(cpu, policy->cpus); > + unlock_policy_rwsem_write(cpu); >
Perhaps we can retain the above as a read operation, ...
> /* If cpu is last user of policy, free policy */ > if (cpus == 1) { > ... and change this suitably (from 1 to 0 etc..) ? To add to it, it will look more clear as well:
if (cpus == 0) { /* No cpus in policy, so free it */ } else { /* Restart governor */ }
Regards, Srivatsa S. Bhat
| |