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.
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
