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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.