Re: [PATCH V2] Cpufreq: Make governor data on nonboot cpus acrosssystem suspend/resume
From: viresh kumar
Date: Sat Nov 16 2013 - 23:13:31 EST
On 16 November 2013 21:06, Lan Tianyu <tianyu.lan@xxxxxxxxx> wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> @@ -1818,9 +1818,13 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> mutex_unlock(&cpufreq_governor_lock);
> }
>
> - if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
> - ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> + if ((event == CPUFREQ_GOV_POLICY_INIT) && ret) {
> + module_put(policy->governor->owner);
> + if (ret == -EALREADY)
> + return 0;
> + } else if ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret) {
> module_put(policy->governor->owner);
> + }
This can still be written more efficiently I believe:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 138ebe9..54e28e1 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1866,10 +1866,12 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
ret = policy->governor->governor(policy, event);
if (!ret) {
- if (event == CPUFREQ_GOV_POLICY_INIT)
+ if (event == CPUFREQ_GOV_POLICY_INIT) {
policy->governor->initialized++;
- else if (event == CPUFREQ_GOV_POLICY_EXIT)
+ } else if (event == CPUFREQ_GOV_POLICY_EXIT) {
policy->governor->initialized--;
+ module_put(policy->governor->owner);
+ }
} else {
/* Restore original values */
mutex_lock(&cpufreq_governor_lock);
@@ -1877,13 +1879,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
policy->governor_enabled = true;
else if (event == CPUFREQ_GOV_START)
policy->governor_enabled = false;
+ else if (event == CPUFREQ_GOV_POLICY_INIT)
+ if (ret == -EALREADY) {
+ module_put(policy->governor->owner);
+ ret = 0;
+ }
mutex_unlock(&cpufreq_governor_lock);
}
- if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
- ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
- module_put(policy->governor->owner);
-
return ret;
> return ret;
> }
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 0806c31..ddb93af 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -204,9 +204,20 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>
> switch (event) {
> case CPUFREQ_GOV_POLICY_INIT:
> + /*
> + * In order to keep governor data across suspend/resume,
> + * Governor doesn't exit when suspend and will be
> + * reinitialized when resume. Here check policy governor
> + * data to determine whether the governor has been exited.
> + * If not, return EALREADY.
> + */
> if (have_governor_per_policy()) {
> - WARN_ON(dbs_data);
> + if (dbs_data)
> + return -EALREADY;
> } else if (dbs_data) {
> + if (policy->governor_data == dbs_data)
> + return -EALREADY;
> +
> dbs_data->usage_count++;
> policy->governor_data = dbs_data;
> return 0;
But I don't really like the solution here. You are handling frozen for EXIT in
cpufreq-core and for INIT in governor. That doesn't look like the right
approach. There are out of tree governors too (I know we don't care about them
:)), and those also need to adapt with some policy made at cpufreq-core level.
I told you that I had another solution for this problem, pretty similar to
yours. It looked like this:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4fa06de..e70e906 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -408,7 +408,8 @@ show_one(scaling_max_freq, max);
show_one(scaling_cur_freq, cur);
static int cpufreq_set_policy(struct cpufreq_policy *policy,
- struct cpufreq_policy *new_policy);
+ struct cpufreq_policy *new_policy,
+ bool frozen);
/**
* cpufreq_per_cpu_attr_write() / store_##file_name() - sysfs write access
@@ -428,7 +429,7 @@ static ssize_t store_##file_name \
if (ret != 1) \
return -EINVAL; \
\
- ret = cpufreq_set_policy(policy, &new_policy); \
+ ret = cpufreq_set_policy(policy, &new_policy, false); \
policy->user_policy.object = policy->object; \
\
return ret ? ret : count; \
@@ -486,7 +487,7 @@ static ssize_t store_scaling_governor(struct cpufreq_policy
*policy,
&new_policy.governor))
return -EINVAL;
- ret = cpufreq_set_policy(policy, &new_policy);
+ ret = cpufreq_set_policy(policy, &new_policy, false);
policy->user_policy.policy = policy->policy;
policy->user_policy.governor = policy->governor;
@@ -822,7 +823,7 @@ err_out_kobj_put:
return ret;
}
-static void cpufreq_init_policy(struct cpufreq_policy *policy)
+static void cpufreq_init_policy(struct cpufreq_policy *policy, bool frozen)
{
struct cpufreq_policy new_policy;
int ret = 0;
@@ -832,7 +833,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
policy->governor = NULL;
/* set default policy */
- ret = cpufreq_set_policy(policy, &new_policy);
+ ret = cpufreq_set_policy(policy, &new_policy, frozen);
policy->user_policy.policy = policy->policy;
policy->user_policy.governor = policy->governor;
@@ -1077,7 +1078,7 @@ static int __cpufreq_add_dev(struct device *dev, struct
subsys_interface *sif,
list_add(&policy->policy_list, &cpufreq_policy_list);
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
- cpufreq_init_policy(policy);
+ cpufreq_init_policy(policy, frozen);
kobject_uevent(&policy->kobj, KOBJ_ADD);
up_read(&cpufreq_rwsem);
@@ -1239,17 +1240,17 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
/* If cpu is last user of policy, free policy */
if (cpus == 1) {
- if (has_target()) {
- ret = __cpufreq_governor(policy,
- CPUFREQ_GOV_POLICY_EXIT);
- if (ret) {
- pr_err("%s: Failed to exit governor\n",
- __func__);
- return ret;
+ if (!frozen) {
+ if (has_target()) {
+ ret = __cpufreq_governor(policy,
+ CPUFREQ_GOV_POLICY_EXIT);
+ if (ret) {
+ pr_err("%s: Failed to exit governor\n",
+ __func__);
+ return ret;
+ }
}
- }
- if (!frozen) {
down_read(&policy->rwsem);
kobj = &policy->kobj;
cmp = &policy->kobj_unregister;
@@ -1920,7 +1921,7 @@ EXPORT_SYMBOL(cpufreq_get_policy);
* new_policy: policy to be set.
*/
static int cpufreq_set_policy(struct cpufreq_policy *policy,
- struct cpufreq_policy *new_policy)
+ struct cpufreq_policy *new_policy, bool frozen)
{
int ret = 0, failed = 1;
@@ -1987,7 +1988,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
/* start new governor */
policy->governor = new_policy->governor;
- if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
+ if (frozen || !__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
if (!__cpufreq_governor(policy, CPUFREQ_GOV_START)) {
failed = 0;
} else {
@@ -2065,7 +2066,7 @@ int cpufreq_update_policy(unsigned int cpu)
}
}
- ret = cpufreq_set_policy(policy, &new_policy);
+ ret = cpufreq_set_policy(policy, &new_policy, false);
up_write(&policy->rwsem);
-------------------------------
But after the PM notifiers patch I even don't want this to go.. I will make sure
that that patch goes in, in one form or another :)
So, just wait for sometime before posting a new version :) (I know you did it
because Rafael suggested a change)..
--
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/