On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote: > 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 :( 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. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
