Messages in this thread | | | Date | Tue, 12 Jan 2016 16:24:47 +0530 | From | Viresh Kumar <> | Subject | Re: [RFC PATCH 13/19] cpufreq: fix locking of policy->rwsem in cpufreq_offline_prepare |
| |
On 11-01-16, 17:35, Juri Lelli wrote: > There are paths in cpufreq_offline_prepare where policy is used, but its > rwsem is not held. > > Fix it. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > --- > drivers/cpufreq/cpufreq.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-)
I know the locking in general in cpufreq core is poor. We recently fixed lots of issues in governors ..
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 2c7cc6c73..91158b0 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1332,13 +1332,13 @@ static void cpufreq_offline_prepare(unsigned int cpu) > return; > } > > + down_write(&policy->rwsem); > if (has_target()) { > int ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP); > if (ret) > pr_err("%s: Failed to stop governor\n", __func__); > } > > - down_write(&policy->rwsem); > cpumask_clear_cpu(cpu, policy->cpus); > > if (policy_is_inactive(policy)) { > @@ -1356,12 +1356,16 @@ static void cpufreq_offline_prepare(unsigned int cpu) > /* Start governor again for active policy */ > if (!policy_is_inactive(policy)) {
Why shouldn't this be under the lock?
> if (has_target()) { > - int ret = __cpufreq_governor(policy, CPUFREQ_GOV_START); > + int ret; > + > + down_write(&policy->rwsem); > + ret = __cpufreq_governor(policy, CPUFREQ_GOV_START); > if (!ret) > ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > > if (ret) > pr_err("%s: Failed to start governor\n", __func__); > + up_write(&policy->rwsem); > } > } else if (cpufreq_driver->stop_cpu) { > cpufreq_driver->stop_cpu(policy);
And this ?
-- viresh
| |