[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160115123708.GR18603@e106622-lin>
Date: Fri, 15 Jan 2016 12:37:08 +0000
From: Juri Lelli <juri.lelli@....com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
peterz@...radead.org, rjw@...ysocki.net, mturquette@...libre.com,
steve.muckle@...aro.org, vincent.guittot@...aro.org,
morten.rasmussen@....com, dietmar.eggemann@....com
Subject: Re: [RFC PATCH 13/19] cpufreq: fix locking of policy->rwsem in
cpufreq_offline_prepare
On 12/01/16 16:24, Viresh Kumar wrote:
> 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@...ysocki.net>
> > Cc: Viresh Kumar <viresh.kumar@...aro.org>
> > Signed-off-by: Juri Lelli <juri.lelli@....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 ?
>
Right. Releasing rwsem at the end seems to work.
Best,
- Juri
Powered by blists - more mailing lists