Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1247943 > unrolled thread

[PATCH V2 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V2 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-15 18:10 +0200
    Re: [PATCH V2 5/5] cpufreq: postfix policy directory with the first  CPU in related_cpus Saravana Kannan <skannan@codeaurora.org> - 2015-10-15 21:20 +0200
      Re: [PATCH V2 5/5] cpufreq: postfix policy directory with the first  CPU in related_cpus Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-16 09:10 +0200
        [PATCH V3 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-16 09:20 +0200
          Re: [PATCH V3 5/5] cpufreq: postfix policy directory with the first  CPU in related_cpus Saravana Kannan <skannan@codeaurora.org> - 2015-10-16 22:00 +0200
            Re: [PATCH V3 5/5] cpufreq: postfix policy directory with the first  CPU in related_cpus Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-17 06:30 +0200

#1247943 — [PATCH V2 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus

The sysfs policy directory is postfixed currently with the CPU number
for which the policy was created, which isn't necessarily the first CPU
in related_cpus mask.

To make it more consistent and predictable, lets postfix the policy with
the first cpu in related-cpus mask.

Suggested-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4fa2215cc6ec..3fe13875565d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1022,7 +1022,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
 	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
-	int ret;
 
 	if (WARN_ON(!dev))
 		return NULL;
@@ -1040,13 +1039,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
 		goto err_free_rcpumask;
 
-	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
-				   cpufreq_global_kobject, "policy%u", cpu);
-	if (ret) {
-		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
-		goto err_free_real_cpus;
-	}
-
 	INIT_LIST_HEAD(&policy->policy_list);
 	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
@@ -1057,7 +1049,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	policy->cpu = cpu;
 	return policy;
 
-err_free_real_cpus:
 	free_cpumask_var(policy->real_cpus);
 err_free_rcpumask:
 	free_cpumask_var(policy->related_cpus);
@@ -1163,6 +1154,16 @@ static int cpufreq_online(unsigned int cpu)
 		cpumask_copy(policy->related_cpus, policy->cpus);
 		/* Remember CPUs present at the policy creation time. */
 		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+
+		/* Initialize the kobject */
+		ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
+					   cpufreq_global_kobject, "policy%u",
+					   cpumask_first(policy->related_cpus));
+		if (ret) {
+			pr_err("%s: failed to init policy->kobj: %d\n",
+			       __func__, ret);
+			goto out_exit_policy;
+		}
 	}
 
 	/*
-- 
2.4.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1248095 — Re: [PATCH V2 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus

On 10/15/2015 09:05 AM, Viresh Kumar wrote:
> The sysfs policy directory is postfixed currently with the CPU number
> for which the policy was created, which isn't necessarily the first CPU
> in related_cpus mask.
>
> To make it more consistent and predictable, lets postfix the policy with
> the first cpu in related-cpus mask.
>
> Suggested-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/cpufreq.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 4fa2215cc6ec..3fe13875565d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1022,7 +1022,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   {
>   	struct device *dev = get_cpu_device(cpu);
>   	struct cpufreq_policy *policy;
> -	int ret;
>
>   	if (WARN_ON(!dev))
>   		return NULL;
> @@ -1040,13 +1039,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
>   		goto err_free_rcpumask;
>
> -	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
> -				   cpufreq_global_kobject, "policy%u", cpu);
> -	if (ret) {
> -		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
> -		goto err_free_real_cpus;
> -	}
> -
>   	INIT_LIST_HEAD(&policy->policy_list);
>   	init_rwsem(&policy->rwsem);
>   	spin_lock_init(&policy->transition_lock);
> @@ -1057,7 +1049,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	policy->cpu = cpu;
>   	return policy;
>
> -err_free_real_cpus:
>   	free_cpumask_var(policy->real_cpus);

Delete this line too? Does GCC not complain about unreachable code?

>   err_free_rcpumask:
>   	free_cpumask_var(policy->related_cpus);
> @@ -1163,6 +1154,16 @@ static int cpufreq_online(unsigned int cpu)
>   		cpumask_copy(policy->related_cpus, policy->cpus);
>   		/* Remember CPUs present at the policy creation time. */
>   		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
> +
> +		/* Initialize the kobject */
> +		ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
> +					   cpufreq_global_kobject, "policy%u",
> +					   cpumask_first(policy->related_cpus));
> +		if (ret) {
> +			pr_err("%s: failed to init policy->kobj: %d\n",
> +			       __func__, ret);
> +			goto out_exit_policy;

out_exit_policy label includes a call to cpufreq_policy_free(). That 
function needs to be changed to not call cpufreq_policy_put_kobj() in 
this case so that we don't try to kobject_put() an unallocated kobj.

Maybe you an call cpufreq_policy_put_kobj() in the error handling path 
of this function? Basically split out kojb alloc and free from policy 
alloc and free and alloc/free them around the same time 
(cpufreq_remove_Dev() will have to also call cpufreq_policy_put_kobj() 
when real_cpus is empty().

The refactor is just a suggestion. I'm looking at the latest code in a 
gitweb and making comments. So, I might have missed some corner cases in 
the refactor.

Also, it might be better to move the notifier from within 
cpufreq_policy_put_kobj() to cpufreq_policy_free()? Seems more appropriate.

Thanks,
Saravana


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248381 — Re: [PATCH V2 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus

On 15-10-15, 12:14, Saravana Kannan wrote:
> On 10/15/2015 09:05 AM, Viresh Kumar wrote:
> >The sysfs policy directory is postfixed currently with the CPU number
> >for which the policy was created, which isn't necessarily the first CPU
> >in related_cpus mask.
> >
> >To make it more consistent and predictable, lets postfix the policy with
> >the first cpu in related-cpus mask.
> >
> >Suggested-by: Saravana Kannan <skannan@codeaurora.org>
> >Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >---
> >  drivers/cpufreq/cpufreq.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >index 4fa2215cc6ec..3fe13875565d 100644
> >--- a/drivers/cpufreq/cpufreq.c
> >+++ b/drivers/cpufreq/cpufreq.c
> >@@ -1022,7 +1022,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> >  {
> >  	struct device *dev = get_cpu_device(cpu);
> >  	struct cpufreq_policy *policy;
> >-	int ret;
> >
> >  	if (WARN_ON(!dev))
> >  		return NULL;
> >@@ -1040,13 +1039,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> >  	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
> >  		goto err_free_rcpumask;
> >
> >-	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
> >-				   cpufreq_global_kobject, "policy%u", cpu);
> >-	if (ret) {
> >-		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
> >-		goto err_free_real_cpus;
> >-	}
> >-
> >  	INIT_LIST_HEAD(&policy->policy_list);
> >  	init_rwsem(&policy->rwsem);
> >  	spin_lock_init(&policy->transition_lock);
> >@@ -1057,7 +1049,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> >  	policy->cpu = cpu;
> >  	return policy;
> >
> >-err_free_real_cpus:
> >  	free_cpumask_var(policy->real_cpus);
> 
> Delete this line too? Does GCC not complain about unreachable code?

Ick.. Nah GCC never complained for it or may be it needs some other
compilation flags to complain about such cases.

> >  err_free_rcpumask:
> >  	free_cpumask_var(policy->related_cpus);
> >@@ -1163,6 +1154,16 @@ static int cpufreq_online(unsigned int cpu)
> >  		cpumask_copy(policy->related_cpus, policy->cpus);
> >  		/* Remember CPUs present at the policy creation time. */
> >  		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
> >+
> >+		/* Initialize the kobject */
> >+		ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
> >+					   cpufreq_global_kobject, "policy%u",
> >+					   cpumask_first(policy->related_cpus));
> >+		if (ret) {
> >+			pr_err("%s: failed to init policy->kobj: %d\n",
> >+			       __func__, ret);
> >+			goto out_exit_policy;
> 
> out_exit_policy label includes a call to cpufreq_policy_free(). That
> function needs to be changed to not call cpufreq_policy_put_kobj()
> in this case so that we don't try to kobject_put() an unallocated
> kobj.
> 
> Maybe you an call cpufreq_policy_put_kobj() in the error handling
> path of this function? Basically split out kojb alloc and free from
> policy alloc and free and alloc/free them around the same time
> (cpufreq_remove_Dev() will have to also call
> cpufreq_policy_put_kobj() when real_cpus is empty().
> 
> The refactor is just a suggestion. I'm looking at the latest code in
> a gitweb and making comments. So, I might have missed some corner
> cases in the refactor.
> 
> Also, it might be better to move the notifier from within
> cpufreq_policy_put_kobj() to cpufreq_policy_free()? Seems more
> appropriate.

Thanks for spotting a real bug here, I have another solution for this
though. Lemme resend that and you review it up.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248388 — [PATCH V3 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus

The sysfs policy directory is postfixed currently with the CPU number
for which the policy was created, which isn't necessarily the first CPU
in related_cpus mask.

To make it more consistent and predictable, lets postfix the policy with
the first cpu in related-cpus mask.

Suggested-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2->V3:
- Fix error path where we may try to put an uninitialized kobject.
- Break kobject_init_and_add() to kobject_init() and kobject_add().

 drivers/cpufreq/cpufreq.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4fa2215cc6ec..7c48e7316d91 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1022,7 +1022,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
 	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
-	int ret;
 
 	if (WARN_ON(!dev))
 		return NULL;
@@ -1040,13 +1039,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
 		goto err_free_rcpumask;
 
-	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
-				   cpufreq_global_kobject, "policy%u", cpu);
-	if (ret) {
-		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
-		goto err_free_real_cpus;
-	}
-
+	kobject_init(&policy->kobj, &ktype_cpufreq);
 	INIT_LIST_HEAD(&policy->policy_list);
 	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
@@ -1057,8 +1050,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	policy->cpu = cpu;
 	return policy;
 
-err_free_real_cpus:
-	free_cpumask_var(policy->real_cpus);
 err_free_rcpumask:
 	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:
@@ -1163,6 +1154,16 @@ static int cpufreq_online(unsigned int cpu)
 		cpumask_copy(policy->related_cpus, policy->cpus);
 		/* Remember CPUs present at the policy creation time. */
 		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+
+		/* Name and add the kobject */
+		ret = kobject_add(&policy->kobj, cpufreq_global_kobject,
+				  "policy%u",
+				  cpumask_first(policy->related_cpus));
+		if (ret) {
+			pr_err("%s: failed to add policy->kobj: %d\n", __func__,
+			       ret);
+			goto out_exit_policy;
+		}
 	}
 
 	/*
-- 
2.4.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1249114 — Re: [PATCH V3 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus

On 10/16/2015 12:11 AM, Viresh Kumar wrote:
> The sysfs policy directory is postfixed currently with the CPU number
> for which the policy was created, which isn't necessarily the first CPU
> in related_cpus mask.
>
> To make it more consistent and predictable, lets postfix the policy with
> the first cpu in related-cpus mask.
>
> Suggested-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2->V3:
> - Fix error path where we may try to put an uninitialized kobject.
> - Break kobject_init_and_add() to kobject_init() and kobject_add().
>
>   drivers/cpufreq/cpufreq.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 4fa2215cc6ec..7c48e7316d91 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1022,7 +1022,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   {
>   	struct device *dev = get_cpu_device(cpu);
>   	struct cpufreq_policy *policy;
> -	int ret;
>
>   	if (WARN_ON(!dev))
>   		return NULL;
> @@ -1040,13 +1039,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
>   		goto err_free_rcpumask;
>
> -	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
> -				   cpufreq_global_kobject, "policy%u", cpu);
> -	if (ret) {
> -		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
> -		goto err_free_real_cpus;
> -	}
> -
> +	kobject_init(&policy->kobj, &ktype_cpufreq);

Oh yeah, this works better. I forgot kobject has a separate init and add 
fuctions.

>   	INIT_LIST_HEAD(&policy->policy_list);
>   	init_rwsem(&policy->rwsem);
>   	spin_lock_init(&policy->transition_lock);
> @@ -1057,8 +1050,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	policy->cpu = cpu;
>   	return policy;
>
> -err_free_real_cpus:
> -	free_cpumask_var(policy->real_cpus);
>   err_free_rcpumask:
>   	free_cpumask_var(policy->related_cpus);
>   err_free_cpumask:
> @@ -1163,6 +1154,16 @@ static int cpufreq_online(unsigned int cpu)
>   		cpumask_copy(policy->related_cpus, policy->cpus);
>   		/* Remember CPUs present at the policy creation time. */
>   		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
> +
> +		/* Name and add the kobject */
> +		ret = kobject_add(&policy->kobj, cpufreq_global_kobject,
> +				  "policy%u",
> +				  cpumask_first(policy->related_cpus));
> +		if (ret) {
> +			pr_err("%s: failed to add policy->kobj: %d\n", __func__,
> +			       ret);
> +			goto out_exit_policy;
> +		}

Another out of patch issue that I see while reviewing this patch:

I think the existing error handling gotos aren't really cleaning things 
up well.

In the lines that follow this code we set the per_cpu(cpufreq_cpu_data) 
to point to the new policy. But if the subsequent cpu->get() fails, we 
goto out_exit_policy. But that label doesn't clean up the 
per_cpu(cpufreq_cpu_data). So, I think we need another label to jump to 
if ->get() fails

>   	}
>
>   	/*
>

Reviewed-by: Saravana Kannan <skannan@codeaurora.org>

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1249264 — Re: [PATCH V3 5/5] cpufreq: postfix policy directory with the first CPU in related_cpus

On 16-10-15, 12:50, Saravana Kannan wrote:
> In the lines that follow this code we set the
> per_cpu(cpufreq_cpu_data) to point to the new policy. But if the
> subsequent cpu->get() fails, we goto out_exit_policy. But that label
> doesn't clean up the per_cpu(cpufreq_cpu_data). So, I think we need
> another label to jump to if ->get() fails

We call cpufreq_policy_free() in that case and that does the cleanup
you are talking about.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web