lkml.org 
[lkml]   [2013]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
From
On 1 September 2013 18:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
>> We can't take a big lock around __cpufreq_governor() as this causes recursive
>> locking for some cases. But calls to this routine must be serialized for every
>> policy.
>
> Care to explain here why it must be serialized?

Yes, added that to the attached patches (Added reported-by too):

commit dc51771506b113b998c49c3be2db0fb88bb92153
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date: Sat Aug 31 17:48:23 2013 +0530

cpufreq: serialize calls to __cpufreq_governor()

We can't take a big lock around __cpufreq_governor() as this
causes recursive
locking for some cases. But calls to this routine must be
serialized for every
policy. Otherwise we can see some unpredictable events.

For example, consider following scenario:

__cpufreq_remove_dev()
__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
policy->governor->governor(policy, CPUFREQ_GOV_STOP);
cpufreq_governor_dbs()
case CPUFREQ_GOV_STOP:
mutex_destroy(&cpu_cdbs->timer_mutex)
cpu_cdbs->cur_policy = NULL;
<PREEMPT>
store()
__cpufreq_set_policy()
__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
policy->governor->governor(policy, CPUFREQ_GOV_LIMITS);
case CPUFREQ_GOV_LIMITS:
mutex_lock(&cpu_cdbs->timer_mutex); <-- Warning (destroyed mutex)
if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL

And so store() will eventually result in a crash cur_policy is already NULL;

Lets introduce another variable which would guarantee serialization here.

Reported-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

>> Lets introduce another variable which would guarantee serialization here.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>> drivers/cpufreq/cpufreq.c | 7 ++++++-
>> include/linux/cpufreq.h | 1 +
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index f320a20..4d5723db 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>> policy->cpu, event);
>>
>> mutex_lock(&cpufreq_governor_lock);
>> - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
>> + if (policy->governor_busy ||
>> + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
>
> Again, broken white space, but I can fix it up.

Sorry, what exactly.. Sorry couldn't understand it :(
[unhandled content-type:application/octet-stream][unhandled content-type:application/octet-stream]
\
 
 \ /
  Last update: 2013-09-01 18:21    [W:0.268 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site