On 07/10/2014 08:12 AM, Bu, Yitian wrote:
>
>
>> 在 2014年7月10日，20:41，"Viresh Kumar" <viresh.kumar@xxxxxxxxxx> 写道：
>>
>> This is only relevant to implementations with multiple clusters, where clusters
>> have separate clock lines but all CPUs within a cluster share it.
>>
>> Consider a dual cluster platform with 2 cores per cluster. During suspend we
>> start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be
>> moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj.
>>
>> Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev().
>> We will recover the old policy and update policy->cpu from 3 to 2 from
>> update_policy_cpu().
>>
>> But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create
>> a link for CPU2, but would try that while bringing CPU3 online. Which will
>> report errors as CPU3 already has kobj assigned to it.
>>
>> This bug got introduced with commit 42f921a, which overlooked this scenario.
>>
>> To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a
>> cluster back. We already have update_policy_cpu() routine which can be updated
>> with this change. That would get rid of the cpufreq_nominate_new_policy_cpu() as
>> update_policy_cpu() is also called on CPU removal.
>>
>> To achieve that we add another parameter to update_policy_cpu() as cpu_dev is
>> present with both the callers.
>>
>> Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume")
>> Cc: Stable <stable@xxxxxxxxxxxxxxx> # 3.13+
>> Reported-by: Bu Yitian <ybu@xxxxxxxxxxxxxxxx>
>> Reported-by: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
>> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> ---
>> V1->V2: Move kobject_move() call to update_policy_cpu(), which makes
>> cpufreq_nominate_new_policy_cpu() almost empty. So we remove it completely.
>>
>> @Yitian: Sorry, but you need to test this again as there were enough
>> modifications in V2.
>>
>
> Sorry, I don't have latest kernel to test this patch...
> What I am using is 3.10, this patch seems too big to manually apply to 3.10.
Even though our kernel is 3.10, I believe we have pulled in all cpufreq
up to 3.14. So, if you have time, you could pull in rest of the cpufreq
changes and test it out. For our tree, maybe the v1 patch would be
sufficient?
-Saravana
>
>
>> drivers/cpufreq/cpufreq.c | 73 +++++++++++++++++++++++------------------------
>> 1 file changed, 36 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 62259d2..c81d9ec6 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1076,10 +1076,20 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>> kfree(policy);
>> }
>>
>> -static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>> +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
>> + struct device *cpu_dev)
>> {
>> + int ret;
>> +
>> if (WARN_ON(cpu == policy->cpu))
>> - return;
>> + return 0;
>> +
>> + /* Move kobject to the new policy->cpu */
>> + ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
>> + if (ret) {
>> + pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
>> + return ret;
>> + }
>>
>> down_write(&policy->rwsem);
>>
>> @@ -1090,6 +1100,8 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>>
>> blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>> CPUFREQ_UPDATE_POLICY_CPU, policy);
>> +
>> + return 0;
>> }
>>
>> static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> * by invoking update_policy_cpu().
>> */
>> if (recover_policy && cpu != policy->cpu)
>> - update_policy_cpu(policy, cpu);
>> + WARN_ON(update_policy_cpu(policy, cpu, dev));
>> else
>> policy->cpu = cpu;
>>
>> @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> return __cpufreq_add_dev(dev, sif);
>> }
>>
>> -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
>> - unsigned int old_cpu)
>> -{
>> - struct device *cpu_dev;
>> - int ret;
>> -
>> - /* first sibling now owns the new sysfs dir */
>> - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu));
>> -
>> - sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
>> - ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
>> - if (ret) {
>> - pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
>> -
>> - down_write(&policy->rwsem);
>> - cpumask_set_cpu(old_cpu, policy->cpus);
>> - up_write(&policy->rwsem);
>> -
>> - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
>> - "cpufreq");
>> -
>> - return -EINVAL;
>> - }
>> -
>> - return cpu_dev->id;
>> -}
>> -
>> static int __cpufreq_remove_dev_prepare(struct device *dev,
>> struct subsys_interface *sif)
>> {
>> unsigned int cpu = dev->id, cpus;
>> - int new_cpu, ret;
>> + int ret;
>> unsigned long flags;
>> struct cpufreq_policy *policy;
>>
>> @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>> if (cpu != policy->cpu) {
>> sysfs_remove_link(&dev->kobj, "cpufreq");
>> } else if (cpus > 1) {
>> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu);
>> - if (new_cpu >= 0) {
>> - update_policy_cpu(policy, new_cpu);
>> + /* Nominate new CPU */
>> + int new_cpu = cpumask_any_but(policy->cpus, cpu);
>> + struct device *cpu_dev = get_cpu_device(new_cpu);
>>
>> - if (!cpufreq_suspended)
>> - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
>> - __func__, new_cpu, cpu);
>> + sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
>> + ret = update_policy_cpu(policy, new_cpu, cpu_dev);
>> + if (ret) {
>> + /*
>> + * To supress compilation warning about return value of
>> + * sysfs_create_link().
>> + */
>> + int temp;
>> +
>> + /* Create link again if we failed. */
>> + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
>> + "cpufreq");
>> + return ret;
>> }
>> +
>> + if (!cpufreq_suspended)
>> + pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
>> + __func__, new_cpu, cpu);
>> } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
>> cpufreq_driver->stop_cpu(policy);
>> }
>> --
>> 2.0.0.rc2
>>
--
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 stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html