These labels are are named in two ways normally: - Based on what caused to jump to such labels - Based on what we do under such labels
We follow the first naming convention today and that leads to multiple labels for doing the same work. Fix it by switching to the second way of naming them.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Hi Rafael,
These can go in 4.3, I don't really mind :)
drivers/cpufreq/cpufreq.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b612411655f9..b7aac8eec525 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1278,7 +1278,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) recover_policy = false; policy = cpufreq_policy_alloc(dev); if (!policy) - goto nomem_out; + goto out_release_rwsem; }
cpumask_copy(policy->cpus, cpumask_of(cpu)); @@ -1289,7 +1289,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) ret = cpufreq_driver->init(policy); if (ret) { pr_debug("initialization failed\n"); - goto err_set_policy_cpu; + goto out_free_policy; }
down_write(&policy->rwsem); @@ -1317,7 +1317,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) policy->cur = cpufreq_driver->get(policy->cpu); if (!policy->cur) { pr_err("%s: ->get() failed\n", __func__); - goto err_get_freq; + goto out_exit_policy; } }
@@ -1367,7 +1367,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) if (!recover_policy) { ret = cpufreq_add_dev_interface(policy, dev); if (ret) - goto err_out_unregister; + goto out_exit_policy; blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_CREATE_POLICY, policy);
@@ -1396,15 +1396,14 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
return 0;
-err_out_unregister: -err_get_freq: +out_exit_policy: up_write(&policy->rwsem);
if (cpufreq_driver->exit) cpufreq_driver->exit(policy); -err_set_policy_cpu: +out_free_policy: cpufreq_policy_free(policy, recover_policy); -nomem_out: +out_release_rwsem: up_read(&cpufreq_rwsem);
return ret;
cpufreq_init_policy() can fail, and we don't do anything except a call to ->exit() on that. The policy should be freed if this happens.
Lets do it properly.
Reported-by: "Jon Medhurst (Tixy)" tixy@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b7aac8eec525..006299214d2e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1051,11 +1051,10 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy, return cpufreq_add_dev_symlink(policy); }
-static void cpufreq_init_policy(struct cpufreq_policy *policy) +static int cpufreq_init_policy(struct cpufreq_policy *policy) { struct cpufreq_governor *gov = NULL; struct cpufreq_policy new_policy; - int ret = 0;
memcpy(&new_policy, policy, sizeof(*policy));
@@ -1074,12 +1073,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy) cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);
/* set default policy */ - ret = cpufreq_set_policy(policy, &new_policy); - if (ret) { - pr_debug("setting policy failed\n"); - if (cpufreq_driver->exit) - cpufreq_driver->exit(policy); - } + return cpufreq_set_policy(policy, &new_policy); }
static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, @@ -1376,7 +1370,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) write_unlock_irqrestore(&cpufreq_driver_lock, flags); }
- cpufreq_init_policy(policy); + ret = cpufreq_init_policy(policy); + if (ret) { + pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n", + __func__, cpu, ret); + goto out_remove_policy_notify; + }
if (!recover_policy) { policy->user_policy.policy = policy->policy; @@ -1396,6 +1395,9 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
return 0;
+out_remove_policy_notify: + /* cpufreq_policy_free() will notify based on this */ + recover_policy = true; out_exit_policy: up_write(&policy->rwsem);
On Wed, 2015-07-08 at 15:12 +0530, Viresh Kumar wrote:
cpufreq_init_policy() can fail, and we don't do anything except a call to ->exit() on that. The policy should be freed if this happens.
Lets do it properly.
Reported-by: "Jon Medhurst (Tixy)" tixy@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I tried these patches without the earlier "cpufreq: Initialize the governor again while restoring policy" patch.
The result is that the error when bringing a cpu online is with flagged up with a kernel message:
cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16)
and afterwards, the sysfs entries that I was poking and causing the crash aren't present. So looks like this patch has done what we want, and cleaned things up after an error. So...
Tested-by: Jon Medhurst tixy@linaro.org
Thanks for the prompt fix.
drivers/cpufreq/cpufreq.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b7aac8eec525..006299214d2e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1051,11 +1051,10 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy, return cpufreq_add_dev_symlink(policy); } -static void cpufreq_init_policy(struct cpufreq_policy *policy) +static int cpufreq_init_policy(struct cpufreq_policy *policy) { struct cpufreq_governor *gov = NULL; struct cpufreq_policy new_policy;
- int ret = 0;
memcpy(&new_policy, policy, sizeof(*policy)); @@ -1074,12 +1073,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy) cpufreq_parse_governor(gov->name, &new_policy.policy, NULL); /* set default policy */
- ret = cpufreq_set_policy(policy, &new_policy);
- if (ret) {
pr_debug("setting policy failed\n");if (cpufreq_driver->exit)cpufreq_driver->exit(policy);- }
- return cpufreq_set_policy(policy, &new_policy);
} static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, @@ -1376,7 +1370,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) write_unlock_irqrestore(&cpufreq_driver_lock, flags); }
- cpufreq_init_policy(policy);
- ret = cpufreq_init_policy(policy);
- if (ret) {
pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n",__func__, cpu, ret);goto out_remove_policy_notify;- }
if (!recover_policy) { policy->user_policy.policy = policy->policy; @@ -1396,6 +1395,9 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) return 0; +out_remove_policy_notify:
- /* cpufreq_policy_free() will notify based on this */
- recover_policy = true;
out_exit_policy: up_write(&policy->rwsem);
On 08-07-15, 12:17, Jon Medhurst (Tixy) wrote:
I tried these patches without the earlier "cpufreq: Initialize the governor again while restoring policy" patch.
The result is that the error when bringing a cpu online is with flagged up with a kernel message:
cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16)
and afterwards, the sysfs entries that I was poking and causing the crash aren't present. So looks like this patch has done what we want, and cleaned things up after an error. So...
Tested-by: Jon Medhurst tixy@linaro.org
Thanks for the prompt fix.
And thanks for your help in getting these tested :)
On Wednesday, July 08, 2015 04:50:23 PM Viresh Kumar wrote:
On 08-07-15, 12:17, Jon Medhurst (Tixy) wrote:
I tried these patches without the earlier "cpufreq: Initialize the governor again while restoring policy" patch.
The result is that the error when bringing a cpu online is with flagged up with a kernel message:
cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16)
and afterwards, the sysfs entries that I was poking and causing the crash aren't present. So looks like this patch has done what we want, and cleaned things up after an error. So...
Tested-by: Jon Medhurst tixy@linaro.org
Thanks for the prompt fix.
And thanks for your help in getting these tested :)
Both queued up for 4.3, thanks!
On Thu, 2015-07-16 at 02:32 +0200, Rafael J. Wysocki wrote:
On Wednesday, July 08, 2015 04:50:23 PM Viresh Kumar wrote:
On 08-07-15, 12:17, Jon Medhurst (Tixy) wrote:
I tried these patches without the earlier "cpufreq: Initialize the governor again while restoring policy" patch.
The result is that the error when bringing a cpu online is with flagged up with a kernel message:
cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16)
and afterwards, the sysfs entries that I was poking and causing the crash aren't present. So looks like this patch has done what we want, and cleaned things up after an error. So...
Tested-by: Jon Medhurst tixy@linaro.org
Thanks for the prompt fix.
And thanks for your help in getting these tested :)
Both queued up for 4.3, thanks!
The crash I was getting was a regression caused by changes that went into 4.2-rc1.
Indeed, the first patch from Viresh is marked:
Fixes: 18bf3a124ef8 ("cpufreq: Mark policy->governor = NULL for inactive policies") For 4.2-rc
And I am having to carry that first patch to keep two ARM big.LITTLE platforms working.
On Thursday, July 16, 2015 10:16:14 AM Jon Medhurst wrote:
On Thu, 2015-07-16 at 02:32 +0200, Rafael J. Wysocki wrote:
On Wednesday, July 08, 2015 04:50:23 PM Viresh Kumar wrote:
On 08-07-15, 12:17, Jon Medhurst (Tixy) wrote:
I tried these patches without the earlier "cpufreq: Initialize the governor again while restoring policy" patch.
The result is that the error when bringing a cpu online is with flagged up with a kernel message:
cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16)
and afterwards, the sysfs entries that I was poking and causing the crash aren't present. So looks like this patch has done what we want, and cleaned things up after an error. So...
Tested-by: Jon Medhurst tixy@linaro.org
Thanks for the prompt fix.
And thanks for your help in getting these tested :)
Both queued up for 4.3, thanks!
The crash I was getting was a regression caused by changes that went into 4.2-rc1.
Indeed, the first patch from Viresh is marked:
Fixes: 18bf3a124ef8 ("cpufreq: Mark policy->governor = NULL for inactive policies") For 4.2-rc
And I am having to carry that first patch to keep two ARM big.LITTLE platforms working.
That one is going into 4.2-rc, I'm about to send a pull request with it.
linaro-kernel@lists.linaro.org
-
Jon Medhurst (Tixy) -
Rafael J. Wysocki -
Viresh Kumar