From: viresh kumar <viresh.kumar@linaro.org>
To: Lan Tianyu <tianyu.lan@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
"cpufreq@vger.kernel.org" <cpufreq@vger.kernel.org>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2] Cpufreq: Make governor data on nonboot cpus across system suspend/resume
Date: Sun, 17 Nov 2013 09:43:14 +0530 [thread overview]
Message-ID: <5288425A.6000501@linaro.org> (raw)
In-Reply-To: <1384616184-6197-1-git-send-email-tianyu.lan@intel.com>
On 16 November 2013 21:06, Lan Tianyu <tianyu.lan@intel.com> 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)..
next prev parent reply other threads:[~2013-11-17 4:13 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-15 6:01 [PATCH 1/1] Cpufreq: Make governor data on nonboot cpus across system syspend/resume Lan Tianyu
2013-11-15 8:15 ` [Update PATCH 1/1] Cpufreq: Make governor data on nonboot cpus across system suspend/resume Lan Tianyu
2013-11-15 10:22 ` Viresh Kumar
2013-11-16 4:33 ` Lan Tianyu
2013-11-15 10:54 ` Viresh Kumar
2013-11-16 5:24 ` viresh kumar
2013-11-16 5:24 ` viresh kumar
2013-11-16 0:38 ` Rafael J. Wysocki
2013-11-16 3:59 ` Lan Tianyu
2013-11-16 14:41 ` Rafael J. Wysocki
2013-11-16 14:57 ` Viresh Kumar
2013-11-16 15:10 ` Rafael J. Wysocki
2013-11-16 15:09 ` Rafael J. Wysocki
2013-11-16 15:23 ` Lan Tianyu
2013-11-16 15:36 ` [PATCH V2] " Lan Tianyu
2013-11-17 4:13 ` viresh kumar [this message]
2013-11-17 14:44 ` Rafael J. Wysocki
2013-11-17 16:23 ` Viresh Kumar
2013-11-22 7:49 ` viresh kumar
2013-11-22 8:19 ` Lan Tianyu
2013-11-22 8:39 ` Viresh Kumar
2013-11-21 14:43 ` Rafael J. Wysocki
2013-11-21 15:54 ` Viresh Kumar
2013-11-21 21:43 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5288425A.6000501@linaro.org \
--to=viresh.kumar@linaro.org \
--cc=cpufreq@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=tianyu.lan@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.