[PATCH] cpufreq: Refactor cpufreq_set_policy()

STALE4618d

3 messages, 3 authors, 2014-02-18 · open the first message on its own page

[PATCH] cpufreq: Refactor cpufreq_set_policy()

From: Rafael J. Wysocki <hidden>
Date: 2014-02-17 00:51:29

From: Rafael J. Wysocki <redacted>

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

Re: [PATCH] cpufreq: Refactor cpufreq_set_policy()

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: 2014-02-17 05:21:19

On 17 February 2014 06:36, Rafael J. Wysocki [off-list ref] wrote:
quoted hunk
From: Rafael J. Wysocki <redacted>

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 <redacted>
---
 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 [off-list ref]

Re: [PATCH] cpufreq: Refactor cpufreq_set_policy()

From: Srivatsa S. Bhat <hidden>
Date: 2014-02-18 09:16:34

On 02/17/2014 06:36 AM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki <redacted>

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 <redacted>
Reviewed-by: Srivatsa S. Bhat <redacted>

Regards,
Srivatsa S. Bhat
quoted hunk
---
 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);
 }

 /**

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help