On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote:
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 thiscauses 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 :(
The second tab is one too many, I usually write such things like this:
if (policy->governor_busy || (policy->governor_enabled && event == CPUFREQ_GOV_START) || ...
Then it is much easier to distinguish the conditional code from the condition itself.