Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Viresh Kumar]

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