diff mbox

cpufreq: Initialize the governor again while restoring policy

Message ID 20150709051024.GJ1805@linux
State New
Headers show

Commit Message

Viresh Kumar July 9, 2015, 5:10 a.m. UTC 
On 09-07-15, 02:33, Rafael J. Wysocki wrote:
> > We also missed marking policy->governor as NULL while restoring the
> > policy. Because of that, we call __cpufreq_governor(CPUFREQ_GOV_LIMITS)
> 
> How exactly does that happen?

Should have mentioned that in detail, sorry for being lazy. Hopefully
this will look better:

---------------------------8<---------------------------

Message-Id: <5f17361741c009a7f0d8488f7f94bab80d9317fd.1436418101.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 8 Jul 2015 10:45:53 +0530
Subject: [PATCH V2] cpufreq: Initialize the governor again while restoring policy

When all CPUs of a policy are hot-unplugged, we EXIT the governor but
don't mark policy->governor as NULL. This was done in order to keep last
used governor's information intact in sysfs, while the CPUs are offline.

But we also marking policy->governor as NULL while restoring the policy.

Because policy->governor still points to the last governor while policy
is restored, following sequence of event happens:
- cpufreq_init_policy() called while restoring policy
- find_governor() matches last_governor string for present governors and
  returns last used governor's pointer, say ondemand. policy->governor
  already has the same address, unless the governor was removed in
  between.
- cpufreq_set_policy() is called with both old/new policies governor set
  as ondemand.
- Because governors matched, we skip governor initialization and return
  after calling __cpufreq_governor(CPUFREQ_GOV_LIMITS). Because the
  governor wasn't initialized for this policy, it returned -EBUSY.
- cpufreq_init_policy() exits the policy on this error, but doesn't
  destroy it properly (should be fixed separately).
- And so we enter a scenario where the policy isn't completely
  initialized but used.

Fix this by setting policy->governor to NULL while restoring the policy.

Reported-and-tested-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Reported-and-tested-by: "Jon Medhurst (Tixy)" <tixy@linaro.org>
Tested-by: Steven Rostedt <rostedt@goodmis.org>
Fixes: 18bf3a124ef8 ("cpufreq: Mark policy->governor = NULL for inactive policies")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2: Detailed changelog

 drivers/cpufreq/cpufreq.c | 1 +
 1 file changed, 1 insertion(+)

the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Viresh Kumar July 10, 2015, 3:38 a.m. UTC | #1 
On 10 July 2015 at 05:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

>> - Because governors matched, we skip governor initialization and return
>>   after calling __cpufreq_governor(CPUFREQ_GOV_LIMITS).
>
> But this sounds fragile in principle.  What's the benefit from skipping the
> governor initialization in that case?

So this is the case where we have changed some property of the governor,
and the governor is already initialised. We need to exit the earlier governor
and initialize the new one only when the governor is actually switched.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b612411655f9..2c22e3902e72 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1132,6 +1132,7 @@  static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 
 		down_write(&policy->rwsem);
 		policy->cpu = cpu;
+		policy->governor = NULL;
 		up_write(&policy->rwsem);
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in