Messages in this thread Patch in this message | | | Date | Sun, 1 Sep 2013 21:30:49 +0530 | Subject | Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor() | From | Viresh Kumar <> |
| |
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] | |