2013/12/5 Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>:
> On Wednesday, December 04, 2013 04:02:18 PM viresh kumar wrote:
>> On Tuesday 03 December 2013 04:44 PM, Bjørn Mork wrote:
>> > This is effectively a revert of commit 5302c3fb2e62 ("cpufreq: Perform
>> > light-weight init/teardown during suspend/resume"), which enabled
>> > suspend/resume optimizations leaving the sysfs files in place.
>> >
>> > Errors during suspend/resume are not handled properly, leaving
>> > dead sysfs attributes in case of failures. There are are number of
>> > functions with special code for the "frozen" case, and all these
>> > need to also have special error handling.
>>
>> Can you try this please if it fixes things for you (with your patch reverted):
>>
>> From: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> Date: Wed, 4 Dec 2013 15:20:12 +0530
>> Subject: [PATCH] cpufreq: remove sysfs files for CPU which failed to come
>> back after resume
>>
>> There are cases where cpufreq_add_dev() may fail for some CPUs during resume.
>> With the current code we will still have sysfs cpufreq files for such CPUs, and
>> struct cpufreq_policy would be already freed for them. Hence any operation on
>> those sysfs files would result in kernel warnings.
>>
>> To fix this, lets remove those sysfs files or put the associated kobject in case
>> of such errors. Also, to make it simple lets remove the sysfs links from all the
>> CPUs (except policy->cpu) during suspend as that operation wouldn't result with a
>> loss of sysfs file permissions. And so we will create those links during resume
>> as well.
>>
>> Reported-by: Bjørn Mork <bjorn@xxxxxxx>
>> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>
> There's no way this can go into 3.13, even though it fixes the problem for
> Bjorn.
>
> I took the Bjorn's patch for 3.13 and this one I can queued up for 3.14,
> but for that I guess it should contain a revert of the change made by the
> Bjorn's patch.
This patch causes a s3 regression. Cc:Martin Ziegler
https://bugzilla.kernel.org/show_bug.cgi?id=66751
>
> Thanks!
>
>> ---
>> drivers/cpufreq/cpufreq.c | 61 ++++++++++++++++++++++++-----------------------
>> 1 file changed, 31 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 606224a..57c80a7 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -849,8 +849,7 @@ 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,
>> - bool frozen)
>> + unsigned int cpu, struct device *dev)
>> {
>> int ret = 0;
>> unsigned long flags;
>> @@ -881,9 +880,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
>> }
>> }
>>
>> - /* Don't touch sysfs links during light-weight init */
>> - if (!frozen)
>> - ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
>> + ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
>>
>> return ret;
>> }
>> @@ -930,6 +927,27 @@ err_free_policy:
>> return NULL;
>> }
>>
>> +static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
>> +{
>> + struct kobject *kobj;
>> + struct completion *cmp;
>> +
>> + down_read(&policy->rwsem);
>> + kobj = &policy->kobj;
>> + cmp = &policy->kobj_unregister;
>> + up_read(&policy->rwsem);
>> + kobject_put(kobj);
>> +
>> + /*
>> + * We need to make sure that the underlying kobj is
>> + * actually not referenced anymore by anybody before we
>> + * proceed with unloading.
>> + */
>> + pr_debug("waiting for dropping of refcount\n");
>> + wait_for_completion(cmp);
>> + pr_debug("wait complete\n");
>> +}
>> +
>> static void cpufreq_policy_free(struct cpufreq_policy *policy)
>> {
>> free_cpumask_var(policy->related_cpus);
>> @@ -990,7 +1008,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>> list_for_each_entry(tpolicy, &cpufreq_policy_list, policy_list) {
>> if (cpumask_test_cpu(cpu, tpolicy->related_cpus)) {
>> read_unlock_irqrestore(&cpufreq_driver_lock, flags);
>> - ret = cpufreq_add_policy_cpu(tpolicy, cpu, dev, frozen);
>> + ret = cpufreq_add_policy_cpu(tpolicy, cpu, dev);
>> up_read(&cpufreq_rwsem);
>> return ret;
>> }
>> @@ -1100,7 +1118,10 @@ err_get_freq:
>> if (cpufreq_driver->exit)
>> cpufreq_driver->exit(policy);
>> err_set_policy_cpu:
>> + if (frozen)
>> + cpufreq_policy_put_kobj(policy);
>> cpufreq_policy_free(policy);
>> +
>> nomem_out:
>> up_read(&cpufreq_rwsem);
>>
>> @@ -1122,7 +1143,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> }
>>
>> static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
>> - unsigned int old_cpu, bool frozen)
>> + unsigned int old_cpu)
>> {
>> struct device *cpu_dev;
>> int ret;
>> @@ -1130,10 +1151,6 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
>> /* first sibling now owns the new sysfs dir */
>> cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu));
>>
>> - /* Don't touch sysfs files during light-weight tear-down */
>> - if (frozen)
>> - return cpu_dev->id;
>> -
>> sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
>> ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
>> if (ret) {
>> @@ -1200,7 +1217,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>> if (!frozen)
>> sysfs_remove_link(&dev->kobj, "cpufreq");
>> } else if (cpus > 1) {
>> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu, frozen);
>> + new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu);
>> if (new_cpu >= 0) {
>> update_policy_cpu(policy, new_cpu);
>>
>> @@ -1222,8 +1239,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>> int ret;
>> unsigned long flags;
>> struct cpufreq_policy *policy;
>> - struct kobject *kobj;
>> - struct completion *cmp;
>>
>> read_lock_irqsave(&cpufreq_driver_lock, flags);
>> policy = per_cpu(cpufreq_cpu_data, cpu);
>> @@ -1253,22 +1268,8 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>> }
>> }
>>
>> - if (!frozen) {
>> - down_read(&policy->rwsem);
>> - kobj = &policy->kobj;
>> - cmp = &policy->kobj_unregister;
>> - up_read(&policy->rwsem);
>> - kobject_put(kobj);
>> -
>> - /*
>> - * We need to make sure that the underlying kobj is
>> - * actually not referenced anymore by anybody before we
>> - * proceed with unloading.
>> - */
>> - pr_debug("waiting for dropping of refcount\n");
>> - wait_for_completion(cmp);
>> - pr_debug("wait complete\n");
>> - }
>> + if (!frozen)
>> + cpufreq_policy_put_kobj(policy);
>>
>> /*
>> * Perform the ->exit() even during light-weight tear-down,
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
