diff mbox

[RFC,15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor

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

Commit Message

Juri Lelli Jan. 11, 2016, 5:35 p.m. UTC 
Commit 6f1e4efd882e ("cpufreq: Fix timer/workqueue corruption by
protecting reading governor_enabled") made policy->governor_enabled
guarded by cpufreq_governor_mutex in __cpufreq_governor. Now that
holding of policy->rwsem is asserted in __cpufreq_governor,
cpufreq_governor_mutex is overkilling.

Remove such usage. Also, this cleans up semantic of
cpufreq_governor_mutex: it guards cpufreq_governor_list only.

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 | 6 ------
 1 file changed, 6 deletions(-)

-- 
2.2.2

Comments

Viresh Kumar Jan. 12, 2016, 11:06 a.m. UTC | #1 
On 11-01-16, 17:35, Juri Lelli wrote:
> Commit 6f1e4efd882e ("cpufreq: Fix timer/workqueue corruption by

> protecting reading governor_enabled") made policy->governor_enabled

> guarded by cpufreq_governor_mutex in __cpufreq_governor. Now that

> holding of policy->rwsem is asserted in __cpufreq_governor,

> cpufreq_governor_mutex is overkilling.


I am sure that is going to break it. Try that x86, somehow I don't get
it on my exynos boards.

> -	mutex_lock(&cpufreq_governor_mutex);

>  	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)

>  	    || (!policy->governor_enabled

>  	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {

> -		mutex_unlock(&cpufreq_governor_mutex);

>  		return -EBUSY;

>  	}


Actually the above checks should also be removed as the governors are
responsible for maintaining their state machines. But
userspace/powersave/performance don't have that support yet and so
these checks save them from going into undefined states.

Over that, above and below checks are incomplete..

> @@ -2006,8 +2004,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

>  	else if (event == CPUFREQ_GOV_START)

>  		policy->governor_enabled = true;

>  

> -	mutex_unlock(&cpufreq_governor_mutex);

> -

>  	ret = policy->governor->governor(policy, event);

>  

>  	if (!ret) {

> @@ -2017,12 +2013,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

>  			policy->governor->initialized--;

>  	} else {

>  		/* Restore original values */

> -		mutex_lock(&cpufreq_governor_mutex);

>  		if (event == CPUFREQ_GOV_STOP)

>  			policy->governor_enabled = true;

>  		else if (event == CPUFREQ_GOV_START)

>  			policy->governor_enabled = false;

> -		mutex_unlock(&cpufreq_governor_mutex);

>  	}

>  

>  	if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||

> -- 

> 2.2.2


-- 
viresh
Juri Lelli Jan. 15, 2016, 4:30 p.m. UTC | #2 
Hi,

On 12/01/16 16:36, Viresh Kumar wrote:
> On 11-01-16, 17:35, Juri Lelli wrote:

> > Commit 6f1e4efd882e ("cpufreq: Fix timer/workqueue corruption by

> > protecting reading governor_enabled") made policy->governor_enabled

> > guarded by cpufreq_governor_mutex in __cpufreq_governor. Now that

> > holding of policy->rwsem is asserted in __cpufreq_governor,

> > cpufreq_governor_mutex is overkilling.

> 

> I am sure that is going to break it. Try that x86, somehow I don't get

> it on my exynos boards.

> 


But governor_enabled seems to not be checked anymore outside cpufreq.c
(see also 01/19), as it was in the commit you are referring to. Now that
users of this should be holding policy->rwsem, so that should suffice
for protecting governor_enabled, as governor_enabled is only changed
inside here.

I run some test on a x86 box I setup and didn't see anything related to
this. I'll wait to get the first 0-day report anyway.

> > -	mutex_lock(&cpufreq_governor_mutex);

> >  	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)

> >  	    || (!policy->governor_enabled

> >  	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {

> > -		mutex_unlock(&cpufreq_governor_mutex);

> >  		return -EBUSY;

> >  	}

> 

> Actually the above checks should also be removed as the governors are

> responsible for maintaining their state machines. But

> userspace/powersave/performance don't have that support yet and so

> these checks save them from going into undefined states.

> 

> Over that, above and below checks are incomplete..

> 


You mean we need an additional patch that extends the checks performed?

Thanks,

- Juri

> > @@ -2006,8 +2004,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

> >  	else if (event == CPUFREQ_GOV_START)

> >  		policy->governor_enabled = true;

> >  

> > -	mutex_unlock(&cpufreq_governor_mutex);

> > -

> >  	ret = policy->governor->governor(policy, event);

> >  

> >  	if (!ret) {

> > @@ -2017,12 +2013,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

> >  			policy->governor->initialized--;

> >  	} else {

> >  		/* Restore original values */

> > -		mutex_lock(&cpufreq_governor_mutex);

> >  		if (event == CPUFREQ_GOV_STOP)

> >  			policy->governor_enabled = true;

> >  		else if (event == CPUFREQ_GOV_START)

> >  			policy->governor_enabled = false;

> > -		mutex_unlock(&cpufreq_governor_mutex);

> >  	}

> >  

> >  	if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||

> > -- 

> > 2.2.2

> 

> -- 

> viresh

>
Viresh Kumar Jan. 20, 2016, 10:18 a.m. UTC | #3 
On 20-01-16, 10:17, Juri Lelli wrote:
> On 20/01/16 12:59, Viresh Kumar wrote:

> > On 19-01-16, 16:49, Juri Lelli wrote:

> > > I'm actually hitting this running sp2, on linux-pm/linux-next :/.

> > 

> > That's really bad .. Are you hitting this on Juno or x86 ?

> > 

> 

> That's on TC2. I'll try to run the same on Juno and x86.


Juno will be the same as that also set the per-policy-governor thing
:)

-- 
viresh
Viresh Kumar Jan. 20, 2016, 10:30 a.m. UTC | #4 
On 20-01-16, 10:27, Juri Lelli wrote:
> That is what I expect as well, but you never know ;).


Perhaps not. We are talking about policy->rwsem here being used while
reading the values of governor's sysfs files. And that lock is only
taken for single governor case.

-- 
viresh
diff mbox

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ba452c3..d58a622 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1993,11 +1993,9 @@  static int __cpufreq_governor(struct cpufreq_policy *policy,
 
 	pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);
 
-	mutex_lock(&cpufreq_governor_mutex);
 	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
 	    || (!policy->governor_enabled
 	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
-		mutex_unlock(&cpufreq_governor_mutex);
 		return -EBUSY;
 	}
 
@@ -2006,8 +2004,6 @@  static int __cpufreq_governor(struct cpufreq_policy *policy,
 	else if (event == CPUFREQ_GOV_START)
 		policy->governor_enabled = true;
 
-	mutex_unlock(&cpufreq_governor_mutex);
-
 	ret = policy->governor->governor(policy, event);
 
 	if (!ret) {
@@ -2017,12 +2013,10 @@  static int __cpufreq_governor(struct cpufreq_policy *policy,
 			policy->governor->initialized--;
 	} else {
 		/* Restore original values */
-		mutex_lock(&cpufreq_governor_mutex);
 		if (event == CPUFREQ_GOV_STOP)
 			policy->governor_enabled = true;
 		else if (event == CPUFREQ_GOV_START)
 			policy->governor_enabled = false;
-		mutex_unlock(&cpufreq_governor_mutex);
 	}
 
 	if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||