One preemptive comment.
On 07/15/2014 03:47 PM, Saravana Kannan wrote:
The CPUfreq core moves the cpufreq policy ownership between CPUs when CPUs
within a cluster (CPUs sharing same policy) go ONLINE/OFFLINE. When moving
policy ownership between CPUs, it also moves the cpufreq sysfs directory
between CPUs and also fixes up the symlinks of the other CPUs in the
cluster.
Also, when all the CPUs in a cluster go OFFLINE, all the sysfs nodes and
directories are deleted, the kobject is released and the policy is freed.
And when the first CPU in a cluster comes up, the policy is reallocated and
initialized, kobject is acquired, the sysfs nodes are created or symlinked,
etc.
All these steps end up creating unnecessarily complicated code and locking.
There's no real benefit to adding/removing/moving the sysfs nodes and the
policy between CPUs. Other per CPU sysfs directories like power and cpuidle
are left alone during hotplug. So there's some precedence to what this
patch is trying to do.
This patch simplifies a lot of the code and locking by removing the
adding/removing/moving of policy/sysfs/kobj and just leaves the cpufreq
directory and policy in place irrespective of whether the CPUs are
ONLINE/OFFLINE.
Leaving the policy, sysfs and kobject in place also brings these additional
benefits:
* Faster suspend/resume
* Faster hotplug
* Sysfs file permissions maintained across hotplug
* Policy settings and governor tunables maintained across hotplug
* Cpufreq stats would be maintained across hotplug for all CPUs and can be
queried even after CPU goes OFFLINE
Tested-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
Signed-off-by: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
---
drivers/cpufreq/cpufreq.c | 388 +++++++++++++---------------------------------
1 file changed, 107 insertions(+), 281 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 62259d2..a0a2ec2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
<SNIP>
@@ -961,60 +967,58 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
}
#ifdef CONFIG_HOTPLUG_CPU
-static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
- unsigned int cpu, struct device *dev)
+static int cpufreq_change_policy_cpus(struct cpufreq_policy *policy,
+ unsigned int cpu, bool add)
{
int ret = 0;
- unsigned long flags;
+ unsigned int cpus, pcpu;
- if (has_target()) {
+ down_write(&policy->rwsem);
+
+ cpus = !cpumask_empty(policy->cpus);
+ if (has_target() && cpus) {
ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
if (ret) {
pr_err("%s: Failed to stop governor\n", __func__);
- return ret;
+ goto unlock;
}
}
<SNIP>
+ if (add)
+ cpumask_set_cpu(cpu, policy->cpus);
+ else
+ cpumask_clear_cpu(cpu, policy->cpus);
- up_write(&policy->rwsem);
+ pcpu = cpumask_first(policy->cpus);
+ if (pcpu < nr_cpu_ids && policy->cpu != pcpu) {
+ policy->last_cpu = policy->cpu;
+ policy->cpu = pcpu;
+ blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+ CPUFREQ_UPDATE_POLICY_CPU, policy);
+ }
- if (has_target()) {
+ cpus = !cpumask_empty(policy->cpus);
+ if (has_target() && cpus) {
ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
if (!ret)
ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
if (ret) {
pr_err("%s: Failed to start governor\n", __func__);
- return ret;
+ goto unlock;
}
}
<SNIP>
+ if (!cpus && cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
+ cpufreq_driver->stop_cpu(policy);
+ }
Viresh, I tried your suggestion (and my initial thought too) to combine
this as an if/else with the previous if. But the indentation got nasty
and made it hard to read. I'm sure the compiler will optimize it. So, I
would prefer to leave it this way.
- policy->governor = NULL;
+unlock:
+ up_write(&policy->rwsem);
- return policy;
+ return ret;
}
+#endif
-Saravana
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
