Hi,
Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
some subtle regressions in the cpufreq subsystem during suspend/resume.
This patchset is aimed at rectifying those problems, by fixing the regression
as well as achieving the original goal of that commit in a proper way.
Patch 1 reverts the above commit, and is CC'ed to stable.
Patches 2 - 5 reorganize the code and have no functional impact, and can go
in as general cleanups as well. This reorganization builds a base that the
rest of the patches will make use of.
Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
across suspend/resume.
All the patches apply on current mainline.
Robert, Durgadoss, it would be great if you could try it out and see if it works
well for your usecase. I tested it locally and cpufreq related files did retain
their permissions across suspend/resume. Let me know if it works fine in your
setup too.
And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
whether their systems work fine after:
a. applying only the first commit (this is what gets backported to stable)
b. applying all the commits
(Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
testing this patchset. Though that patch also touches cpufreq subsystem, it
doesn't affect this patchset in any way and there is absolutely no dependency
between the two in terms of code. That fix just makes basic CPU hotplug work
without locking up on current mainline).
[1]. https://lkml.org/lkml/2013/7/10/611
Thank you very much!
Srivatsa S. Bhat (8):
cpufreq: Revert commit a66b2e to fix cpufreq regression during suspend/resume
cpufreq: Fix misplaced call to cpufreq_update_policy()
cpufreq: Add helper to perform alloc/free of policy structure
cpufreq: Extract non-interface related stuff from cpufreq_add_dev_interface
cpufreq: Extract the handover of policy cpu to a helper function
cpufreq: Introduce a flag ('frozen') to separate full vs temporary init/teardown
cpufreq: Preserve policy structure across suspend/resume
cpufreq: Perform light-weight init/teardown during suspend/resume
drivers/cpufreq/cpufreq.c | 297 ++++++++++++++++++++++++++-------------
drivers/cpufreq/cpufreq_stats.c | 10 -
2 files changed, 199 insertions(+), 108 deletions(-)
Thanks,
Srivatsa S. Bhat
IBM Linux Technology Center
commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) has
unfortunately caused several things in the cpufreq subsystem to break subtly
after a suspend/resume cycle.
The intention of that patch was to retain the file permissions of the
cpufreq related sysfs files across suspend/resume. To achieve that, the commit
completely removed the calls to cpufreq_add_dev() and __cpufreq_remove_dev()
during suspend/resume transitions. But the problem is that those functions
do 2 kinds of things:
1. Low-level initialization/tear-down that are critical to the correct
functioning of cpufreq-core.
2. Kobject and sysfs related initialization/teardown.
Ideally we should have reorganized the code to cleanly separate these two
responsibilities, and skipped only the sysfs related parts during
suspend/resume. Since we skipped the entire callbacks instead (which also
included some CPU and cpufreq-specific critical components), cpufreq
subsystem started behaving erratically after suspend/resume.
So revert the commit to fix the regression. We'll revisit and address the
original goal of that commit separately, since it involves quite a bit of
careful code reorganization and appears to be non-trivial.
(While reverting the commit, note that another commit f51e1eb "cpufreq:
Fix cpufreq regression after suspend/resume" already reverted part of the
original set of changes. So revert only the remaining ones).
Cc: [email protected]
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 4 +++-
drivers/cpufreq/cpufreq_stats.c | 6 ++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6a015ad..ccc6eab 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1941,13 +1941,15 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
if (dev) {
switch (action) {
case CPU_ONLINE:
+ case CPU_ONLINE_FROZEN:
cpufreq_add_dev(dev, NULL);
break;
case CPU_DOWN_PREPARE:
- case CPU_UP_CANCELED_FROZEN:
+ case CPU_DOWN_PREPARE_FROZEN:
__cpufreq_remove_dev(dev, NULL);
break;
case CPU_DOWN_FAILED:
+ case CPU_DOWN_FAILED_FROZEN:
cpufreq_add_dev(dev, NULL);
break;
}
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index cd9e817..12225d1 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -353,13 +353,11 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
cpufreq_update_policy(cpu);
break;
case CPU_DOWN_PREPARE:
+ case CPU_DOWN_PREPARE_FROZEN:
cpufreq_stats_free_sysfs(cpu);
break;
case CPU_DEAD:
- cpufreq_stats_free_table(cpu);
- break;
- case CPU_UP_CANCELED_FROZEN:
- cpufreq_stats_free_sysfs(cpu);
+ case CPU_DEAD_FROZEN:
cpufreq_stats_free_table(cpu);
break;
}
The call to cpufreq_update_policy() is placed in the CPU hotplug callback
of cpufreq_stats, which has a higher priority than the CPU hotplug callback
of cpufreq-core. As a result, during CPU_ONLINE/CPU_ONLINE_FROZEN, we end up
calling cpufreq_update_policy() *before* calling cpufreq_add_dev() !
And for uninitialized CPUs, it just returns silently, not doing anything.
To add to it, cpufreq_stats is not even the right place to call
cpufreq_update_policy() to begin with. The cpufreq core ought to handle
this in its own callback, from an elegance/relevance perspective.
So move the invocation of cpufreq_update_policy() to cpufreq_cpu_callback,
and place it *after* cpufreq_add_dev().
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 1 +
drivers/cpufreq/cpufreq_stats.c | 6 ------
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ccc6eab..f8c3100 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1943,6 +1943,7 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
case CPU_ONLINE:
case CPU_ONLINE_FROZEN:
cpufreq_add_dev(dev, NULL);
+ cpufreq_update_policy(cpu);
break;
case CPU_DOWN_PREPARE:
case CPU_DOWN_PREPARE_FROZEN:
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 12225d1..a3e7475 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -348,10 +348,6 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
unsigned int cpu = (unsigned long)hcpu;
switch (action) {
- case CPU_ONLINE:
- case CPU_ONLINE_FROZEN:
- cpufreq_update_policy(cpu);
- break;
case CPU_DOWN_PREPARE:
case CPU_DOWN_PREPARE_FROZEN:
cpufreq_stats_free_sysfs(cpu);
@@ -390,8 +386,6 @@ static int __init cpufreq_stats_init(void)
return ret;
register_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
- for_each_online_cpu(cpu)
- cpufreq_update_policy(cpu);
ret = cpufreq_register_notifier(¬ifier_trans_block,
CPUFREQ_TRANSITION_NOTIFIER);
Separate out the allocation of the cpufreq policy structure (along with
its error handling) to a helper function. This makes the code easier to
read and also helps with some upcoming code reorganization.
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 50 ++++++++++++++++++++++++++++++++-------------
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f8c3100..ca14dc2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -943,6 +943,37 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
}
#endif
+static struct cpufreq_policy *cpufreq_policy_alloc(void)
+{
+ struct cpufreq_policy *policy;
+
+ policy = kzalloc(sizeof(struct cpufreq_policy), GFP_KERNEL);
+ if (!policy)
+ return NULL;
+
+ if (!alloc_cpumask_var(&policy->cpus, GFP_KERNEL))
+ goto err_free_policy;
+
+ if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL))
+ goto err_free_cpumask;
+
+ return policy;
+
+err_free_cpumask:
+ free_cpumask_var(policy->cpus);
+err_free_policy:
+ kfree(policy);
+
+ return NULL;
+}
+
+static void cpufreq_policy_free(struct cpufreq_policy *policy)
+{
+ free_cpumask_var(policy->related_cpus);
+ free_cpumask_var(policy->cpus);
+ kfree(policy);
+}
+
/**
* cpufreq_add_dev - add a CPU device
*
@@ -996,16 +1027,10 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
goto module_out;
}
- policy = kzalloc(sizeof(struct cpufreq_policy), GFP_KERNEL);
+ policy = cpufreq_policy_alloc();
if (!policy)
goto nomem_out;
- if (!alloc_cpumask_var(&policy->cpus, GFP_KERNEL))
- goto err_free_policy;
-
- if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL))
- goto err_free_cpumask;
-
policy->cpu = cpu;
policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
cpumask_copy(policy->cpus, cpumask_of(cpu));
@@ -1070,11 +1095,7 @@ err_out_unregister:
err_set_policy_cpu:
per_cpu(cpufreq_policy_cpu, cpu) = -1;
- free_cpumask_var(policy->related_cpus);
-err_free_cpumask:
- free_cpumask_var(policy->cpus);
-err_free_policy:
- kfree(policy);
+ cpufreq_policy_free(policy);
nomem_out:
module_put(cpufreq_driver->owner);
module_out:
@@ -1201,9 +1222,8 @@ static int __cpufreq_remove_dev(struct device *dev,
if (cpufreq_driver->exit)
cpufreq_driver->exit(data);
- free_cpumask_var(data->related_cpus);
- free_cpumask_var(data->cpus);
- kfree(data);
+ cpufreq_policy_free(data);
+
} else if (cpufreq_driver->target) {
__cpufreq_governor(data, CPUFREQ_GOV_START);
__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
During cpu offline, when the policy->cpu is going down, some other CPU
present in the policy->cpus mask is nominated as the new policy->cpu.
Extract this functionality from __cpufreq_remove_dev() and implement
it in a helper function. This helps in upcoming code reorganization.
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 62 ++++++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 24 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2ab39f3..efdb607 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1128,6 +1128,38 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
CPUFREQ_UPDATE_POLICY_CPU, policy);
}
+static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *data,
+ unsigned int old_cpu)
+{
+ struct device *cpu_dev;
+ unsigned long flags;
+ int ret;
+
+ /* first sibling now owns the new sysfs dir */
+ cpu_dev = get_cpu_device(cpumask_first(data->cpus));
+ sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
+ ret = kobject_move(&data->kobj, &cpu_dev->kobj);
+ if (ret) {
+ pr_err("%s: Failed to move kobj: %d", __func__, ret);
+
+ WARN_ON(lock_policy_rwsem_write(old_cpu));
+ cpumask_set_cpu(old_cpu, data->cpus);
+
+ write_lock_irqsave(&cpufreq_driver_lock, flags);
+ per_cpu(cpufreq_cpu_data, old_cpu) = data;
+ write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+ unlock_policy_rwsem_write(old_cpu);
+
+ ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
+ "cpufreq");
+
+ return -EINVAL;
+ }
+
+ return cpu_dev->id;
+}
+
/**
* __cpufreq_remove_dev - remove a CPU device
*
@@ -1138,12 +1170,11 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
static int __cpufreq_remove_dev(struct device *dev,
struct subsys_interface *sif)
{
- unsigned int cpu = dev->id, ret, cpus;
+ unsigned int cpu = dev->id, cpus, new_cpu;
unsigned long flags;
struct cpufreq_policy *data;
struct kobject *kobj;
struct completion *cmp;
- struct device *cpu_dev;
pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
@@ -1178,32 +1209,15 @@ static int __cpufreq_remove_dev(struct device *dev,
if (cpu != data->cpu) {
sysfs_remove_link(&dev->kobj, "cpufreq");
} else if (cpus > 1) {
- /* first sibling now owns the new sysfs dir */
- cpu_dev = get_cpu_device(cpumask_first(data->cpus));
- sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
- ret = kobject_move(&data->kobj, &cpu_dev->kobj);
- if (ret) {
- pr_err("%s: Failed to move kobj: %d", __func__, ret);
+ new_cpu = cpufreq_nominate_new_policy_cpu(data, cpu);
+ if (new_cpu >= 0) {
WARN_ON(lock_policy_rwsem_write(cpu));
- cpumask_set_cpu(cpu, data->cpus);
-
- write_lock_irqsave(&cpufreq_driver_lock, flags);
- per_cpu(cpufreq_cpu_data, cpu) = data;
- write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
+ update_policy_cpu(data, new_cpu);
unlock_policy_rwsem_write(cpu);
-
- ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
- "cpufreq");
- return -EINVAL;
+ pr_debug("%s: policy Kobject moved to cpu: %d "
+ "from: %d\n",__func__, new_cpu, cpu);
}
-
- WARN_ON(lock_policy_rwsem_write(cpu));
- update_policy_cpu(data, cpu_dev->id);
- unlock_policy_rwsem_write(cpu);
- pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
- __func__, cpu_dev->id, cpu);
}
if ((cpus == 1) && (cpufreq_driver->target))
During suspend/resume we would like to do a light-weight init/teardown of
CPUs in the cpufreq subsystem and preserve certain things such as sysfs files
etc across suspend/resume transitions. Add a flag called 'frozen' to help
distinguish the full init/teardown sequence from the light-weight one.
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 66 ++++++++++++++++++++++++++++-----------------
1 file changed, 41 insertions(+), 25 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index efdb607..1128753 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -902,7 +902,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
#ifdef CONFIG_HOTPLUG_CPU
static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
- struct device *dev)
+ struct device *dev, bool frozen)
{
struct cpufreq_policy *policy;
int ret = 0, has_target = !!cpufreq_driver->target;
@@ -930,13 +930,15 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
}
+ /* Don't touch sysfs links during light-weight init */
+ if (frozen)
+ return 0;
+
ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
- if (ret) {
+ if (ret)
cpufreq_cpu_put(policy);
- return ret;
- }
- return 0;
+ return ret;
}
#endif
@@ -971,16 +973,8 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
kfree(policy);
}
-/**
- * cpufreq_add_dev - add a CPU device
- *
- * Adds the cpufreq interface for a CPU device.
- *
- * The Oracle says: try running cpufreq registration/unregistration concurrently
- * with with cpu hotplugging and all hell will break loose. Tried to clean this
- * mess up, but more thorough testing is needed. - Mathieu
- */
-static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
+static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
+ bool frozen)
{
unsigned int j, cpu = dev->id;
int ret = -ENOMEM;
@@ -1012,7 +1006,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
if (cp && cpumask_test_cpu(cpu, cp->related_cpus)) {
read_unlock_irqrestore(&cpufreq_driver_lock, flags);
- return cpufreq_add_policy_cpu(cpu, sibling, dev);
+ return cpufreq_add_policy_cpu(cpu, sibling, dev,
+ frozen);
}
}
read_unlock_irqrestore(&cpufreq_driver_lock, flags);
@@ -1078,9 +1073,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
}
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
- ret = cpufreq_add_dev_interface(cpu, policy, dev);
- if (ret)
- goto err_out_unregister;
+ if (!frozen) {
+ ret = cpufreq_add_dev_interface(cpu, policy, dev);
+ if (ret)
+ goto err_out_unregister;
+ }
cpufreq_init_policy(policy);
@@ -1111,6 +1108,20 @@ module_out:
return ret;
}
+/**
+ * cpufreq_add_dev - add a CPU device
+ *
+ * Adds the cpufreq interface for a CPU device.
+ *
+ * The Oracle says: try running cpufreq registration/unregistration concurrently
+ * with with cpu hotplugging and all hell will break loose. Tried to clean this
+ * mess up, but more thorough testing is needed. - Mathieu
+ */
+static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
+{
+ return __cpufreq_add_dev(dev, sif, false);
+}
+
static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
{
int j;
@@ -1129,7 +1140,7 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
}
static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *data,
- unsigned int old_cpu)
+ unsigned int old_cpu, bool frozen)
{
struct device *cpu_dev;
unsigned long flags;
@@ -1137,6 +1148,11 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *data,
/* first sibling now owns the new sysfs dir */
cpu_dev = get_cpu_device(cpumask_first(data->cpus));
+
+ /* 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(&data->kobj, &cpu_dev->kobj);
if (ret) {
@@ -1168,7 +1184,7 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *data,
* This routine frees the rwsem before returning.
*/
static int __cpufreq_remove_dev(struct device *dev,
- struct subsys_interface *sif)
+ struct subsys_interface *sif, bool frozen)
{
unsigned int cpu = dev->id, cpus, new_cpu;
unsigned long flags;
@@ -1206,11 +1222,11 @@ static int __cpufreq_remove_dev(struct device *dev,
cpumask_clear_cpu(cpu, data->cpus);
unlock_policy_rwsem_write(cpu);
- if (cpu != data->cpu) {
+ if (cpu != data->cpu && !frozen) {
sysfs_remove_link(&dev->kobj, "cpufreq");
} else if (cpus > 1) {
- new_cpu = cpufreq_nominate_new_policy_cpu(data, cpu);
+ new_cpu = cpufreq_nominate_new_policy_cpu(data, cpu, frozen);
if (new_cpu >= 0) {
WARN_ON(lock_policy_rwsem_write(cpu));
update_policy_cpu(data, new_cpu);
@@ -1264,7 +1280,7 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
if (cpu_is_offline(cpu))
return 0;
- retval = __cpufreq_remove_dev(dev, sif);
+ retval = __cpufreq_remove_dev(dev, sif, false);
return retval;
}
@@ -1990,7 +2006,7 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
break;
case CPU_DOWN_PREPARE:
case CPU_DOWN_PREPARE_FROZEN:
- __cpufreq_remove_dev(dev, NULL);
+ __cpufreq_remove_dev(dev, NULL, false);
break;
case CPU_DOWN_FAILED:
case CPU_DOWN_FAILED_FROZEN:
To perform light-weight cpu-init and teardown in the cpufreq subsystem
during suspend/resume, we need to separate out the 2 main functionalities
of the cpufreq CPU hotplug callbacks, as outlined below:
1. Init/tear-down of core cpufreq and CPU-specific components, which are
critical to the correct functioning of the cpufreq subsystem.
2. Init/tear-down of cpufreq sysfs files during suspend/resume.
The first part requires accurate updates to the policy structure such as
its ->cpus and ->related_cpus masks, whereas the second part requires that
the policy->kobj structure is not released or re-initialized during
suspend/resume.
To handle both these requirements, we need to allow updates to the policy
structure throughout suspend/resume, but prevent the structure from getting
freed up. Also, we must have a mechanism by which the cpu-up callbacks can
restore the policy structure, without allocating things afresh. (That also
helps avoid memory leaks).
To achieve this, we use 2 schemes:
a. Use a fallback per-cpu storage area for preserving the policy structures
during suspend, so that they can be restored during resume appropriately.
b. Use the 'frozen' flag to determine when to free or allocate the policy
structure vs when to restore the policy from the saved fallback storage.
Thus we can successfully preserve the structure across suspend/resume.
Effectively, this helps us complete the separation of the 'light-weight'
and the 'full' init/tear-down sequences in the cpufreq subsystem, so that
this can be made use of in the suspend/resume scenario.
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 69 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 52 insertions(+), 17 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1128753..15ced5f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -44,6 +44,7 @@
*/
static struct cpufreq_driver *cpufreq_driver;
static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
+static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);
static DEFINE_RWLOCK(cpufreq_driver_lock);
static DEFINE_MUTEX(cpufreq_governor_lock);
@@ -942,6 +943,20 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
}
#endif
+static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
+{
+ struct cpufreq_policy *policy;
+ unsigned long flags;
+
+ write_lock_irqsave(&cpufreq_driver_lock, flags);
+
+ policy = per_cpu(cpufreq_cpu_data_fallback, cpu);
+
+ write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+ return policy;
+}
+
static struct cpufreq_policy *cpufreq_policy_alloc(void)
{
struct cpufreq_policy *policy;
@@ -1019,7 +1034,12 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
goto module_out;
}
- policy = cpufreq_policy_alloc();
+ if (frozen)
+ /* Restore the saved policy when doing light-weight init */
+ policy = cpufreq_policy_restore(cpu);
+ else
+ policy = cpufreq_policy_alloc();
+
if (!policy)
goto nomem_out;
@@ -1199,6 +1219,10 @@ static int __cpufreq_remove_dev(struct device *dev,
data = per_cpu(cpufreq_cpu_data, cpu);
per_cpu(cpufreq_cpu_data, cpu) = NULL;
+ /* Save the policy somewhere when doing a light-weight tear-down */
+ if (frozen)
+ per_cpu(cpufreq_cpu_data_fallback, cpu) = data;
+
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
if (!data) {
@@ -1239,29 +1263,40 @@ static int __cpufreq_remove_dev(struct device *dev,
if ((cpus == 1) && (cpufreq_driver->target))
__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
- pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
- cpufreq_cpu_put(data);
+ if (!frozen) {
+ pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
+ cpufreq_cpu_put(data);
+ }
/* If cpu is last user of policy, free policy */
if (cpus == 1) {
- lock_policy_rwsem_read(cpu);
- kobj = &data->kobj;
- cmp = &data->kobj_unregister;
- unlock_policy_rwsem_read(cpu);
- 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) {
+ lock_policy_rwsem_read(cpu);
+ kobj = &data->kobj;
+ cmp = &data->kobj_unregister;
+ unlock_policy_rwsem_read(cpu);
+ 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");
+ }
+ /*
+ * Perform the ->exit() even during light-weight tear-down,
+ * since this is a core component, and is essential for the
+ * the subsequent light-weight ->init() to succeed.
+ */
if (cpufreq_driver->exit)
cpufreq_driver->exit(data);
- cpufreq_policy_free(data);
+ if (!frozen)
+ cpufreq_policy_free(data);
} else if (cpufreq_driver->target) {
__cpufreq_governor(data, CPUFREQ_GOV_START);
Now that we have the infrastructure to perform a light-weight init/tear-down,
use that in the cpufreq CPU hotplug notifier when invoked from the
suspend/resume path.
This also ensures that the file permissions of the cpufreq sysfs files are
preserved across suspend/resume, something which commit a66b2e (cpufreq:
Preserve sysfs files across suspend/resume) originally intended to do, but
had to be reverted due to other problems.
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 18 +++++++++++-------
drivers/cpufreq/cpufreq_stats.c | 2 --
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 15ced5f..c7f59e8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2030,22 +2030,26 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
{
unsigned int cpu = (unsigned long)hcpu;
struct device *dev;
+ bool frozen = false;
dev = get_cpu_device(cpu);
if (dev) {
- switch (action) {
+
+ if (action & CPU_TASKS_FROZEN)
+ frozen = true;
+
+ switch (action & ~CPU_TASKS_FROZEN) {
case CPU_ONLINE:
- case CPU_ONLINE_FROZEN:
- cpufreq_add_dev(dev, NULL);
+ __cpufreq_add_dev(dev, NULL, frozen);
cpufreq_update_policy(cpu);
break;
+
case CPU_DOWN_PREPARE:
- case CPU_DOWN_PREPARE_FROZEN:
- __cpufreq_remove_dev(dev, NULL, false);
+ __cpufreq_remove_dev(dev, NULL, frozen);
break;
+
case CPU_DOWN_FAILED:
- case CPU_DOWN_FAILED_FROZEN:
- cpufreq_add_dev(dev, NULL);
+ __cpufreq_add_dev(dev, NULL, frozen);
break;
}
}
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index a3e7475..4e1eb3f 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -349,11 +349,9 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
switch (action) {
case CPU_DOWN_PREPARE:
- case CPU_DOWN_PREPARE_FROZEN:
cpufreq_stats_free_sysfs(cpu);
break;
case CPU_DEAD:
- case CPU_DEAD_FROZEN:
cpufreq_stats_free_table(cpu);
break;
}
cpufreq_add_dev_interface() includes the work of exposing the interface
to the device, as well as a lot of unrelated stuff. Move the latter to
cpufreq_add_dev(), where it is more appropriate.
Signed-off-by: Srivatsa S. Bhat <[email protected]>
---
drivers/cpufreq/cpufreq.c | 43 ++++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ca14dc2..2ab39f3 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -834,11 +834,8 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
struct cpufreq_policy *policy,
struct device *dev)
{
- struct cpufreq_policy new_policy;
struct freq_attr **drv_attr;
- unsigned long flags;
int ret = 0;
- unsigned int j;
/* prepare interface data */
ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
@@ -870,17 +867,23 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
goto err_out_kobj_put;
}
- write_lock_irqsave(&cpufreq_driver_lock, flags);
- for_each_cpu(j, policy->cpus) {
- per_cpu(cpufreq_cpu_data, j) = policy;
- per_cpu(cpufreq_policy_cpu, j) = policy->cpu;
- }
- write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
ret = cpufreq_add_dev_symlink(cpu, policy);
if (ret)
goto err_out_kobj_put;
+ return ret;
+
+err_out_kobj_put:
+ kobject_put(&policy->kobj);
+ wait_for_completion(&policy->kobj_unregister);
+ return ret;
+}
+
+static void cpufreq_init_policy(struct cpufreq_policy *policy)
+{
+ struct cpufreq_policy new_policy;
+ int ret = 0;
+
memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
/* assure that the starting sequence is run in __cpufreq_set_policy */
policy->governor = NULL;
@@ -895,12 +898,6 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
if (cpufreq_driver->exit)
cpufreq_driver->exit(policy);
}
- return ret;
-
-err_out_kobj_put:
- kobject_put(&policy->kobj);
- wait_for_completion(&policy->kobj_unregister);
- return ret;
}
#ifdef CONFIG_HOTPLUG_CPU
@@ -1074,10 +1071,19 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
}
#endif
+ write_lock_irqsave(&cpufreq_driver_lock, flags);
+ for_each_cpu(j, policy->cpus) {
+ per_cpu(cpufreq_cpu_data, j) = policy;
+ per_cpu(cpufreq_policy_cpu, j) = policy->cpu;
+ }
+ write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
ret = cpufreq_add_dev_interface(cpu, policy, dev);
if (ret)
goto err_out_unregister;
+ cpufreq_init_policy(policy);
+
kobject_uevent(&policy->kobj, KOBJ_ADD);
module_put(cpufreq_driver->owner);
pr_debug("initialization complete\n");
@@ -1086,8 +1092,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
err_out_unregister:
write_lock_irqsave(&cpufreq_driver_lock, flags);
- for_each_cpu(j, policy->cpus)
+ for_each_cpu(j, policy->cpus) {
per_cpu(cpufreq_cpu_data, j) = NULL;
+ if (j != cpu)
+ per_cpu(cpufreq_policy_cpu, j) = -1;
+ }
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
kobject_put(&policy->kobj);
On Friday, July 12, 2013 03:45:17 AM Srivatsa S. Bhat wrote:
>
> Hi,
Hi,
> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
> some subtle regressions in the cpufreq subsystem during suspend/resume.
> This patchset is aimed at rectifying those problems, by fixing the regression
> as well as achieving the original goal of that commit in a proper way.
>
> Patch 1 reverts the above commit, and is CC'ed to stable.
>
> Patches 2 - 5 reorganize the code and have no functional impact, and can go
> in as general cleanups as well. This reorganization builds a base that the
> rest of the patches will make use of.
>
> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
> across suspend/resume.
>
> All the patches apply on current mainline.
>
>
> Robert, Durgadoss, it would be great if you could try it out and see if it works
> well for your usecase. I tested it locally and cpufreq related files did retain
> their permissions across suspend/resume. Let me know if it works fine in your
> setup too.
>
> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
> whether their systems work fine after:
> a. applying only the first commit (this is what gets backported to stable)
> b. applying all the commits
>
> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
> testing this patchset. Though that patch also touches cpufreq subsystem, it
> doesn't affect this patchset in any way and there is absolutely no dependency
> between the two in terms of code. That fix just makes basic CPU hotplug work
> without locking up on current mainline).
>
> [1]. https://lkml.org/lkml/2013/7/10/611
>
>
> Thank you very much!
Thanks Srivatsa!
I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
hate them. This way we'll have some more testing coverage before they reach
the mainline hopefully.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
-----Original Message-----
From: Srivatsa S. Bhat [mailto:[email protected]]
Sent: Friday, July 12, 2013 00:15
> Robert, Durgadoss, it would be great if you could try it out and
> see if it works well for your usecase. I tested it locally and
> cpufreq related files did retain their permissions across
> suspend/resume. Let me know if it works fine in your setup too.
Sure, I'll check for Monday last delay, if that's not too late for you, as I'm a bit in a hurry right now, but I'll give it a try in the WE I think.
Cheers.
--
Robert
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 07/12/2013 04:03 AM, Rafael J. Wysocki wrote:
> On Friday, July 12, 2013 03:45:17 AM Srivatsa S. Bhat wrote:
>>
>> Hi,
>
> Hi,
>
>> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
>> some subtle regressions in the cpufreq subsystem during suspend/resume.
>> This patchset is aimed at rectifying those problems, by fixing the regression
>> as well as achieving the original goal of that commit in a proper way.
>>
>> Patch 1 reverts the above commit, and is CC'ed to stable.
>>
>> Patches 2 - 5 reorganize the code and have no functional impact, and can go
>> in as general cleanups as well. This reorganization builds a base that the
>> rest of the patches will make use of.
>>
>> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
>> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
>> across suspend/resume.
>>
>> All the patches apply on current mainline.
>>
>>
>> Robert, Durgadoss, it would be great if you could try it out and see if it works
>> well for your usecase. I tested it locally and cpufreq related files did retain
>> their permissions across suspend/resume. Let me know if it works fine in your
>> setup too.
>>
>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>> whether their systems work fine after:
>> a. applying only the first commit (this is what gets backported to stable)
>> b. applying all the commits
>>
>> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
>> testing this patchset. Though that patch also touches cpufreq subsystem, it
>> doesn't affect this patchset in any way and there is absolutely no dependency
>> between the two in terms of code. That fix just makes basic CPU hotplug work
>> without locking up on current mainline).
>>
>> [1]. https://lkml.org/lkml/2013/7/10/611
>>
>>
>> Thank you very much!
>
> Thanks Srivatsa!
>
> I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
> hate them. This way we'll have some more testing coverage before they reach
> the mainline hopefully.
>
Sounds great! Thanks a lot Rafael!
Regards,
Srivatsa S. Bhat
On 12 July 2013 03:45, Srivatsa S. Bhat
<[email protected]> wrote:
> The call to cpufreq_update_policy() is placed in the CPU hotplug callback
> of cpufreq_stats, which has a higher priority than the CPU hotplug callback
> of cpufreq-core. As a result, during CPU_ONLINE/CPU_ONLINE_FROZEN, we end up
> calling cpufreq_update_policy() *before* calling cpufreq_add_dev() !
> And for uninitialized CPUs, it just returns silently, not doing anything.
Hmm..
> To add to it, cpufreq_stats is not even the right place to call
> cpufreq_update_policy() to begin with. The cpufreq core ought to handle
> this in its own callback, from an elegance/relevance perspective.
>
> So move the invocation of cpufreq_update_policy() to cpufreq_cpu_callback,
> and place it *after* cpufreq_add_dev().
>
> Signed-off-by: Srivatsa S. Bhat <[email protected]>
> ---
>
> drivers/cpufreq/cpufreq.c | 1 +
> drivers/cpufreq/cpufreq_stats.c | 6 ------
> 2 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index ccc6eab..f8c3100 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1943,6 +1943,7 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
> case CPU_ONLINE:
> case CPU_ONLINE_FROZEN:
> cpufreq_add_dev(dev, NULL);
> + cpufreq_update_policy(cpu);
Do we need to call this for every hotplug of cpu? I am not
talking about suspend/resume here.
On 12 July 2013 03:46, Srivatsa S. Bhat
<[email protected]> wrote:
> Separate out the allocation of the cpufreq policy structure (along with
> its error handling) to a helper function. This makes the code easier to
> read and also helps with some upcoming code reorganization.
>
> Signed-off-by: Srivatsa S. Bhat <[email protected]>
> ---
>
> drivers/cpufreq/cpufreq.c | 50 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index f8c3100..ca14dc2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -943,6 +943,37 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
> }
> #endif
>
> +static struct cpufreq_policy *cpufreq_policy_alloc(void)
> +{
> + struct cpufreq_policy *policy;
> +
> + policy = kzalloc(sizeof(struct cpufreq_policy), GFP_KERNEL);
sizeof(*policy) ??
Acked-by: Viresh Kumar <[email protected]>
On 12 July 2013 03:46, Srivatsa S. Bhat
<[email protected]> wrote:
> cpufreq_add_dev_interface() includes the work of exposing the interface
> to the device, as well as a lot of unrelated stuff. Move the latter to
> cpufreq_add_dev(), where it is more appropriate.
>
> Signed-off-by: Srivatsa S. Bhat <[email protected]>
> ---
>
> drivers/cpufreq/cpufreq.c | 43 ++++++++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
On 12 July 2013 03:45, Srivatsa S. Bhat
<[email protected]> wrote:
> commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) has
> unfortunately caused several things in the cpufreq subsystem to break subtly
> after a suspend/resume cycle.
>
> The intention of that patch was to retain the file permissions of the
> cpufreq related sysfs files across suspend/resume. To achieve that, the commit
> completely removed the calls to cpufreq_add_dev() and __cpufreq_remove_dev()
> during suspend/resume transitions. But the problem is that those functions
> do 2 kinds of things:
> 1. Low-level initialization/tear-down that are critical to the correct
> functioning of cpufreq-core.
> 2. Kobject and sysfs related initialization/teardown.
>
> Ideally we should have reorganized the code to cleanly separate these two
> responsibilities, and skipped only the sysfs related parts during
> suspend/resume. Since we skipped the entire callbacks instead (which also
> included some CPU and cpufreq-specific critical components), cpufreq
> subsystem started behaving erratically after suspend/resume.
>
> So revert the commit to fix the regression. We'll revisit and address the
> original goal of that commit separately, since it involves quite a bit of
> careful code reorganization and appears to be non-trivial.
>
> (While reverting the commit, note that another commit f51e1eb "cpufreq:
> Fix cpufreq regression after suspend/resume" already reverted part of the
> original set of changes. So revert only the remaining ones).
>
> Cc: [email protected]
> Signed-off-by: Srivatsa S. Bhat <[email protected]>
> ---
>
> drivers/cpufreq/cpufreq.c | 4 +++-
> drivers/cpufreq/cpufreq_stats.c | 6 ++----
> 2 files changed, 5 insertions(+), 5 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
On 12 July 2013 03:46, Srivatsa S. Bhat
<[email protected]> wrote:
> During cpu offline, when the policy->cpu is going down, some other CPU
> present in the policy->cpus mask is nominated as the new policy->cpu.
> Extract this functionality from __cpufreq_remove_dev() and implement
> it in a helper function. This helps in upcoming code reorganization.
>
> Signed-off-by: Srivatsa S. Bhat <[email protected]>
> ---
>
> drivers/cpufreq/cpufreq.c | 62 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 38 insertions(+), 24 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
On 12 July 2013 03:46, Srivatsa S. Bhat
<[email protected]> wrote:
> During suspend/resume we would like to do a light-weight init/teardown of
> CPUs in the cpufreq subsystem and preserve certain things such as sysfs files
> etc across suspend/resume transitions. Add a flag called 'frozen' to help
> distinguish the full init/teardown sequence from the light-weight one.
>
> Signed-off-by: Srivatsa S. Bhat <[email protected]>
> ---
>
> drivers/cpufreq/cpufreq.c | 66 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 41 insertions(+), 25 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
On 07/12/2013 12:15 AM, Srivatsa S. Bhat wrote:
> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
> whether their systems work fine after:
> a. applying only the first commit (this is what gets backported to stable)
applied on top of straight 3.10 .0 : Breaks my system completely -
trying s2ram just blanks the console, lets the power led blinking,
neither sys-rq nor anything else worked now, no output to console nor to
syslog
> b. applying all the commits
patch 2#8 doesn't apply at 3.10.0 (neither after patch 1#8 nor directly)
FWIW /me double checked all those test results
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
On Fri, 2013-07-12 at 12:48 +0530, Viresh Kumar wrote:
> On 12 July 2013 03:45, Srivatsa S. Bhat
> <[email protected]> wrote:
> > commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) has
> > unfortunately caused several things in the cpufreq subsystem to break subtly
> > after a suspend/resume cycle.
> >
> > The intention of that patch was to retain the file permissions of the
> > cpufreq related sysfs files across suspend/resume. To achieve that, the commit
> > completely removed the calls to cpufreq_add_dev() and __cpufreq_remove_dev()
> > during suspend/resume transitions. But the problem is that those functions
> > do 2 kinds of things:
> > 1. Low-level initialization/tear-down that are critical to the correct
> > functioning of cpufreq-core.
> > 2. Kobject and sysfs related initialization/teardown.
> >
> > Ideally we should have reorganized the code to cleanly separate these two
> > responsibilities, and skipped only the sysfs related parts during
> > suspend/resume. Since we skipped the entire callbacks instead (which also
> > included some CPU and cpufreq-specific critical components), cpufreq
> > subsystem started behaving erratically after suspend/resume.
> >
> > So revert the commit to fix the regression. We'll revisit and address the
> > original goal of that commit separately, since it involves quite a bit of
> > careful code reorganization and appears to be non-trivial.
> >
> > (While reverting the commit, note that another commit f51e1eb "cpufreq:
> > Fix cpufreq regression after suspend/resume" already reverted part of the
> > original set of changes. So revert only the remaining ones).
> >
> > Cc: [email protected]
> > Signed-off-by: Srivatsa S. Bhat <[email protected]>
> > ---
> >
> > drivers/cpufreq/cpufreq.c | 4 +++-
> > drivers/cpufreq/cpufreq_stats.c | 6 ++----
> > 2 files changed, 5 insertions(+), 5 deletions(-)
>
> Acked-by: Viresh Kumar <[email protected]>
This seems to fix the "core stuck at some frequency after resume" issue
I ran into since v3.10. So:
Tested-by: Paul Bolle <[email protected]>
Paul Bolle
On 07/13/2013 11:23 AM, Toralf Förster wrote:
> On 07/12/2013 12:15 AM, Srivatsa S. Bhat wrote:
>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>> whether their systems work fine after:
>> a. applying only the first commit (this is what gets backported to stable)
>
> applied on top of straight 3.10 .0 : Breaks my system completely -
overlooked, that the 8 patches are 3.11/3.12 material - but nevertheless :
Applied 1/8 on top of v3.10-9289-g9903883 brought same bad picture as
described for 3.10.0
And applying patches 1-8 on top of that commit id just gives the same
pciture - systems hangs during s2ram completly
> trying s2ram just blanks the console, lets the power led blinking,
> neither sys-rq nor anything else worked now, no output to console nor to
> syslog
>
>> b. applying all the commits
> patch 2#8 doesn't apply at 3.10.0 (neither after patch 1#8 nor directly)
I attached the .config I'm using for my tests
(/me wonders if it is worth to notice, that it is a 32bit system booted
from an external USB drive ?)
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
On 07/13/2013 06:16 PM, Paul Bolle wrote:
> On Fri, 2013-07-12 at 12:48 +0530, Viresh Kumar wrote:
>> On 12 July 2013 03:45, Srivatsa S. Bhat
>> <[email protected]> wrote:
>>> commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) has
>>> unfortunately caused several things in the cpufreq subsystem to break subtly
>>> after a suspend/resume cycle.
>>>
>>> The intention of that patch was to retain the file permissions of the
>>> cpufreq related sysfs files across suspend/resume. To achieve that, the commit
>>> completely removed the calls to cpufreq_add_dev() and __cpufreq_remove_dev()
>>> during suspend/resume transitions. But the problem is that those functions
>>> do 2 kinds of things:
>>> 1. Low-level initialization/tear-down that are critical to the correct
>>> functioning of cpufreq-core.
>>> 2. Kobject and sysfs related initialization/teardown.
>>>
>>> Ideally we should have reorganized the code to cleanly separate these two
>>> responsibilities, and skipped only the sysfs related parts during
>>> suspend/resume. Since we skipped the entire callbacks instead (which also
>>> included some CPU and cpufreq-specific critical components), cpufreq
>>> subsystem started behaving erratically after suspend/resume.
>>>
>>> So revert the commit to fix the regression. We'll revisit and address the
>>> original goal of that commit separately, since it involves quite a bit of
>>> careful code reorganization and appears to be non-trivial.
>>>
>>> (While reverting the commit, note that another commit f51e1eb "cpufreq:
>>> Fix cpufreq regression after suspend/resume" already reverted part of the
>>> original set of changes. So revert only the remaining ones).
>>>
>>> Cc: [email protected]
>>> Signed-off-by: Srivatsa S. Bhat <[email protected]>
>>> ---
>>>
>>> drivers/cpufreq/cpufreq.c | 4 +++-
>>> drivers/cpufreq/cpufreq_stats.c | 6 ++----
>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> Acked-by: Viresh Kumar <[email protected]>
>
> This seems to fix the "core stuck at some frequency after resume" issue
> I ran into since v3.10. So:
>
> Tested-by: Paul Bolle <[email protected]>
>
Thanks Paul!
Regards,
Srivatsa S. Bhat
On 07/12/2013 12:36 PM, Viresh Kumar wrote:
> On 12 July 2013 03:45, Srivatsa S. Bhat
> <[email protected]> wrote:
>> The call to cpufreq_update_policy() is placed in the CPU hotplug callback
>> of cpufreq_stats, which has a higher priority than the CPU hotplug callback
>> of cpufreq-core. As a result, during CPU_ONLINE/CPU_ONLINE_FROZEN, we end up
>> calling cpufreq_update_policy() *before* calling cpufreq_add_dev() !
>> And for uninitialized CPUs, it just returns silently, not doing anything.
>
> Hmm..
>
>> To add to it, cpufreq_stats is not even the right place to call
>> cpufreq_update_policy() to begin with. The cpufreq core ought to handle
>> this in its own callback, from an elegance/relevance perspective.
>>
>> So move the invocation of cpufreq_update_policy() to cpufreq_cpu_callback,
>> and place it *after* cpufreq_add_dev().
>>
>> Signed-off-by: Srivatsa S. Bhat <[email protected]>
>> ---
>>
>> drivers/cpufreq/cpufreq.c | 1 +
>> drivers/cpufreq/cpufreq_stats.c | 6 ------
>> 2 files changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index ccc6eab..f8c3100 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1943,6 +1943,7 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>> case CPU_ONLINE:
>> case CPU_ONLINE_FROZEN:
>> cpufreq_add_dev(dev, NULL);
>> + cpufreq_update_policy(cpu);
>
> Do we need to call this for every hotplug of cpu? I am not
> talking about suspend/resume here.
>
I don't think we need to, but I think it would be better to postpone
optimizations until all the cpufreq regressions get fixed. Later perhaps
we could revisit these minor optimizations if desired.
Regards,
Srivatsa S. Bhat
On 07/12/2013 12:39 PM, Viresh Kumar wrote:
> On 12 July 2013 03:46, Srivatsa S. Bhat
> <[email protected]> wrote:
>> Separate out the allocation of the cpufreq policy structure (along with
>> its error handling) to a helper function. This makes the code easier to
>> read and also helps with some upcoming code reorganization.
>>
>> Signed-off-by: Srivatsa S. Bhat <[email protected]>
>> ---
>>
>> drivers/cpufreq/cpufreq.c | 50 ++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index f8c3100..ca14dc2 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -943,6 +943,37 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
>> }
>> #endif
>>
>> +static struct cpufreq_policy *cpufreq_policy_alloc(void)
>> +{
>> + struct cpufreq_policy *policy;
>> +
>> + policy = kzalloc(sizeof(struct cpufreq_policy), GFP_KERNEL);
>
> sizeof(*policy) ??
Ah, thanks for pointing that. That must be a remnant from the old code.
But, to make it easier for people testing this patchset to fix their
cpufreq regressions, I'll hold off on posting newer versions of this patchset,
to avoid confusion. We can revisit this at a later point in time, IMHO.
Regards,
Srivatsa S. Bhat
On 07/13/2013 07:20 PM, Toralf Förster wrote:
> On 07/13/2013 11:23 AM, Toralf Förster wrote:
>> On 07/12/2013 12:15 AM, Srivatsa S. Bhat wrote:
>>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>>> whether their systems work fine after:
>>> a. applying only the first commit (this is what gets backported to stable)
>>
>> applied on top of straight 3.10 .0 : Breaks my system completely -
>
> overlooked, that the 8 patches are 3.11/3.12 material - but nevertheless :
>
Let me clarify where to apply these patches:
Assuming that you are using mainline (not -stable) for your testing, this is how
it goes:
For mainline v3.10 (final release, commit 8bb495e3f):
You need to apply 2 patches, in the order mentioned below:
1. Commit f51e1eb63d (cpufreq: Fix cpufreq regression after suspend/resume)
2. Patch 1/8 in this patchset. https://lkml.org/lkml/2013/7/11/661
Those 2 together, should be able to fix all the cpufreq regression you
originally saw with commit a66b2e (cpufreq: Preserve sysfs files across
suspend/resume).
------
Now, coming to current mainline, ie., 3.10+ (after 3.10, in-between the merge
window), you need to test two different things:
Scenario 1:
Apply only patch 1/8 in this patchset. https://lkml.org/lkml/2013/7/11/661
Check if cpufreq behaves fine after suspend/resume.
Scenario 2:
Apply all the 8 patches in this patchset, and check if cpufreq still
works fine after suspend/resume.
Important note:
--------------
This patchset and any of the patches/commits I mentioned above *do* *not* try
to fix any core suspend/resume regression. They only try to fix the *cpufreq*
regression related to suspend/resume, which commit a66b2e (cpufreq: Preserve
sysfs files across suspend/resume) had caused.
In other words, if basic suspend/resume itself is not working even before you
apply any of the patches mentioned above, then something *else* is totally
broken, and we need to address that separately.
> Applied 1/8 on top of v3.10-9289-g9903883 brought same bad picture as
> described for 3.10.0
>
> And applying patches 1-8 on top of that commit id just gives the same
> pciture - systems hangs during s2ram completly
>
Please verify whether suspend/resume works fine before applying any of the
patches. That's an important baseline. This patchset tries to fix only the
cpufreq regression, and not all the suspend/resume related problems (which
might have creeped in during the merge window).
>
>> trying s2ram just blanks the console, lets the power led blinking,
>> neither sys-rq nor anything else worked now, no output to console nor to
>> syslog
>>
>>> b. applying all the commits
>> patch 2#8 doesn't apply at 3.10.0 (neither after patch 1#8 nor directly)
>
> I attached the .config I'm using for my tests
> (/me wonders if it is worth to notice, that it is a 32bit system booted
> from an external USB drive ?)
>
Regards,
Srivatsa S. Bhat
On 2013年07月12日 06:15, Srivatsa S. Bhat wrote:
>
> Hi,
>
> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
> some subtle regressions in the cpufreq subsystem during suspend/resume.
> This patchset is aimed at rectifying those problems, by fixing the regression
> as well as achieving the original goal of that commit in a proper way.
>
> Patch 1 reverts the above commit, and is CC'ed to stable.
>
> Patches 2 - 5 reorganize the code and have no functional impact, and can go
> in as general cleanups as well. This reorganization builds a base that the
> rest of the patches will make use of.
>
> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
> across suspend/resume.
>
> All the patches apply on current mainline.
>
>
> Robert, Durgadoss, it would be great if you could try it out and see if it works
> well for your usecase. I tested it locally and cpufreq related files did retain
> their permissions across suspend/resume. Let me know if it works fine in your
> setup too.
>
> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
> whether their systems work fine after:
> a. applying only the first commit (this is what gets backported to stable)
> b. applying all the commits
Hi, I tested this patchset on my machine and the issue in bug 59781 has
been resolved.
Tested-by: Tianyu Lan <[email protected]>
>
> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
> testing this patchset. Though that patch also touches cpufreq subsystem, it
> doesn't affect this patchset in any way and there is absolutely no dependency
> between the two in terms of code. That fix just makes basic CPU hotplug work
> without locking up on current mainline).
>
> [1]. https://lkml.org/lkml/2013/7/10/611
>
>
> Thank you very much!
>
>
> Srivatsa S. Bhat (8):
> cpufreq: Revert commit a66b2e to fix cpufreq regression during suspend/resume
> cpufreq: Fix misplaced call to cpufreq_update_policy()
> cpufreq: Add helper to perform alloc/free of policy structure
> cpufreq: Extract non-interface related stuff from cpufreq_add_dev_interface
> cpufreq: Extract the handover of policy cpu to a helper function
> cpufreq: Introduce a flag ('frozen') to separate full vs temporary init/teardown
> cpufreq: Preserve policy structure across suspend/resume
> cpufreq: Perform light-weight init/teardown during suspend/resume
>
> drivers/cpufreq/cpufreq.c | 297 ++++++++++++++++++++++++++-------------
> drivers/cpufreq/cpufreq_stats.c | 10 -
> 2 files changed, 199 insertions(+), 108 deletions(-)
>
>
> Thanks,
> Srivatsa S. Bhat
> IBM Linux Technology Center
>
--
Best regards
Tianyu Lan
On 07/15/2013 01:57 PM, Lan Tianyu wrote:
> On 2013年07月12日 06:15, Srivatsa S. Bhat wrote:
>>
>> Hi,
>>
>> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
>> some subtle regressions in the cpufreq subsystem during suspend/resume.
>> This patchset is aimed at rectifying those problems, by fixing the regression
>> as well as achieving the original goal of that commit in a proper way.
>>
>> Patch 1 reverts the above commit, and is CC'ed to stable.
>>
>> Patches 2 - 5 reorganize the code and have no functional impact, and can go
>> in as general cleanups as well. This reorganization builds a base that the
>> rest of the patches will make use of.
>>
>> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
>> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
>> across suspend/resume.
>>
>> All the patches apply on current mainline.
>>
>>
>> Robert, Durgadoss, it would be great if you could try it out and see if it works
>> well for your usecase. I tested it locally and cpufreq related files did retain
>> their permissions across suspend/resume. Let me know if it works fine in your
>> setup too.
>>
>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>> whether their systems work fine after:
>> a. applying only the first commit (this is what gets backported to stable)
>> b. applying all the commits
>
> Hi, I tested this patchset on my machine and the issue in bug 59781 has
> been resolved.
>
> Tested-by: Tianyu Lan <[email protected]>
>
Awesome! Thanks a lot!
Regards,
Srivatsa S. Bhat
Hi Srivatsa,
I may be wrong but it looks something is wrong in this patch.
On 12 July 2013 03:47, Srivatsa S. Bhat
<[email protected]> wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> @@ -1239,29 +1263,40 @@ static int __cpufreq_remove_dev(struct device *dev,
> if ((cpus == 1) && (cpufreq_driver->target))
> __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
>
> - pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
> - cpufreq_cpu_put(data);
> + if (!frozen) {
> + pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
> + cpufreq_cpu_put(data);
So, we don't decrement usage count here. But we are still increasing
counts on cpufreq_add_dev after resume, isn't it?
So, we wouldn't be able to free policy struct once all the cpus of a
policy are removed after suspend/resume has happened once.
On 07/15/2013 03:25 PM, Viresh Kumar wrote:
> Hi Srivatsa,
>
> I may be wrong but it looks something is wrong in this patch.
>
> On 12 July 2013 03:47, Srivatsa S. Bhat
> <[email protected]> wrote:
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>
>> @@ -1239,29 +1263,40 @@ static int __cpufreq_remove_dev(struct device *dev,
>> if ((cpus == 1) && (cpufreq_driver->target))
>> __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
>>
>> - pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
>> - cpufreq_cpu_put(data);
>> + if (!frozen) {
>> + pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
>> + cpufreq_cpu_put(data);
>
> So, we don't decrement usage count here. But we are still increasing
> counts on cpufreq_add_dev after resume, isn't it?
>
> So, we wouldn't be able to free policy struct once all the cpus of a
> policy are removed after suspend/resume has happened once.
>
Actually even I was wondering about this while writing the patch and
I even tested shutdown after multiple suspend/resume cycles, to verify that
the refcount is messed up. But surprisingly, things worked just fine.
Logically there should've been a refcount mismatch and things should have
failed, but everything worked fine during my tests. Apart from suspend/resume
and shutdown tests, I even tried mixing a few regular CPU hotplug operations
(echo 0/1 to sysfs online files), but nothing stood out.
Sorry, I forgot to document this in the patch. Either the patch is wrong
or something else is silently fixing this up. Not sure what is the exact
situation.
Regards,
Srivatsa S. Bhat
On 15 July 2013 15:35, Srivatsa S. Bhat
<[email protected]> wrote:
> Actually even I was wondering about this while writing the patch and
> I even tested shutdown after multiple suspend/resume cycles, to verify that
> the refcount is messed up. But surprisingly, things worked just fine.
What kind of system have you tested it on?
On Monday, July 15, 2013 03:35:04 PM Srivatsa S. Bhat wrote:
> On 07/15/2013 03:25 PM, Viresh Kumar wrote:
> > Hi Srivatsa,
> >
> > I may be wrong but it looks something is wrong in this patch.
> >
> > On 12 July 2013 03:47, Srivatsa S. Bhat
> > <[email protected]> wrote:
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >
> >> @@ -1239,29 +1263,40 @@ static int __cpufreq_remove_dev(struct device *dev,
> >> if ((cpus == 1) && (cpufreq_driver->target))
> >> __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
> >>
> >> - pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
> >> - cpufreq_cpu_put(data);
> >> + if (!frozen) {
> >> + pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
> >> + cpufreq_cpu_put(data);
> >
> > So, we don't decrement usage count here. But we are still increasing
> > counts on cpufreq_add_dev after resume, isn't it?
> >
> > So, we wouldn't be able to free policy struct once all the cpus of a
> > policy are removed after suspend/resume has happened once.
> >
>
> Actually even I was wondering about this while writing the patch and
> I even tested shutdown after multiple suspend/resume cycles, to verify that
> the refcount is messed up. But surprisingly, things worked just fine.
>
> Logically there should've been a refcount mismatch and things should have
> failed, but everything worked fine during my tests. Apart from suspend/resume
> and shutdown tests, I even tried mixing a few regular CPU hotplug operations
> (echo 0/1 to sysfs online files), but nothing stood out.
>
> Sorry, I forgot to document this in the patch. Either the patch is wrong
> or something else is silently fixing this up. Not sure what is the exact
> situation.
OK, so I'm not going to queue [2-8/8] up until we find out what's going on
here (and until Toralf tells me that it doesn't break his system any more).
I've queued up [1/8] for 3.11 already.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Monday, July 15, 2013 11:50:24 AM Srivatsa S. Bhat wrote:
> On 07/12/2013 12:36 PM, Viresh Kumar wrote:
> > On 12 July 2013 03:45, Srivatsa S. Bhat
> > <[email protected]> wrote:
> >> The call to cpufreq_update_policy() is placed in the CPU hotplug callback
> >> of cpufreq_stats, which has a higher priority than the CPU hotplug callback
> >> of cpufreq-core. As a result, during CPU_ONLINE/CPU_ONLINE_FROZEN, we end up
> >> calling cpufreq_update_policy() *before* calling cpufreq_add_dev() !
> >> And for uninitialized CPUs, it just returns silently, not doing anything.
> >
> > Hmm..
> >
> >> To add to it, cpufreq_stats is not even the right place to call
> >> cpufreq_update_policy() to begin with. The cpufreq core ought to handle
> >> this in its own callback, from an elegance/relevance perspective.
> >>
> >> So move the invocation of cpufreq_update_policy() to cpufreq_cpu_callback,
> >> and place it *after* cpufreq_add_dev().
> >>
> >> Signed-off-by: Srivatsa S. Bhat <[email protected]>
> >> ---
> >>
> >> drivers/cpufreq/cpufreq.c | 1 +
> >> drivers/cpufreq/cpufreq_stats.c | 6 ------
> >> 2 files changed, 1 insertion(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index ccc6eab..f8c3100 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -1943,6 +1943,7 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
> >> case CPU_ONLINE:
> >> case CPU_ONLINE_FROZEN:
> >> cpufreq_add_dev(dev, NULL);
> >> + cpufreq_update_policy(cpu);
> >
> > Do we need to call this for every hotplug of cpu? I am not
> > talking about suspend/resume here.
> >
>
> I don't think we need to, but I think it would be better to postpone
> optimizations until all the cpufreq regressions get fixed. Later perhaps
> we could revisit these minor optimizations if desired.
Agreed.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 07/15/2013 03:51 PM, Viresh Kumar wrote:
> On 15 July 2013 15:35, Srivatsa S. Bhat
> <[email protected]> wrote:
>> Actually even I was wondering about this while writing the patch and
>> I even tested shutdown after multiple suspend/resume cycles, to verify that
>> the refcount is messed up. But surprisingly, things worked just fine.
>
> What kind of system have you tested it on?
>
The system has 2 sockets with 8 cores each, and has Intel Sandybridge
CPUs. I had used a local patch to simulate CPU hotplug in the suspend-to-ram
path using the freeze state of pm_test (because I had other problems in
using the 'processors' state of pm_test). The patch is shown below:
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index ece0422..fe07b77 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -342,8 +342,13 @@ static int enter_state(suspend_state_t state)
if (error)
goto Unlock;
- if (suspend_test(TEST_FREEZER))
+ if (suspend_test(TEST_FREEZER)) {
+ pr_debug("Disabling nonboot CPUs\n");
+ disable_nonboot_cpus();
+ pr_debug("Enabling nonboot CPUs\n");
+ enable_nonboot_cpus();
goto Finish;
+ }
pr_debug("PM: Entering %s sleep\n", pm_states[state]);
pm_restrict_gfp_mask();
Regards,
Srivatsa S. Bhat
On 07/15/2013 05:05 PM, Rafael J. Wysocki wrote:
> On Monday, July 15, 2013 03:35:04 PM Srivatsa S. Bhat wrote:
>> On 07/15/2013 03:25 PM, Viresh Kumar wrote:
>>> Hi Srivatsa,
>>>
>>> I may be wrong but it looks something is wrong in this patch.
>>>
>>> On 12 July 2013 03:47, Srivatsa S. Bhat
>>> <[email protected]> wrote:
>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>>
>>>> @@ -1239,29 +1263,40 @@ static int __cpufreq_remove_dev(struct device *dev,
>>>> if ((cpus == 1) && (cpufreq_driver->target))
>>>> __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
>>>>
>>>> - pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
>>>> - cpufreq_cpu_put(data);
>>>> + if (!frozen) {
>>>> + pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
>>>> + cpufreq_cpu_put(data);
>>>
>>> So, we don't decrement usage count here. But we are still increasing
>>> counts on cpufreq_add_dev after resume, isn't it?
>>>
>>> So, we wouldn't be able to free policy struct once all the cpus of a
>>> policy are removed after suspend/resume has happened once.
>>>
>>
>> Actually even I was wondering about this while writing the patch and
>> I even tested shutdown after multiple suspend/resume cycles, to verify that
>> the refcount is messed up. But surprisingly, things worked just fine.
>>
>> Logically there should've been a refcount mismatch and things should have
>> failed, but everything worked fine during my tests. Apart from suspend/resume
>> and shutdown tests, I even tried mixing a few regular CPU hotplug operations
>> (echo 0/1 to sysfs online files), but nothing stood out.
>>
>> Sorry, I forgot to document this in the patch. Either the patch is wrong
>> or something else is silently fixing this up. Not sure what is the exact
>> situation.
>
> OK, so I'm not going to queue [2-8/8] up until we find out what's going on
> here (and until Toralf tells me that it doesn't break his system any more).
>
Ok, that sounds good.
> I've queued up [1/8] for 3.11 already.
>
Thank you!
Regards,
Srivatsa S. Bhat
On 07/12/2013 12:33 AM, Rafael J. Wysocki wrote:
> I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
> hate them. This way we'll have some more testing coverage before they reach
> the mainline hopefully.
Applied patch 1#8 on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
cycles fine and crashed the system at the 3rd attempt / one times just
at the 4th (blinking power led, no sysrq, ...).
Applying patch 1-8 on top of that tree differs in that way that it
crashes now the system even at the 1st attempt or at least at the 2nd
My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
Gentoo Linux - FWIW .config attached.
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
On Monday, July 15, 2013 07:38:02 PM Toralf Förster wrote:
> On 07/12/2013 12:33 AM, Rafael J. Wysocki wrote:
> > I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
> > hate them. This way we'll have some more testing coverage before they reach
> > the mainline hopefully.
>
> Applied patch 1#8 on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
Sorry, I have no idea what 1#8 means.
> cycles fine and crashed the system at the 3rd attempt / one times just
> at the 4th (blinking power led, no sysrq, ...).
>
> Applying patch 1-8 on top of that tree differs in that way that it
> crashes now the system even at the 1st attempt or at least at the 2nd
What are you talking about?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 15 July 2013 15:35, Srivatsa S. Bhat
<[email protected]> wrote:
> Actually even I was wondering about this while writing the patch and
> I even tested shutdown after multiple suspend/resume cycles, to verify that
> the refcount is messed up. But surprisingly, things worked just fine.
>
> Logically there should've been a refcount mismatch and things should have
> failed, but everything worked fine during my tests. Apart from suspend/resume
> and shutdown tests, I even tried mixing a few regular CPU hotplug operations
> (echo 0/1 to sysfs online files), but nothing stood out.
>
> Sorry, I forgot to document this in the patch. Either the patch is wrong
> or something else is silently fixing this up. Not sure what is the exact
> situation.
To understand it I actually applied your patches to get better view of the code.
(Haven't tested it though).. And found that your code is doing the right thing
and we shouldn't get a mismatch.. This is the sequence of events I can draw:
- __cpu_add_dev() for first cpu. sets the refcount to 'x', where x are
the no. of
cpus in its clock domain.
- _cpu_add_dev() for other cpus: doesn't change anything in refcount
- Suspend:
- cpu_remove_dev() for all cpus, due to frozen flag we don't touch the value
of count
- Resume:
- cpu_add_dev() for all cpus, due to frozen flag we don't touch the
value of count.
And so things work as expected. That's why your code isn't breaking anything I
believe.
But can no. of cpus change inbetween suspend/resume? Then count would be
tricky as we are using the same policy structure.
On 07/16/2013 11:45 AM, Viresh Kumar wrote:
> On 15 July 2013 15:35, Srivatsa S. Bhat
> <[email protected]> wrote:
>> Actually even I was wondering about this while writing the patch and
>> I even tested shutdown after multiple suspend/resume cycles, to verify that
>> the refcount is messed up. But surprisingly, things worked just fine.
>>
>> Logically there should've been a refcount mismatch and things should have
>> failed, but everything worked fine during my tests. Apart from suspend/resume
>> and shutdown tests, I even tried mixing a few regular CPU hotplug operations
>> (echo 0/1 to sysfs online files), but nothing stood out.
>>
>> Sorry, I forgot to document this in the patch. Either the patch is wrong
>> or something else is silently fixing this up. Not sure what is the exact
>> situation.
>
> To understand it I actually applied your patches to get better view of the code.
> (Haven't tested it though).. And found that your code is doing the right thing
> and we shouldn't get a mismatch.. This is the sequence of events I can draw:
>
> - __cpu_add_dev() for first cpu. sets the refcount to 'x', where x are
> the no. of
> cpus in its clock domain.
> - _cpu_add_dev() for other cpus: doesn't change anything in refcount
>
> - Suspend:
> - cpu_remove_dev() for all cpus, due to frozen flag we don't touch the value
> of count
> - Resume:
> - cpu_add_dev() for all cpus, due to frozen flag we don't touch the
> value of count.
>
Actually this one is tricky (I took a look again). So we have this code in the
beginning of _cpufreq_add_dev():
1008 #ifdef CONFIG_SMP
1009 /* check whether a different CPU already registered this
1010 * CPU because it is in the same boat. */
1011 policy = cpufreq_cpu_get(cpu);
1012 if (unlikely(policy)) {
1013 cpufreq_cpu_put(policy);
1014 return 0;
1015 }
The _get() is not controlled by the frozen flag, but it still doesn't take a
refcount because of a subtle reason: per_cpu(cpufreq_cpu_data, cpu) was set to
NULL in __cpufreq_remove_dev() and the memory was saved away in fallback storage.
So, when __cpufreq_cpu_get() executes, it sees:
204 /* get the CPU */
205 data = per_cpu(cpufreq_cpu_data, cpu);
206
207 if (!data)
208 goto err_out_put_module;
Thus, since data is NULL, cpufreq_cpu_get() won't take a refcount and will return
silently.
Further down in __cpufreq_add_dev(), we restore the original memory, using
the frozen flag:
1037 if (frozen)
1038 /* Restore the saved policy when doing light-weight init */
1039 policy = cpufreq_policy_restore(cpu);
1040 else
1041 policy = cpufreq_policy_alloc();
So that is how we manage to fool cpufreq_cpu_get() into not taking a fresh
refcount while resuming :)
> And so things work as expected. That's why your code isn't breaking anything I
> believe.
>
Thanks a lot for the code inspection and your detailed analysis!
> But can no. of cpus change inbetween suspend/resume? Then count would be
> tricky as we are using the same policy structure.
>
No, number of CPUs won't change in between suspend/resume. Even if somebody
tried that, that would be an eccentric case and we won't handle that.
Besides, *many more* things will break than just cpufreq, if somebody actually
tries that out!
Regards,
Srivatsa S. Bhat
On 16 July 2013 14:26, Srivatsa S. Bhat
<[email protected]> wrote:
> On 07/16/2013 11:45 AM, Viresh Kumar wrote:
>> To understand it I actually applied your patches to get better view of the code.
>> (Haven't tested it though).. And found that your code is doing the right thing
>> and we shouldn't get a mismatch.. This is the sequence of events I can draw:
>>
>> - __cpu_add_dev() for first cpu. sets the refcount to 'x', where x are
>> the no. of
>> cpus in its clock domain.
>> - _cpu_add_dev() for other cpus: doesn't change anything in refcount
>>
>> - Suspend:
>> - cpu_remove_dev() for all cpus, due to frozen flag we don't touch the value
>> of count
>> - Resume:
>> - cpu_add_dev() for all cpus, due to frozen flag we don't touch the
>> value of count.
>>
>
> Actually this one is tricky (I took a look again). So we have this code in the
> beginning of _cpufreq_add_dev():
>
>
> 1008 #ifdef CONFIG_SMP
> 1009 /* check whether a different CPU already registered this
> 1010 * CPU because it is in the same boat. */
> 1011 policy = cpufreq_cpu_get(cpu);
> 1012 if (unlikely(policy)) {
> 1013 cpufreq_cpu_put(policy);
> 1014 return 0;
> 1015 }
>
> The _get() is not controlled by the frozen flag, but it still doesn't take a
> refcount because of a subtle reason: per_cpu(cpufreq_cpu_data, cpu) was set to
> NULL in __cpufreq_remove_dev() and the memory was saved away in fallback storage.
> So, when __cpufreq_cpu_get() executes, it sees:
>
> 204 /* get the CPU */
> 205 data = per_cpu(cpufreq_cpu_data, cpu);
> 206
> 207 if (!data)
> 208 goto err_out_put_module;
>
> Thus, since data is NULL, cpufreq_cpu_get() won't take a refcount and will return
> silently.
Even if this wouldn't have happened, refcount wouldn't have been
touched due to this code:
> 1012 if (unlikely(policy)) {
> 1013 cpufreq_cpu_put(policy);
> 1014 return 0;
> 1015 }
i.e. If we get a valid policy structure, we siimply put the policy again
and so decrement the incremented refcount.
So, even if you don't keep the fallback storage, things should work
without any issue (probably worth trying as this will get rid of a per
cpu variable :))
> Further down in __cpufreq_add_dev(), we restore the original memory, using
> the frozen flag:
>
> 1037 if (frozen)
> 1038 /* Restore the saved policy when doing light-weight init */
> 1039 policy = cpufreq_policy_restore(cpu);
> 1040 else
> 1041 policy = cpufreq_policy_alloc();
>
>
> So that is how we manage to fool cpufreq_cpu_get() into not taking a fresh
> refcount while resuming :)
On 07/16/2013 02:40 PM, Viresh Kumar wrote:
> On 16 July 2013 14:26, Srivatsa S. Bhat
> <[email protected]> wrote:
>> On 07/16/2013 11:45 AM, Viresh Kumar wrote:
>
>>> To understand it I actually applied your patches to get better view of the code.
>>> (Haven't tested it though).. And found that your code is doing the right thing
>>> and we shouldn't get a mismatch.. This is the sequence of events I can draw:
>>>
>>> - __cpu_add_dev() for first cpu. sets the refcount to 'x', where x are
>>> the no. of
>>> cpus in its clock domain.
>>> - _cpu_add_dev() for other cpus: doesn't change anything in refcount
>>>
>>> - Suspend:
>>> - cpu_remove_dev() for all cpus, due to frozen flag we don't touch the value
>>> of count
>>> - Resume:
>>> - cpu_add_dev() for all cpus, due to frozen flag we don't touch the
>>> value of count.
>>>
>>
>> Actually this one is tricky (I took a look again). So we have this code in the
>> beginning of _cpufreq_add_dev():
>>
>>
>> 1008 #ifdef CONFIG_SMP
>> 1009 /* check whether a different CPU already registered this
>> 1010 * CPU because it is in the same boat. */
>> 1011 policy = cpufreq_cpu_get(cpu);
>> 1012 if (unlikely(policy)) {
>> 1013 cpufreq_cpu_put(policy);
>> 1014 return 0;
>> 1015 }
>>
>> The _get() is not controlled by the frozen flag, but it still doesn't take a
>> refcount because of a subtle reason: per_cpu(cpufreq_cpu_data, cpu) was set to
>> NULL in __cpufreq_remove_dev() and the memory was saved away in fallback storage.
>> So, when __cpufreq_cpu_get() executes, it sees:
>>
>> 204 /* get the CPU */
>> 205 data = per_cpu(cpufreq_cpu_data, cpu);
>> 206
>> 207 if (!data)
>> 208 goto err_out_put_module;
>>
>> Thus, since data is NULL, cpufreq_cpu_get() won't take a refcount and will return
>> silently.
>
> Even if this wouldn't have happened, refcount wouldn't have been
> touched due to this code:
>
>> 1012 if (unlikely(policy)) {
>> 1013 cpufreq_cpu_put(policy);
>> 1014 return 0;
>> 1015 }
>
> i.e. If we get a valid policy structure, we siimply put the policy again
> and so decrement the incremented refcount.
Ah, yes!
>
> So, even if you don't keep the fallback storage, things should work
> without any issue (probably worth trying as this will get rid of a per
> cpu variable :))
>
No, I already tried that and it didn't work ;-( The thing is, we need the
__cpufreq_add_dev() code to call the ->init() routines of drivers etc. But if
it finds the policy structure, it will skip all of that initialization and happily
proceed. Which is precisely the cause of all the erratic behaviour we are seeing
(ie., lack of proper initialization post-resume).
So this approach keeps the memory preserved in a fallback storage and lets the
init code run to full completion without any issues.
Perhaps we could do some _more_ code reorganization in the future to take this
issue into account etc., but IMHO that might be non-trivial. I'm trying to keep
this as simple and straight-forward as possible as a first step, to atleast get
it properly working. (Changing the order in which init is done is kinda scary
since its hard to comprehend what assumptions we might be breaking!).
We can perhaps revisit your idea later and optimize out the extra per-cpu data.
>> Further down in __cpufreq_add_dev(), we restore the original memory, using
>> the frozen flag:
>>
>> 1037 if (frozen)
>> 1038 /* Restore the saved policy when doing light-weight init */
>> 1039 policy = cpufreq_policy_restore(cpu);
>> 1040 else
>> 1041 policy = cpufreq_policy_alloc();
>>
>>
>> So that is how we manage to fool cpufreq_cpu_get() into not taking a fresh
>> refcount while resuming :)
Regards,
Srivatsa S. Bhat
On 16 July 2013 14:59, Srivatsa S. Bhat
<[email protected]> wrote:
> On 07/16/2013 02:40 PM, Viresh Kumar wrote:
>> So, even if you don't keep the fallback storage, things should work
>> without any issue (probably worth trying as this will get rid of a per
>> cpu variable :))
>>
>
> No, I already tried that and it didn't work ;-( The thing is, we need the
> __cpufreq_add_dev() code to call the ->init() routines of drivers etc. But if
> it finds the policy structure, it will skip all of that initialization and happily
> proceed. Which is precisely the cause of all the erratic behaviour we are seeing
> (ie., lack of proper initialization post-resume).
I missed that point. :)
> So this approach keeps the memory preserved in a fallback storage and lets the
> init code run to full completion without any issues.
>
> Perhaps we could do some _more_ code reorganization in the future to take this
> issue into account etc., but IMHO that might be non-trivial. I'm trying to keep
> this as simple and straight-forward as possible as a first step, to atleast get
> it properly working. (Changing the order in which init is done is kinda scary
> since its hard to comprehend what assumptions we might be breaking!).
>
> We can perhaps revisit your idea later and optimize out the extra per-cpu data.
No, we don't need to optimize it that way. Current design looks good
for now.
On 07/16/2013 03:05 PM, Viresh Kumar wrote:
> On 16 July 2013 14:59, Srivatsa S. Bhat
> <[email protected]> wrote:
>> On 07/16/2013 02:40 PM, Viresh Kumar wrote:
>
>>> So, even if you don't keep the fallback storage, things should work
>>> without any issue (probably worth trying as this will get rid of a per
>>> cpu variable :))
>>>
>>
>> No, I already tried that and it didn't work ;-( The thing is, we need the
>> __cpufreq_add_dev() code to call the ->init() routines of drivers etc. But if
>> it finds the policy structure, it will skip all of that initialization and happily
>> proceed. Which is precisely the cause of all the erratic behaviour we are seeing
>> (ie., lack of proper initialization post-resume).
>
> I missed that point. :)
>
>> So this approach keeps the memory preserved in a fallback storage and lets the
>> init code run to full completion without any issues.
>>
>> Perhaps we could do some _more_ code reorganization in the future to take this
>> issue into account etc., but IMHO that might be non-trivial. I'm trying to keep
>> this as simple and straight-forward as possible as a first step, to atleast get
>> it properly working. (Changing the order in which init is done is kinda scary
>> since its hard to comprehend what assumptions we might be breaking!).
>>
>> We can perhaps revisit your idea later and optimize out the extra per-cpu data.
>
> No, we don't need to optimize it that way. Current design looks good
> for now.
Cool! Thanks :)
Regards,
Srivatsa S. Bhat
On 07/12/2013 12:23 AM, Srivatsa S. Bhat wrote:
> On 07/12/2013 04:03 AM, Rafael J. Wysocki wrote:
>> On Friday, July 12, 2013 03:45:17 AM Srivatsa S. Bhat wrote:
>>>
>>> Hi,
>>
>> Hi,
>>
>>> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
>>> some subtle regressions in the cpufreq subsystem during suspend/resume.
>>> This patchset is aimed at rectifying those problems, by fixing the regression
>>> as well as achieving the original goal of that commit in a proper way.
>>>
>>> Patch 1 reverts the above commit, and is CC'ed to stable.
>>>
>>> Patches 2 - 5 reorganize the code and have no functional impact, and can go
>>> in as general cleanups as well. This reorganization builds a base that the
>>> rest of the patches will make use of.
>>>
>>> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
>>> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
>>> across suspend/resume.
>>>
>>> All the patches apply on current mainline.
>>>
>>>
>>> Robert, Durgadoss, it would be great if you could try it out and see if it works
>>> well for your usecase. I tested it locally and cpufreq related files did retain
>>> their permissions across suspend/resume. Let me know if it works fine in your
>>> setup too.
>>>
>>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>>> whether their systems work fine after:
>>> a. applying only the first commit (this is what gets backported to stable)
>>> b. applying all the commits
>>>
>>> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
>>> testing this patchset. Though that patch also touches cpufreq subsystem, it
>>> doesn't affect this patchset in any way and there is absolutely no dependency
>>> between the two in terms of code. That fix just makes basic CPU hotplug work
>>> without locking up on current mainline).
>>>
>>> [1]. https://lkml.org/lkml/2013/7/10/611
>>>
>>>
>>> Thank you very much!
>>
>> Thanks Srivatsa!
>>
>> I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
>> hate them. This way we'll have some more testing coverage before they reach
>> the mainline hopefully.
>>
On 07/16/2013 01:25 AM, Rafael J. Wysocki wrote:> On Monday, July 15, 2013 07:38:02 PM Toralf Förster wrote:
> Sorry, I have no idea what 1#8 means.
sry - here again with full quote of the email :
I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
cycles fine and crashed the system at the 3rd attempt / one times just at
the 4th (blinking power led, no sysrq, ...).
Applying patch 1-8 on top of that tree differs in that way that it
crashes now the system even at the 1st attempt or at least at the 2nd
My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
Gentoo Linux - FWIW .config attached.
>
> Sounds great! Thanks a lot Rafael!
>
> Regards,
> Srivatsa S. Bhat
>
>
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
> On 07/12/2013 12:23 AM, Srivatsa S. Bhat wrote:
> > On 07/12/2013 04:03 AM, Rafael J. Wysocki wrote:
> >> On Friday, July 12, 2013 03:45:17 AM Srivatsa S. Bhat wrote:
> >>>
> >>> Hi,
> >>
> >> Hi,
> >>
> >>> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
> >>> some subtle regressions in the cpufreq subsystem during suspend/resume.
> >>> This patchset is aimed at rectifying those problems, by fixing the regression
> >>> as well as achieving the original goal of that commit in a proper way.
> >>>
> >>> Patch 1 reverts the above commit, and is CC'ed to stable.
> >>>
> >>> Patches 2 - 5 reorganize the code and have no functional impact, and can go
> >>> in as general cleanups as well. This reorganization builds a base that the
> >>> rest of the patches will make use of.
> >>>
> >>> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
> >>> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
> >>> across suspend/resume.
> >>>
> >>> All the patches apply on current mainline.
> >>>
> >>>
> >>> Robert, Durgadoss, it would be great if you could try it out and see if it works
> >>> well for your usecase. I tested it locally and cpufreq related files did retain
> >>> their permissions across suspend/resume. Let me know if it works fine in your
> >>> setup too.
> >>>
> >>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
> >>> whether their systems work fine after:
> >>> a. applying only the first commit (this is what gets backported to stable)
> >>> b. applying all the commits
> >>>
> >>> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
> >>> testing this patchset. Though that patch also touches cpufreq subsystem, it
> >>> doesn't affect this patchset in any way and there is absolutely no dependency
> >>> between the two in terms of code. That fix just makes basic CPU hotplug work
> >>> without locking up on current mainline).
> >>>
> >>> [1]. https://lkml.org/lkml/2013/7/10/611
> >>>
> >>>
> >>> Thank you very much!
> >>
> >> Thanks Srivatsa!
> >>
> >> I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
> >> hate them. This way we'll have some more testing coverage before they reach
> >> the mainline hopefully.
> >>
>
> On 07/16/2013 01:25 AM, Rafael J. Wysocki wrote:> On Monday, July 15, 2013 07:38:02 PM Toralf Förster wrote:
> > Sorry, I have no idea what 1#8 means.
>
> sry - here again with full quote of the email :
>
> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
> cycles fine and crashed the system at the 3rd attempt / one times just at
> the 4th (blinking power led, no sysrq, ...).
>
> Applying patch 1-8 on top of that tree differs in that way that it
> crashes now the system even at the 1st attempt or at least at the 2nd
>
> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
> Gentoo Linux - FWIW .config attached.
I think you'll need the fixes first, basically [1/8] from this series and
this: https://patchwork.kernel.org/patch/2827512/ .
Please try to run with these two things applied only and see how that goes.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 07/17/2013 03:02 AM, Rafael J. Wysocki wrote:
> On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
>> On 07/12/2013 12:23 AM, Srivatsa S. Bhat wrote:
>>> On 07/12/2013 04:03 AM, Rafael J. Wysocki wrote:
>>>> On Friday, July 12, 2013 03:45:17 AM Srivatsa S. Bhat wrote:
>>>>>
>>>>> Hi,
>>>>
>>>> Hi,
>>>>
>>>>> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
>>>>> some subtle regressions in the cpufreq subsystem during suspend/resume.
>>>>> This patchset is aimed at rectifying those problems, by fixing the regression
>>>>> as well as achieving the original goal of that commit in a proper way.
>>>>>
>>>>> Patch 1 reverts the above commit, and is CC'ed to stable.
>>>>>
>>>>> Patches 2 - 5 reorganize the code and have no functional impact, and can go
>>>>> in as general cleanups as well. This reorganization builds a base that the
>>>>> rest of the patches will make use of.
>>>>>
>>>>> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
>>>>> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
>>>>> across suspend/resume.
>>>>>
>>>>> All the patches apply on current mainline.
>>>>>
>>>>>
>>>>> Robert, Durgadoss, it would be great if you could try it out and see if it works
>>>>> well for your usecase. I tested it locally and cpufreq related files did retain
>>>>> their permissions across suspend/resume. Let me know if it works fine in your
>>>>> setup too.
>>>>>
>>>>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>>>>> whether their systems work fine after:
>>>>> a. applying only the first commit (this is what gets backported to stable)
>>>>> b. applying all the commits
>>>>>
>>>>> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
>>>>> testing this patchset. Though that patch also touches cpufreq subsystem, it
>>>>> doesn't affect this patchset in any way and there is absolutely no dependency
>>>>> between the two in terms of code. That fix just makes basic CPU hotplug work
>>>>> without locking up on current mainline).
>>>>>
>>>>> [1]. https://lkml.org/lkml/2013/7/10/611
>>>>>
>>>>>
>>>>> Thank you very much!
>>>>
>>>> Thanks Srivatsa!
>>>>
>>>> I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
>>>> hate them. This way we'll have some more testing coverage before they reach
>>>> the mainline hopefully.
>>>>
>>
>> On 07/16/2013 01:25 AM, Rafael J. Wysocki wrote:> On Monday, July 15, 2013 07:38:02 PM Toralf Förster wrote:
>>> Sorry, I have no idea what 1#8 means.
>>
>> sry - here again with full quote of the email :
>>
>> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
>> cycles fine and crashed the system at the 3rd attempt / one times just at
>> the 4th (blinking power led, no sysrq, ...).
>>
>> Applying patch 1-8 on top of that tree differs in that way that it
>> crashes now the system even at the 1st attempt or at least at the 2nd
>>
>> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
>> Gentoo Linux - FWIW .config attached.
>
> I think you'll need the fixes first, basically [1/8] from this series and
> this: https://patchwork.kernel.org/patch/2827512/ .
>
> Please try to run with these two things applied only and see how that goes.
>
In addition to what Rafael suggested above, also try running your kernel
with cpufreq completely turned off (CONFIG_CPU_FREQ=n). My patches touch only
cpufreq, so this experiment will tell us if your suspend/resume issues are
really related to cpufreq or not. If turning off cpufreq also breaks your
suspend/resume, then a fresh git-bisect might be the only way to go.
Regards,
Srivatsa S. Bhat
On 07/16/2013 11:32 PM, Rafael J. Wysocki wrote:
> On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
>> On 07/12/2013 12:23 AM, Srivatsa S. Bhat wrote:
>>> On 07/12/2013 04:03 AM, Rafael J. Wysocki wrote:
>>>> On Friday, July 12, 2013 03:45:17 AM Srivatsa S. Bhat wrote:
>>>>>
>>>>> Hi,
>>>>
>>>> Hi,
>>>>
>>>>> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
>>>>> some subtle regressions in the cpufreq subsystem during suspend/resume.
>>>>> This patchset is aimed at rectifying those problems, by fixing the regression
>>>>> as well as achieving the original goal of that commit in a proper way.
>>>>>
>>>>> Patch 1 reverts the above commit, and is CC'ed to stable.
>>>>>
>>>>> Patches 2 - 5 reorganize the code and have no functional impact, and can go
>>>>> in as general cleanups as well. This reorganization builds a base that the
>>>>> rest of the patches will make use of.
>>>>>
>>>>> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
>>>>> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
>>>>> across suspend/resume.
>>>>>
>>>>> All the patches apply on current mainline.
>>>>>
>>>>>
>>>>> Robert, Durgadoss, it would be great if you could try it out and see if it works
>>>>> well for your usecase. I tested it locally and cpufreq related files did retain
>>>>> their permissions across suspend/resume. Let me know if it works fine in your
>>>>> setup too.
>>>>>
>>>>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>>>>> whether their systems work fine after:
>>>>> a. applying only the first commit (this is what gets backported to stable)
>>>>> b. applying all the commits
>>>>>
>>>>> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
>>>>> testing this patchset. Though that patch also touches cpufreq subsystem, it
>>>>> doesn't affect this patchset in any way and there is absolutely no dependency
>>>>> between the two in terms of code. That fix just makes basic CPU hotplug work
>>>>> without locking up on current mainline).
>>>>>
>>>>> [1]. https://lkml.org/lkml/2013/7/10/611
>>>>>
>>>>>
>>>>> Thank you very much!
>>>>
>>>> Thanks Srivatsa!
>>>>
>>>> I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
>>>> hate them. This way we'll have some more testing coverage before they reach
>>>> the mainline hopefully.
>>>>
>>
>> On 07/16/2013 01:25 AM, Rafael J. Wysocki wrote:> On Monday, July 15, 2013 07:38:02 PM Toralf Förster wrote:
>>> Sorry, I have no idea what 1#8 means.
>>
>> sry - here again with full quote of the email :
>>
>> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
>> cycles fine and crashed the system at the 3rd attempt / one times just at
>> the 4th (blinking power led, no sysrq, ...).
>>
>> Applying patch 1-8 on top of that tree differs in that way that it
>> crashes now the system even at the 1st attempt or at least at the 2nd
>>
>> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
>> Gentoo Linux - FWIW .config attached.
>
> I think you'll need the fixes first, basically [1/8] from this series and
> this: https://patchwork.kernel.org/patch/2827512/ .
>
> Please try to run with these two things applied only and see how that goes.
>
> Thanks,
> Rafael
>
>
That was it.
Applying https://patchwork.kernel.org/patch/2827512/ and then patch
[1/8] on top of v3.11-rc1-8-g47188d3 works fine and solved the reported
issue.
Furthermore applying patches 2-8 works too - suspend/wakeup works fine
and frequencies are scaled right after wakeup at the T420.
Thx
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
On 07/17/2013 08:57 PM, Toralf Förster wrote:
> On 07/16/2013 11:32 PM, Rafael J. Wysocki wrote:
>> On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
>>> On 07/12/2013 12:23 AM, Srivatsa S. Bhat wrote:
>>>> On 07/12/2013 04:03 AM, Rafael J. Wysocki wrote:
>>>>> On Friday, July 12, 2013 03:45:17 AM Srivatsa S. Bhat wrote:
>>>>>>
>>>>>> Hi,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> Commit a66b2e (cpufreq: Preserve sysfs files across suspend/resume) caused
>>>>>> some subtle regressions in the cpufreq subsystem during suspend/resume.
>>>>>> This patchset is aimed at rectifying those problems, by fixing the regression
>>>>>> as well as achieving the original goal of that commit in a proper way.
>>>>>>
>>>>>> Patch 1 reverts the above commit, and is CC'ed to stable.
>>>>>>
>>>>>> Patches 2 - 5 reorganize the code and have no functional impact, and can go
>>>>>> in as general cleanups as well. This reorganization builds a base that the
>>>>>> rest of the patches will make use of.
>>>>>>
>>>>>> Patch 6 and 7 add a mechanism to perform light-weight init/tear-down of CPUs
>>>>>> in the cpufreq subsystem and finally patch 8 uses it to preserve sysfs files
>>>>>> across suspend/resume.
>>>>>>
>>>>>> All the patches apply on current mainline.
>>>>>>
>>>>>>
>>>>>> Robert, Durgadoss, it would be great if you could try it out and see if it works
>>>>>> well for your usecase. I tested it locally and cpufreq related files did retain
>>>>>> their permissions across suspend/resume. Let me know if it works fine in your
>>>>>> setup too.
>>>>>>
>>>>>> And I'd of course appreciate to hear from Dirk, Tianyu and Toralf to know
>>>>>> whether their systems work fine after:
>>>>>> a. applying only the first commit (this is what gets backported to stable)
>>>>>> b. applying all the commits
>>>>>>
>>>>>> (Note: I had to use Michael's fix[1] to avoid CPU hotplug deadlock while
>>>>>> testing this patchset. Though that patch also touches cpufreq subsystem, it
>>>>>> doesn't affect this patchset in any way and there is absolutely no dependency
>>>>>> between the two in terms of code. That fix just makes basic CPU hotplug work
>>>>>> without locking up on current mainline).
>>>>>>
>>>>>> [1]. https://lkml.org/lkml/2013/7/10/611
>>>>>>
>>>>>>
>>>>>> Thank you very much!
>>>>>
>>>>> Thanks Srivatsa!
>>>>>
>>>>> I'm going to take [1/8] for 3.11 and queue up the rest for 3.12 if people don't
>>>>> hate them. This way we'll have some more testing coverage before they reach
>>>>> the mainline hopefully.
>>>>>
>>>
>>> On 07/16/2013 01:25 AM, Rafael J. Wysocki wrote:> On Monday, July 15, 2013 07:38:02 PM Toralf Förster wrote:
>>>> Sorry, I have no idea what 1#8 means.
>>>
>>> sry - here again with full quote of the email :
>>>
>>> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
>>> cycles fine and crashed the system at the 3rd attempt / one times just at
>>> the 4th (blinking power led, no sysrq, ...).
>>>
>>> Applying patch 1-8 on top of that tree differs in that way that it
>>> crashes now the system even at the 1st attempt or at least at the 2nd
>>>
>>> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
>>> Gentoo Linux - FWIW .config attached.
>>
>> I think you'll need the fixes first, basically [1/8] from this series and
>> this: https://patchwork.kernel.org/patch/2827512/ .
>>
>> Please try to run with these two things applied only and see how that goes.
>>
>> Thanks,
>> Rafael
>>
>>
> That was it.
>
> Applying https://patchwork.kernel.org/patch/2827512/ and then patch
> [1/8] on top of v3.11-rc1-8-g47188d3 works fine and solved the reported
> issue.
>
> Furthermore applying patches 2-8 works too - suspend/wakeup works fine
> and frequencies are scaled right after wakeup at the T420.
>
Phew! Finally :-)
Thank you for all your testing efforts!
Regards,
Srivatsa S. Bhat
On 07/17/2013 09:19 PM, Srivatsa S. Bhat wrote:
> On 07/17/2013 08:57 PM, Toralf Förster wrote:
>> On 07/16/2013 11:32 PM, Rafael J. Wysocki wrote:
>>> On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
[...]
>>>> sry - here again with full quote of the email :
>>>>
>>>> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
>>>> cycles fine and crashed the system at the 3rd attempt / one times just at
>>>> the 4th (blinking power led, no sysrq, ...).
>>>>
>>>> Applying patch 1-8 on top of that tree differs in that way that it
>>>> crashes now the system even at the 1st attempt or at least at the 2nd
>>>>
>>>> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
>>>> Gentoo Linux - FWIW .config attached.
>>>
>>> I think you'll need the fixes first, basically [1/8] from this series and
>>> this: https://patchwork.kernel.org/patch/2827512/ .
>>>
>>> Please try to run with these two things applied only and see how that goes.
>>>
>>> Thanks,
>>> Rafael
>>>
>>>
>> That was it.
>>
>> Applying https://patchwork.kernel.org/patch/2827512/ and then patch
>> [1/8] on top of v3.11-rc1-8-g47188d3 works fine and solved the reported
>> issue.
>>
>> Furthermore applying patches 2-8 works too - suspend/wakeup works fine
>> and frequencies are scaled right after wakeup at the T420.
>>
>
> Phew! Finally :-)
>
> Thank you for all your testing efforts!
>
Rafael, Viresh, any thoughts on picking up patches 2-8 from this series
for 3.12?
>From the discussions on this thread so far, there are no pending issues:
Toralf verified that these patches work fine on his system, as he mentioned
above, and Tianyu Lan independently tested this patchset and found no
issues with them. Also, Viresh analyzed the refcounting used in the patches
and we came to the conclusion that there is no problem with them either.
Regards,
Srivatsa S. Bhat
On 07/21/2013 10:43 AM, Srivatsa S. Bhat wrote:
> On 07/17/2013 09:19 PM, Srivatsa S. Bhat wrote:
>> On 07/17/2013 08:57 PM, Toralf Förster wrote:
>>> On 07/16/2013 11:32 PM, Rafael J. Wysocki wrote:
>>>> On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
> [...]
>>>>> sry - here again with full quote of the email :
>>>>>
>>>>> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
>>>>> cycles fine and crashed the system at the 3rd attempt / one times just at
>>>>> the 4th (blinking power led, no sysrq, ...).
>>>>>
>>>>> Applying patch 1-8 on top of that tree differs in that way that it
>>>>> crashes now the system even at the 1st attempt or at least at the 2nd
>>>>>
>>>>> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
>>>>> Gentoo Linux - FWIW .config attached.
>>>>
>>>> I think you'll need the fixes first, basically [1/8] from this series and
>>>> this: https://patchwork.kernel.org/patch/2827512/ .
>>>>
>>>> Please try to run with these two things applied only and see how that goes.
>>>>
>>>> Thanks,
>>>> Rafael
>>>>
>>>>
>>> That was it.
>>>
>>> Applying https://patchwork.kernel.org/patch/2827512/ and then patch
>>> [1/8] on top of v3.11-rc1-8-g47188d3 works fine and solved the reported
>>> issue.
>>>
>>> Furthermore applying patches 2-8 works too - suspend/wakeup works fine
>>> and frequencies are scaled right after wakeup at the T420.
>>>
>>
>> Phew! Finally :-)
>>
>> Thank you for all your testing efforts!
>>
>
> Rafael, Viresh, any thoughts on picking up patches 2-8 from this series
> for 3.12?
What's about the additional patch Rafael adviced me in this thread:
https://patchwork.kernel.org/patch/2827512/
?
On my ThinkPad T420 I do have to apply this on top of 3.10.1 otherwise I
do suffer from the hang during s2ram.
>>From the discussions on this thread so far, there are no pending issues:
> Toralf verified that these patches work fine on his system, as he mentioned
> above, and Tianyu Lan independently tested this patchset and found no
> issues with them. Also, Viresh analyzed the refcounting used in the patches
> and we came to the conclusion that there is no problem with them either.
>
>
> Regards,
> Srivatsa S. Bhat
>
>
>
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
On 07/21/2013 03:10 PM, Toralf Förster wrote:
> On 07/21/2013 10:43 AM, Srivatsa S. Bhat wrote:
>> On 07/17/2013 09:19 PM, Srivatsa S. Bhat wrote:
>>> On 07/17/2013 08:57 PM, Toralf Förster wrote:
>>>> On 07/16/2013 11:32 PM, Rafael J. Wysocki wrote:
>>>>> On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
>> [...]
>>>>>> sry - here again with full quote of the email :
>>>>>>
>>>>>> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
>>>>>> cycles fine and crashed the system at the 3rd attempt / one times just at
>>>>>> the 4th (blinking power led, no sysrq, ...).
>>>>>>
>>>>>> Applying patch 1-8 on top of that tree differs in that way that it
>>>>>> crashes now the system even at the 1st attempt or at least at the 2nd
>>>>>>
>>>>>> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
>>>>>> Gentoo Linux - FWIW .config attached.
>>>>>
>>>>> I think you'll need the fixes first, basically [1/8] from this series and
>>>>> this: https://patchwork.kernel.org/patch/2827512/ .
>>>>>
>>>>> Please try to run with these two things applied only and see how that goes.
>>>>>
>>>>> Thanks,
>>>>> Rafael
>>>>>
>>>>>
>>>> That was it.
>>>>
>>>> Applying https://patchwork.kernel.org/patch/2827512/ and then patch
>>>> [1/8] on top of v3.11-rc1-8-g47188d3 works fine and solved the reported
>>>> issue.
>>>>
>>>> Furthermore applying patches 2-8 works too - suspend/wakeup works fine
>>>> and frequencies are scaled right after wakeup at the T420.
>>>>
>>>
>>> Phew! Finally :-)
>>>
>>> Thank you for all your testing efforts!
>>>
>>
>> Rafael, Viresh, any thoughts on picking up patches 2-8 from this series
>> for 3.12?
>
> What's about the additional patch Rafael adviced me in this thread:
> https://patchwork.kernel.org/patch/2827512/
> ?
>
> On my ThinkPad T420 I do have to apply this on top of 3.10.1 otherwise I
> do suffer from the hang during s2ram.
>
Rafael already picked it up and in fact its now in the mainline kernel
as commit:
commit e8d05276f236ee6435e78411f62be9714e0b9377
Author: Srivatsa S. Bhat <[email protected]>
Date: Tue Jul 16 22:46:48 2013 +0200
cpufreq: Revert commit 2f7021a8 to fix CPU hotplug regression
Thanks for following up on that fix, Toralf!
Regards,
Srivatsa S. Bhat
On Sunday, July 21, 2013 02:13:42 PM Srivatsa S. Bhat wrote:
> On 07/17/2013 09:19 PM, Srivatsa S. Bhat wrote:
> > On 07/17/2013 08:57 PM, Toralf Förster wrote:
> >> On 07/16/2013 11:32 PM, Rafael J. Wysocki wrote:
> >>> On Tuesday, July 16, 2013 05:15:14 PM Toralf Förster wrote:
> [...]
> >>>> sry - here again with full quote of the email :
> >>>>
> >>>> I applied patch [1/8] on top of v3.11-rc1-8-g47188d3 passes two s2ram/wakeup
> >>>> cycles fine and crashed the system at the 3rd attempt / one times just at
> >>>> the 4th (blinking power led, no sysrq, ...).
> >>>>
> >>>> Applying patch 1-8 on top of that tree differs in that way that it
> >>>> crashes now the system even at the 1st attempt or at least at the 2nd
> >>>>
> >>>> My hardware is a ThinkPad T420 with latest BIOS and a 32 bit stable
> >>>> Gentoo Linux - FWIW .config attached.
> >>>
> >>> I think you'll need the fixes first, basically [1/8] from this series and
> >>> this: https://patchwork.kernel.org/patch/2827512/ .
> >>>
> >>> Please try to run with these two things applied only and see how that goes.
> >>>
> >>> Thanks,
> >>> Rafael
> >>>
> >>>
> >> That was it.
> >>
> >> Applying https://patchwork.kernel.org/patch/2827512/ and then patch
> >> [1/8] on top of v3.11-rc1-8-g47188d3 works fine and solved the reported
> >> issue.
> >>
> >> Furthermore applying patches 2-8 works too - suspend/wakeup works fine
> >> and frequencies are scaled right after wakeup at the T420.
> >>
> >
> > Phew! Finally :-)
> >
> > Thank you for all your testing efforts!
> >
>
> Rafael, Viresh, any thoughts on picking up patches 2-8 from this series
> for 3.12?
>
> From the discussions on this thread so far, there are no pending issues:
> Toralf verified that these patches work fine on his system, as he mentioned
> above, and Tianyu Lan independently tested this patchset and found no
> issues with them. Also, Viresh analyzed the refcounting used in the patches
> and we came to the conclusion that there is no problem with them either.
Yes, I'm going to queue up that series for 3.12.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.