[PATCH 1/2] cpufreq: cpufreq_add_dev: name goto labels based on what they do

[PATCH 1/2] cpufreq: cpufreq_add_dev: name goto labels based on what they do

[Viresh Kumar]

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;
-- 
2.4.0

[PATCH 2/2] cpufreq: Properly handle errors from cpufreq_init_policy()

[Viresh Kumar]

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);
 
-- 
2.4.0

Re: [PATCH 2/2] cpufreq: Properly handle errors from cpufreq_init_policy()

[Jon Medhurst (Tixy)]

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);
>  

Re: [PATCH 2/2] cpufreq: Properly handle errors from cpufreq_init_policy()

[Viresh Kumar]

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 :)

-- 
viresh

Re: [PATCH 2/2] cpufreq: Properly handle errors from cpufreq_init_policy()

[Rafael J. Wysocki]

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!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpufreq: Properly handle errors from cpufreq_init_policy()

[Jon Medhurst (Tixy)]

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.

-- 
Tixy

Re: [PATCH 2/2] cpufreq: Properly handle errors from cpufreq_init_policy()

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.