| Message ID | 80c4bd8c577e5da0aa63342671773be5cdc26a9a.1378012620.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
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? > 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. > (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || > (event == CPUFREQ_GOV_STOP)))) { > mutex_unlock(&cpufreq_governor_lock); > return -EBUSY; > } > > + policy->governor_busy = true; > if (event == CPUFREQ_GOV_STOP) > policy->governor_enabled = false; > else if (event == CPUFREQ_GOV_START) > @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, > ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret)) > module_put(policy->governor->owner); > > + mutex_lock(&cpufreq_governor_lock); > + policy->governor_busy = false; > + mutex_unlock(&cpufreq_governor_lock); > return ret; > } > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index d568f39..cca885d 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -76,6 +76,7 @@ struct cpufreq_policy { > struct cpufreq_governor *governor; /* see below */ > void *governor_data; > bool governor_enabled; /* governor start/stop flag */ > + bool governor_busy; > > struct work_struct update; /* if update_policy() needs to be > * called, but you're in IRQ context */ >
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 :(
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 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 :( 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.
On 2 September 2013 01:57, Rafael J. Wysocki <rjw@sisk.pl> wrote: > 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 see... I tried to do it this way but got confused again :) You fix it this time and I will understand it from that :) -- viresh
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): OK, the attached patches queued up for 3.12. Thanks, Rafael
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)) || (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || (event == CPUFREQ_GOV_STOP)))) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY; } + policy->governor_busy = true; if (event == CPUFREQ_GOV_STOP) policy->governor_enabled = false; else if (event == CPUFREQ_GOV_START) @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret)) module_put(policy->governor->owner); + mutex_lock(&cpufreq_governor_lock); + policy->governor_busy = false; + mutex_unlock(&cpufreq_governor_lock); return ret; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index d568f39..cca885d 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -76,6 +76,7 @@ struct cpufreq_policy { struct cpufreq_governor *governor; /* see below */ void *governor_data; bool governor_enabled; /* governor start/stop flag */ + bool governor_busy; struct work_struct update; /* if update_policy() needs to be * called, but you're in IRQ context */
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. 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(-)