Re: [PATCH] cpufreq: Fix cpufreq regression after suspend/resume
From: Lan Tianyu
Date: Thu Jul 11 2013 - 01:41:09 EST
2013/7/11 Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>:
> 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;
This looks no right. "data" has already been freed but data-> kobj still is
referenced.
> + 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 cpufreq" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe 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/