* [PATCH] cpufreq: Refactor cpufreq_set_policy()
@ 2014-02-17 1:06 Rafael J. Wysocki
2014-02-17 5:21 ` Viresh Kumar
2014-02-18 9:10 ` Srivatsa S. Bhat
0 siblings, 2 replies; 3+ messages in thread
From: Rafael J. Wysocki @ 2014-02-17 1:06 UTC (permalink / raw)
To: Linux PM list; +Cc: Viresh Kumar, Linux Kernel Mailing List, Srivatsa S. Bhat
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reduce the rampant usage of goto and the indentation level in
cpufreq_set_policy() to improve the readability of that code.
No functional changes should result from that.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpufreq/cpufreq.c | 102 ++++++++++++++++++++--------------------------
1 file changed, 45 insertions(+), 57 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2018,22 +2018,21 @@ EXPORT_SYMBOL(cpufreq_get_policy);
static int cpufreq_set_policy(struct cpufreq_policy *policy,
struct cpufreq_policy *new_policy)
{
- int ret = 0, failed = 1;
+ struct cpufreq_governor *old_gov;
+ int ret;
pr_debug("setting new policy for CPU %u: %u - %u kHz\n", new_policy->cpu,
new_policy->min, new_policy->max);
memcpy(&new_policy->cpuinfo, &policy->cpuinfo, sizeof(policy->cpuinfo));
- if (new_policy->min > policy->max || new_policy->max < policy->min) {
- ret = -EINVAL;
- goto error_out;
- }
+ if (new_policy->min > policy->max || new_policy->max < policy->min)
+ return -EINVAL;
/* verify the cpu speed can be set within this limit */
ret = cpufreq_driver->verify(new_policy);
if (ret)
- goto error_out;
+ return ret;
/* adjust if necessary - all reasons */
blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
@@ -2049,7 +2048,7 @@ static int cpufreq_set_policy(struct cpu
*/
ret = cpufreq_driver->verify(new_policy);
if (ret)
- goto error_out;
+ return ret;
/* notification of the new policy */
blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
@@ -2064,58 +2063,47 @@ static int cpufreq_set_policy(struct cpu
if (cpufreq_driver->setpolicy) {
policy->policy = new_policy->policy;
pr_debug("setting range\n");
- ret = cpufreq_driver->setpolicy(new_policy);
- } else {
- if (new_policy->governor != policy->governor) {
- /* save old, working values */
- struct cpufreq_governor *old_gov = policy->governor;
-
- pr_debug("governor switch\n");
-
- /* end old governor */
- if (policy->governor) {
- __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
- up_write(&policy->rwsem);
- __cpufreq_governor(policy,
- CPUFREQ_GOV_POLICY_EXIT);
- down_write(&policy->rwsem);
- }
-
- /* start new governor */
- policy->governor = new_policy->governor;
- if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
- if (!__cpufreq_governor(policy, CPUFREQ_GOV_START)) {
- failed = 0;
- } else {
- up_write(&policy->rwsem);
- __cpufreq_governor(policy,
- CPUFREQ_GOV_POLICY_EXIT);
- down_write(&policy->rwsem);
- }
- }
-
- if (failed) {
- /* new governor failed, so re-start old one */
- pr_debug("starting governor %s failed\n",
- policy->governor->name);
- if (old_gov) {
- policy->governor = old_gov;
- __cpufreq_governor(policy,
- CPUFREQ_GOV_POLICY_INIT);
- __cpufreq_governor(policy,
- CPUFREQ_GOV_START);
- }
- ret = -EINVAL;
- goto error_out;
- }
- /* might be a policy change, too, so fall through */
- }
- pr_debug("governor: change or update limits\n");
- ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
+ return cpufreq_driver->setpolicy(new_policy);
}
+ if (new_policy->governor == policy->governor)
+ goto out;
+
+ pr_debug("governor switch\n");
+
+ /* save old, working values */
+ old_gov = policy->governor;
+ /* end old governor */
+ if (old_gov) {
+ __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
+ up_write(&policy->rwsem);
+ __cpufreq_governor(policy,CPUFREQ_GOV_POLICY_EXIT);
+ down_write(&policy->rwsem);
+ }
+
+ /* start new governor */
+ policy->governor = new_policy->governor;
+ if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
+ if (!__cpufreq_governor(policy, CPUFREQ_GOV_START))
+ goto out;
+
+ up_write(&policy->rwsem);
+ __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+ down_write(&policy->rwsem);
+ }
+
+ /* new governor failed, so re-start old one */
+ pr_debug("starting governor %s failed\n", policy->governor->name);
+ if (old_gov) {
+ policy->governor = old_gov;
+ __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT);
+ __cpufreq_governor(policy, CPUFREQ_GOV_START);
+ }
+
+ return -EINVAL;
-error_out:
- return ret;
+ out:
+ pr_debug("governor: change or update limits\n");
+ return __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
}
/**
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] cpufreq: Refactor cpufreq_set_policy()
2014-02-17 1:06 [PATCH] cpufreq: Refactor cpufreq_set_policy() Rafael J. Wysocki
@ 2014-02-17 5:21 ` Viresh Kumar
2014-02-18 9:10 ` Srivatsa S. Bhat
1 sibling, 0 replies; 3+ messages in thread
From: Viresh Kumar @ 2014-02-17 5:21 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM list, Linux Kernel Mailing List, Srivatsa S. Bhat
On 17 February 2014 06:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Reduce the rampant usage of goto and the indentation level in
> cpufreq_set_policy() to improve the readability of that code.
>
> No functional changes should result from that.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpufreq/cpufreq.c | 102 ++++++++++++++++++++--------------------------
> 1 file changed, 45 insertions(+), 57 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2018,22 +2018,21 @@ EXPORT_SYMBOL(cpufreq_get_policy);
> static int cpufreq_set_policy(struct cpufreq_policy *policy,
> struct cpufreq_policy *new_policy)
> {
> - int ret = 0, failed = 1;
> + struct cpufreq_governor *old_gov;
> + int ret;
>
> pr_debug("setting new policy for CPU %u: %u - %u kHz\n", new_policy->cpu,
> new_policy->min, new_policy->max);
>
> memcpy(&new_policy->cpuinfo, &policy->cpuinfo, sizeof(policy->cpuinfo));
>
> - if (new_policy->min > policy->max || new_policy->max < policy->min) {
> - ret = -EINVAL;
> - goto error_out;
> - }
> + if (new_policy->min > policy->max || new_policy->max < policy->min)
> + return -EINVAL;
>
> /* verify the cpu speed can be set within this limit */
> ret = cpufreq_driver->verify(new_policy);
> if (ret)
> - goto error_out;
> + return ret;
>
> /* adjust if necessary - all reasons */
> blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> @@ -2049,7 +2048,7 @@ static int cpufreq_set_policy(struct cpu
> */
> ret = cpufreq_driver->verify(new_policy);
> if (ret)
> - goto error_out;
> + return ret;
>
> /* notification of the new policy */
> blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> @@ -2064,58 +2063,47 @@ static int cpufreq_set_policy(struct cpu
> if (cpufreq_driver->setpolicy) {
> policy->policy = new_policy->policy;
> pr_debug("setting range\n");
> - ret = cpufreq_driver->setpolicy(new_policy);
> - } else {
> - if (new_policy->governor != policy->governor) {
> - /* save old, working values */
> - struct cpufreq_governor *old_gov = policy->governor;
> -
> - pr_debug("governor switch\n");
> -
> - /* end old governor */
> - if (policy->governor) {
> - __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> - up_write(&policy->rwsem);
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_POLICY_EXIT);
> - down_write(&policy->rwsem);
> - }
> -
> - /* start new governor */
> - policy->governor = new_policy->governor;
> - if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
> - if (!__cpufreq_governor(policy, CPUFREQ_GOV_START)) {
> - failed = 0;
> - } else {
> - up_write(&policy->rwsem);
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_POLICY_EXIT);
> - down_write(&policy->rwsem);
> - }
> - }
> -
> - if (failed) {
> - /* new governor failed, so re-start old one */
> - pr_debug("starting governor %s failed\n",
> - policy->governor->name);
> - if (old_gov) {
> - policy->governor = old_gov;
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_POLICY_INIT);
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_START);
> - }
> - ret = -EINVAL;
> - goto error_out;
> - }
> - /* might be a policy change, too, so fall through */
> - }
> - pr_debug("governor: change or update limits\n");
> - ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> + return cpufreq_driver->setpolicy(new_policy);
> }
Maybe a blank line here..
> + if (new_policy->governor == policy->governor)
> + goto out;
Otherwise: Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] cpufreq: Refactor cpufreq_set_policy()
2014-02-17 1:06 [PATCH] cpufreq: Refactor cpufreq_set_policy() Rafael J. Wysocki
2014-02-17 5:21 ` Viresh Kumar
@ 2014-02-18 9:10 ` Srivatsa S. Bhat
1 sibling, 0 replies; 3+ messages in thread
From: Srivatsa S. Bhat @ 2014-02-18 9:10 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM list, Viresh Kumar, Linux Kernel Mailing List
On 02/17/2014 06:36 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Reduce the rampant usage of goto and the indentation level in
> cpufreq_set_policy() to improve the readability of that code.
>
> No functional changes should result from that.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Regards,
Srivatsa S. Bhat
> ---
> drivers/cpufreq/cpufreq.c | 102 ++++++++++++++++++++--------------------------
> 1 file changed, 45 insertions(+), 57 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2018,22 +2018,21 @@ EXPORT_SYMBOL(cpufreq_get_policy);
> static int cpufreq_set_policy(struct cpufreq_policy *policy,
> struct cpufreq_policy *new_policy)
> {
> - int ret = 0, failed = 1;
> + struct cpufreq_governor *old_gov;
> + int ret;
>
> pr_debug("setting new policy for CPU %u: %u - %u kHz\n", new_policy->cpu,
> new_policy->min, new_policy->max);
>
> memcpy(&new_policy->cpuinfo, &policy->cpuinfo, sizeof(policy->cpuinfo));
>
> - if (new_policy->min > policy->max || new_policy->max < policy->min) {
> - ret = -EINVAL;
> - goto error_out;
> - }
> + if (new_policy->min > policy->max || new_policy->max < policy->min)
> + return -EINVAL;
>
> /* verify the cpu speed can be set within this limit */
> ret = cpufreq_driver->verify(new_policy);
> if (ret)
> - goto error_out;
> + return ret;
>
> /* adjust if necessary - all reasons */
> blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> @@ -2049,7 +2048,7 @@ static int cpufreq_set_policy(struct cpu
> */
> ret = cpufreq_driver->verify(new_policy);
> if (ret)
> - goto error_out;
> + return ret;
>
> /* notification of the new policy */
> blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> @@ -2064,58 +2063,47 @@ static int cpufreq_set_policy(struct cpu
> if (cpufreq_driver->setpolicy) {
> policy->policy = new_policy->policy;
> pr_debug("setting range\n");
> - ret = cpufreq_driver->setpolicy(new_policy);
> - } else {
> - if (new_policy->governor != policy->governor) {
> - /* save old, working values */
> - struct cpufreq_governor *old_gov = policy->governor;
> -
> - pr_debug("governor switch\n");
> -
> - /* end old governor */
> - if (policy->governor) {
> - __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> - up_write(&policy->rwsem);
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_POLICY_EXIT);
> - down_write(&policy->rwsem);
> - }
> -
> - /* start new governor */
> - policy->governor = new_policy->governor;
> - if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
> - if (!__cpufreq_governor(policy, CPUFREQ_GOV_START)) {
> - failed = 0;
> - } else {
> - up_write(&policy->rwsem);
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_POLICY_EXIT);
> - down_write(&policy->rwsem);
> - }
> - }
> -
> - if (failed) {
> - /* new governor failed, so re-start old one */
> - pr_debug("starting governor %s failed\n",
> - policy->governor->name);
> - if (old_gov) {
> - policy->governor = old_gov;
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_POLICY_INIT);
> - __cpufreq_governor(policy,
> - CPUFREQ_GOV_START);
> - }
> - ret = -EINVAL;
> - goto error_out;
> - }
> - /* might be a policy change, too, so fall through */
> - }
> - pr_debug("governor: change or update limits\n");
> - ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> + return cpufreq_driver->setpolicy(new_policy);
> }
> + if (new_policy->governor == policy->governor)
> + goto out;
> +
> + pr_debug("governor switch\n");
> +
> + /* save old, working values */
> + old_gov = policy->governor;
> + /* end old governor */
> + if (old_gov) {
> + __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> + up_write(&policy->rwsem);
> + __cpufreq_governor(policy,CPUFREQ_GOV_POLICY_EXIT);
> + down_write(&policy->rwsem);
> + }
> +
> + /* start new governor */
> + policy->governor = new_policy->governor;
> + if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
> + if (!__cpufreq_governor(policy, CPUFREQ_GOV_START))
> + goto out;
> +
> + up_write(&policy->rwsem);
> + __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
> + down_write(&policy->rwsem);
> + }
> +
> + /* new governor failed, so re-start old one */
> + pr_debug("starting governor %s failed\n", policy->governor->name);
> + if (old_gov) {
> + policy->governor = old_gov;
> + __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT);
> + __cpufreq_governor(policy, CPUFREQ_GOV_START);
> + }
> +
> + return -EINVAL;
>
> -error_out:
> - return ret;
> + out:
> + pr_debug("governor: change or update limits\n");
> + return __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> }
>
> /**
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-18 9:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-17 1:06 [PATCH] cpufreq: Refactor cpufreq_set_policy() Rafael J. Wysocki
2014-02-17 5:21 ` Viresh Kumar
2014-02-18 9:10 ` Srivatsa S. Bhat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).