Re: [PATCH] cpufreq: Fix cpufreq regression after suspend/resume

From: Srivatsa S. Bhat
Date: Wed Jul 10 2013 - 18:33:41 EST

Hi Toralf,

On 07/11/2013 02:20 AM, Toralf FÃrster wrote:
> I tested the patch several times on top of a66b2e5 - the origin issue is
> fixed but - erratically another issue now appears : all 4 cores are runs
> after wakeup at 2.6 GHz.
> The temporary hot fix is to switch between governor performance and
> ondemand for all 4 cores.
>
>

Thanks for all your testing efforts. The commit a66b2e5 took a shortcut to
achieve its goals but failed subtly in many aspects. Below is a patch (only
compile-tested) which tries to achieve the goal (preserve sysfs files) in
the proper way, by separating out the cpufreq-core bits from the sysfs bits.
You might want to give it a try on current mainline and see if it improves
anything.

Regards,
Srivatsa S. Bhat


(Applies on current mainline)
---

drivers/cpufreq/cpufreq.c | 144 ++++++++++++++++++++++++++-------------------
1 file changed, 82 insertions(+), 62 deletions(-)


diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6a015ad..28c690f 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,31 +867,10 @@ 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;

- memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
- /* assure that the starting sequence is run in __cpufreq_set_policy */
- policy->governor = NULL;
-
- /* set default policy */
- ret = __cpufreq_set_policy(policy, &new_policy);
- policy->user_policy.policy = policy->policy;
- policy->user_policy.governor = policy->governor;
-
- if (ret) {
- pr_debug("setting policy failed\n");
- if (cpufreq_driver->exit)
- cpufreq_driver->exit(policy);
- }
return ret;

err_out_kobj_put:
@@ -905,7 +881,7 @@ err_out_kobj_put:

#ifdef CONFIG_HOTPLUG_CPU
static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
- struct device *dev)
+ struct device *dev, bool init_sysfs)
{
struct cpufreq_policy *policy;
int ret = 0, has_target = !!cpufreq_driver->target;
@@ -933,30 +909,25 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
}

- ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
- if (ret) {
- cpufreq_cpu_put(policy);
- return ret;
+ if (init_sysfs) {
+ ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+ if (ret) {
+ cpufreq_cpu_put(policy);
+ return ret;
+ }
}

return 0;
}
#endif

-/**
- * 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 init_sysfs)
{
unsigned int j, cpu = dev->id;
int ret = -ENOMEM;
struct cpufreq_policy *policy;
+ struct cpufreq_policy new_policy;
unsigned long flags;
#ifdef CONFIG_HOTPLUG_CPU
struct cpufreq_governor *gov;
@@ -984,7 +955,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,
+ init_sysfs);
}
}
read_unlock_irqrestore(&cpufreq_driver_lock, flags);
@@ -1049,9 +1021,33 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
}
#endif

- ret = cpufreq_add_dev_interface(cpu, policy, dev);
- if (ret)
- goto err_out_unregister;
+ 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);
+
+ if (init_sysfs) {
+ ret = cpufreq_add_dev_interface(cpu, policy, dev);
+ if (ret)
+ goto err_out_unregister;
+ }
+
+ memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
+ /* assure that the starting sequence is run in __cpufreq_set_policy */
+ policy->governor = NULL;
+
+ /* set default policy */
+ ret = __cpufreq_set_policy(policy, &new_policy);
+ policy->user_policy.policy = policy->policy;
+ policy->user_policy.governor = policy->governor;
+
+ if (ret) {
+ pr_debug("setting policy failed\n");
+ if (cpufreq_driver->exit)
+ cpufreq_driver->exit(policy);
+ }

kobject_uevent(&policy->kobj, KOBJ_ADD);
module_put(cpufreq_driver->owner);
@@ -1081,6 +1077,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, true);
+}
+
static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
{
int j;
@@ -1106,7 +1116,7 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
* This routine frees the rwsem before returning.
*/
static int __cpufreq_remove_dev(struct device *dev,
- struct subsys_interface *sif)
+ struct subsys_interface *sif, bool remove_sysfs)
{
unsigned int cpu = dev->id, ret, cpus;
unsigned long flags;
@@ -1145,9 +1155,9 @@ 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 && remove_sysfs) {
sysfs_remove_link(&dev->kobj, "cpufreq");
- } else if (cpus > 1) {
+ } else if (cpus > 1 && remove_sysfs) {
/* first sibling now owns the new sysfs dir */
cpu_dev = get_cpu_device(cpumask_first(data->cpus));
sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
@@ -1184,26 +1194,25 @@ static int __cpufreq_remove_dev(struct device *dev,

/* 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 (cpufreq_driver->exit)
cpufreq_driver->exit(data);

free_cpumask_var(data->related_cpus);
free_cpumask_var(data->cpus);
kfree(data);
+
+ if (remove_sysfs) {
+ lock_policy_rwsem_read(cpu);
+ kobj = &data->kobj;
+ cmp = &data->kobj_unregister;
+ unlock_policy_rwsem_read(cpu);
+ kobject_put(kobj);
+
+ pr_debug("waiting for dropping of refcount\n");
+ wait_for_completion(cmp);
+ pr_debug("wait complete\n");
+ }
+
} else if (cpufreq_driver->target) {
__cpufreq_governor(data, CPUFREQ_GOV_START);
__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
@@ -1221,7 +1230,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, true);
return retval;
}

@@ -1943,13 +1952,24 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
case CPU_ONLINE:
cpufreq_add_dev(dev, NULL);
break;
+ case CPU_ONLINE_FROZEN:
+ __cpufreq_add_dev(dev, NULL, false);
+ break;
+
case CPU_DOWN_PREPARE:
case CPU_UP_CANCELED_FROZEN:
- __cpufreq_remove_dev(dev, NULL);
+ __cpufreq_remove_dev(dev, NULL, true);
+ break;
+ case CPU_DOWN_PREPARE_FROZEN:
+ __cpufreq_remove_dev(dev, NULL, false);
break;
+
case CPU_DOWN_FAILED:
cpufreq_add_dev(dev, NULL);
break;
+ case CPU_DOWN_FAILED_FROZEN:
+ __cpufreq_add_dev(dev, NULL, false);
+ break;
}
}
return NOTIFY_OK;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/