On 1 September 2013 18:58, Rafael J. Wysocki <rjw@xxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxx>
Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> Lets introduce another variable which would guarantee serialization here.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> ---
>> 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 :(
Attachment:
0001-cpufreq-don-t-allow-governor-limits-to-be-changed-wh.patch
Description: Binary data
Attachment:
0002-cpufreq-serialize-calls-to-__cpufreq_governor.patch
Description: Binary data
