[RFC,13/19] cpufreq: fix locking of policy->rwsem in cpufreq_offline_prepare

Message ID 1452533760-13787-14-git-send-email-juri.lelli@arm.com
State New
Headers show

Commit Message

Juri Lelli Jan. 11, 2016, 5:35 p.m.
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(-)

-- 
2.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Juri Lelli Jan. 15, 2016, 12:37 p.m. | #1
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@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 ?

> 


Right. Releasing rwsem at the end seems to work.

Best,

- Juri
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)) {
 		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);