2014-02-17 00:51:29

by Rafael J. Wysocki

Subject: [PATCH] cpufreq: Refactor cpufreq_set_policy()

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>
---
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);
}

/**

2014-02-17 05:21:19

by Viresh Kumar

Subject: Re: [PATCH] cpufreq: Refactor cpufreq_set_policy()

On 17 February 2014 06:36, Rafael J. Wysocki <[email protected]> wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>

2014-02-18 09:16:34

by Srivatsa S. Bhat

Subject: Re: [PATCH] cpufreq: Refactor cpufreq_set_policy()

On 02/17/2014 06:36 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>

Reviewed-by: Srivatsa S. Bhat <[email protected]>

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);
> }
>
> /**
>