[PATCH V2 1/5] cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

[PATCH V2 1/5] cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

[Viresh Kumar]

->related_cpus is empty at this point of time and copying ->cpus to it
or orring ->related_cpus with ->cpus would result in the same value. But
cpumask_copy makes it rather clear.

Reviewed-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8701dc559850..16b9e811ff01 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1214,7 +1214,7 @@ static int cpufreq_online(unsigned int cpu)
 
 	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
-		cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
+		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);
 	}
-- 
2.4.0

[PATCH V2 2/5] cpufreq: create cpu/cpufreq at boot time

[Viresh Kumar]

Later patches will need to create policy specific directories in
/sys/devices/system/cpu/cpufreq/ directory and so the cpufreq directory
wouldn't be ever empty.

And so no fun creating/destroying it on need basis anymore. Create it
once on system boot.

Reviewed-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c          | 32 ++------------------------------
 drivers/cpufreq/cpufreq_governor.c | 20 +++++---------------
 include/linux/cpufreq.h            |  2 --
 3 files changed, 7 insertions(+), 47 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 16b9e811ff01..c1fd57db50bd 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -876,43 +876,15 @@ static struct kobj_type ktype_cpufreq = {
 struct kobject *cpufreq_global_kobject;
 EXPORT_SYMBOL(cpufreq_global_kobject);
 
-static int cpufreq_global_kobject_usage;
-
-int cpufreq_get_global_kobject(void)
-{
-	if (!cpufreq_global_kobject_usage++)
-		return kobject_add(cpufreq_global_kobject,
-				&cpu_subsys.dev_root->kobj, "%s", "cpufreq");
-
-	return 0;
-}
-EXPORT_SYMBOL(cpufreq_get_global_kobject);
-
-void cpufreq_put_global_kobject(void)
-{
-	if (!--cpufreq_global_kobject_usage)
-		kobject_del(cpufreq_global_kobject);
-}
-EXPORT_SYMBOL(cpufreq_put_global_kobject);
-
 int cpufreq_sysfs_create_file(const struct attribute *attr)
 {
-	int ret = cpufreq_get_global_kobject();
-
-	if (!ret) {
-		ret = sysfs_create_file(cpufreq_global_kobject, attr);
-		if (ret)
-			cpufreq_put_global_kobject();
-	}
-
-	return ret;
+	return sysfs_create_file(cpufreq_global_kobject, attr);
 }
 EXPORT_SYMBOL(cpufreq_sysfs_create_file);
 
 void cpufreq_sysfs_remove_file(const struct attribute *attr)
 {
 	sysfs_remove_file(cpufreq_global_kobject, attr);
-	cpufreq_put_global_kobject();
 }
 EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
 
@@ -2582,7 +2554,7 @@ static int __init cpufreq_core_init(void)
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	cpufreq_global_kobject = kobject_create();
+	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
 	BUG_ON(!cpufreq_global_kobject);
 
 	register_syscore_ops(&cpufreq_syscore_ops);
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 750626d8fb03..11258c4c1b17 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -348,29 +348,21 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 	set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
 					latency * LATENCY_MULTIPLIER));
 
-	if (!have_governor_per_policy()) {
-		if (WARN_ON(cpufreq_get_global_kobject())) {
-			ret = -EINVAL;
-			goto cdata_exit;
-		}
+	if (!have_governor_per_policy())
 		cdata->gdbs_data = dbs_data;
-	}
 
 	ret = sysfs_create_group(get_governor_parent_kobj(policy),
 				 get_sysfs_attr(dbs_data));
 	if (ret)
-		goto put_kobj;
+		goto reset_gdbs_data;
 
 	policy->governor_data = dbs_data;
 
 	return 0;
 
-put_kobj:
-	if (!have_governor_per_policy()) {
+reset_gdbs_data:
+	if (!have_governor_per_policy())
 		cdata->gdbs_data = NULL;
-		cpufreq_put_global_kobject();
-	}
-cdata_exit:
 	cdata->exit(dbs_data, !policy->governor->initialized);
 free_common_dbs_info:
 	free_common_dbs_info(policy, cdata);
@@ -394,10 +386,8 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
 		sysfs_remove_group(get_governor_parent_kobj(policy),
 				   get_sysfs_attr(dbs_data));
 
-		if (!have_governor_per_policy()) {
+		if (!have_governor_per_policy())
 			cdata->gdbs_data = NULL;
-			cpufreq_put_global_kobject();
-		}
 
 		cdata->exit(dbs_data, policy->governor->initialized == 1);
 		kfree(dbs_data);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dca22de98d94..338bf0e59bb8 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -149,8 +149,6 @@ static inline bool policy_is_shared(struct cpufreq_policy *policy)
 
 /* /sys/devices/system/cpu/cpufreq: entry point for global variables */
 extern struct kobject *cpufreq_global_kobject;
-int cpufreq_get_global_kobject(void);
-void cpufreq_put_global_kobject(void);
 int cpufreq_sysfs_create_file(const struct attribute *attr);
 void cpufreq_sysfs_remove_file(const struct attribute *attr);
 
-- 
2.4.0

[PATCH V2 3/5] cpufreq: remove cpufreq_sysfs_{create|remove}_file()

[Viresh Kumar]

They don't do anything special now, remove the unnecessary wrapper.

Reviewed-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 22 +++++-----------------
 include/linux/cpufreq.h   |  2 --
 2 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c1fd57db50bd..04222e7bbc73 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -873,21 +873,6 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-struct kobject *cpufreq_global_kobject;
-EXPORT_SYMBOL(cpufreq_global_kobject);
-
-int cpufreq_sysfs_create_file(const struct attribute *attr)
-{
-	return sysfs_create_file(cpufreq_global_kobject, attr);
-}
-EXPORT_SYMBOL(cpufreq_sysfs_create_file);
-
-void cpufreq_sysfs_remove_file(const struct attribute *attr)
-{
-	sysfs_remove_file(cpufreq_global_kobject, attr);
-}
-EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
-
 static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
 {
 	struct device *cpu_dev;
@@ -2390,7 +2375,7 @@ static int create_boost_sysfs_file(void)
 	if (!cpufreq_driver->set_boost)
 		cpufreq_driver->set_boost = cpufreq_boost_set_sw;
 
-	ret = cpufreq_sysfs_create_file(&boost.attr);
+	ret = sysfs_create_file(cpufreq_global_kobject, &boost.attr);
 	if (ret)
 		pr_err("%s: cannot register global BOOST sysfs file\n",
 		       __func__);
@@ -2401,7 +2386,7 @@ static int create_boost_sysfs_file(void)
 static void remove_boost_sysfs_file(void)
 {
 	if (cpufreq_boost_supported())
-		cpufreq_sysfs_remove_file(&boost.attr);
+		sysfs_remove_file(cpufreq_global_kobject, &boost.attr);
 }
 
 int cpufreq_enable_boost_support(void)
@@ -2549,6 +2534,9 @@ static struct syscore_ops cpufreq_syscore_ops = {
 	.shutdown = cpufreq_suspend,
 };
 
+struct kobject *cpufreq_global_kobject;
+EXPORT_SYMBOL(cpufreq_global_kobject);
+
 static int __init cpufreq_core_init(void)
 {
 	if (cpufreq_disabled())
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 338bf0e59bb8..9623218d996a 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -149,8 +149,6 @@ static inline bool policy_is_shared(struct cpufreq_policy *policy)
 
 /* /sys/devices/system/cpu/cpufreq: entry point for global variables */
 extern struct kobject *cpufreq_global_kobject;
-int cpufreq_sysfs_create_file(const struct attribute *attr);
-void cpufreq_sysfs_remove_file(const struct attribute *attr);
 
 #ifdef CONFIG_CPU_FREQ
 unsigned int cpufreq_get(unsigned int cpu);
-- 
2.4.0

[PATCH V2 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Viresh Kumar]

The cpufreq sysfs interface had been a bit inconsistent as one of the
CPUs for a policy had a real directory within its sysfs 'cpuX' directory
and all other CPUs had links to it. That also made the code a bit
complex as we need to take care of moving the sysfs directory if the CPU
containing the real directory is getting physically hot-unplugged.

Solve this by creating 'policyX' directories (per-policy) in
/sys/devices/system/cpu/cpufreq/ directory, where X is the CPU for which
the policy was first created.

This also removes the need of keeping kobj_cpu and we can remove it now.

Suggested-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 04222e7bbc73..4fa2215cc6ec 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -910,9 +910,6 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
 
 	/* Some related CPUs might not be present (physically hotplugged) */
 	for_each_cpu(j, policy->real_cpus) {
-		if (j == policy->kobj_cpu)
-			continue;
-
 		ret = add_cpu_dev_symlink(policy, j);
 		if (ret)
 			break;
@@ -926,12 +923,8 @@ static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
 	unsigned int j;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus) {
-		if (j == policy->kobj_cpu)
-			continue;
-
+	for_each_cpu(j, policy->real_cpus)
 		remove_cpu_dev_symlink(policy, j);
-	}
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -1047,8 +1040,8 @@ 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, &dev->kobj,
-				   "cpufreq");
+	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;
@@ -1062,10 +1055,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	INIT_WORK(&policy->update, handle_update);
 
 	policy->cpu = cpu;
-
-	/* Set this once on allocation */
-	policy->kobj_cpu = cpu;
-
 	return policy;
 
 err_free_real_cpus:
@@ -1417,22 +1406,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 		return;
 	}
 
-	if (cpu != policy->kobj_cpu) {
-		remove_cpu_dev_symlink(policy, cpu);
-	} else {
-		/*
-		 * The CPU owning the policy object is going away.  Move it to
-		 * another suitable CPU.
-		 */
-		unsigned int new_cpu = cpumask_first(policy->real_cpus);
-		struct device *new_dev = get_cpu_device(new_cpu);
-
-		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
-
-		sysfs_remove_link(&new_dev->kobj, "cpufreq");
-		policy->kobj_cpu = new_cpu;
-		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
-	}
+	remove_cpu_dev_symlink(policy, cpu);
 }
 
 static void handle_update(struct work_struct *work)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 9623218d996a..ef4c5b1a860f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -65,7 +65,6 @@ struct cpufreq_policy {
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
 	unsigned int		cpu;    /* cpu managing this policy, must be online */
-	unsigned int		kobj_cpu; /* cpu managing sysfs files, can be offline */
 
 	struct clk		*clk;
 	struct cpufreq_cpuinfo	cpuinfo;/* see above */
-- 
2.4.0

Re: [PATCH V2 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Saravana Kannan]

On 10/15/2015 09:05 AM, Viresh Kumar wrote:
> The cpufreq sysfs interface had been a bit inconsistent as one of the
> CPUs for a policy had a real directory within its sysfs 'cpuX' directory
> and all other CPUs had links to it. That also made the code a bit
> complex as we need to take care of moving the sysfs directory if the CPU
> containing the real directory is getting physically hot-unplugged.
>
> Solve this by creating 'policyX' directories (per-policy) in
> /sys/devices/system/cpu/cpufreq/ directory, where X is the CPU for which
> the policy was first created.
>
> This also removes the need of keeping kobj_cpu and we can remove it now.
>
> Suggested-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>


Since you've added a separate patch for making policyX more consistent:
Reviewed-by: Saravana Kannan <skannan@codeaurora.org>

Btw, does a Review-by have an implicit Acked-by?

> ---
>   drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
>   include/linux/cpufreq.h   |  1 -
>   2 files changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 04222e7bbc73..4fa2215cc6ec 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -910,9 +910,6 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
>
>   	/* Some related CPUs might not be present (physically hotplugged) */
>   	for_each_cpu(j, policy->real_cpus) {
> -		if (j == policy->kobj_cpu)
> -			continue;
> -
>   		ret = add_cpu_dev_symlink(policy, j);
>   		if (ret)
>   			break;

Kinda unrelated to this patch, but shouldn't this function undo the 
symlinks is has created so far before returning? Otherwise, we'd be 
leaving around broken symlinks.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V2 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Rafael J. Wysocki]

On Thursday, October 15, 2015 12:25:27 PM Saravana Kannan wrote:
> On 10/15/2015 09:05 AM, Viresh Kumar wrote:
> > The cpufreq sysfs interface had been a bit inconsistent as one of the
> > CPUs for a policy had a real directory within its sysfs 'cpuX' directory
> > and all other CPUs had links to it. That also made the code a bit
> > complex as we need to take care of moving the sysfs directory if the CPU
> > containing the real directory is getting physically hot-unplugged.
> >
> > Solve this by creating 'policyX' directories (per-policy) in
> > /sys/devices/system/cpu/cpufreq/ directory, where X is the CPU for which
> > the policy was first created.
> >
> > This also removes the need of keeping kobj_cpu and we can remove it now.
> >
> > Suggested-by: Saravana Kannan <skannan@codeaurora.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> 
> Since you've added a separate patch for making policyX more consistent:
> Reviewed-by: Saravana Kannan <skannan@codeaurora.org>
> 
> Btw, does a Review-by have an implicit Acked-by?

Yes it does, at least as far as I'm concerned.

Thanks,
Rafael

Re: [PATCH V2 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Viresh Kumar]

On 15-10-15, 12:25, Saravana Kannan wrote:
> Btw, does a Review-by have an implicit Acked-by?

I have attended a session at Linaro Connect where this was discussed
and the answer was:

Acked-by: is more of a general agreement from the person that he is
fine with the patch, but he might not have done a very careful review
and he isn't really responsible for the patch's content.

Reviewed-by: is a more strict tag and implies that the reviewer has
reviewed it at his best and he is as much responsible for the content
of the patch as the author.

> Kinda unrelated to this patch, but shouldn't this function undo the
> symlinks is has created so far before returning? Otherwise, we'd be
> leaving around broken symlinks.

Hmm, yeah. Will fix it separately.

-- 
viresh

Re: [PATCH V2 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Rafael J. Wysocki]

On Friday, October 16, 2015 11:21:18 AM Viresh Kumar wrote:
> On 15-10-15, 12:25, Saravana Kannan wrote:
> > Btw, does a Review-by have an implicit Acked-by?
> 
> I have attended a session at Linaro Connect where this was discussed
> and the answer was:
> 
> Acked-by: is more of a general agreement from the person that he is
> fine with the patch, but he might not have done a very careful review
> and he isn't really responsible for the patch's content.
> 
> Reviewed-by: is a more strict tag and implies that the reviewer has
> reviewed it at his best and he is as much responsible for the content
> of the patch as the author.

That's a bit too much IMO.  It means "I have carried out a detailed review
of this patch and haven't found problems in it."  How much responsibility
that implies is not so clear (evidently, there are differing opinions
regarding that).

Thanks,
Rafael

Re: [PATCH V2 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Viresh Kumar]

On 28-10-15, 09:16, Rafael J. Wysocki wrote:
> That's a bit too much IMO.  It means "I have carried out a detailed review
> of this patch and haven't found problems in it."  How much responsibility
> that implies is not so clear (evidently, there are differing opinions
> regarding that).

Right. I agree.

-- 
viresh

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

[Viresh Kumar]

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

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

[Saravana Kannan]

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

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

[Viresh Kumar]

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

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

[Viresh Kumar]

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

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

[Saravana Kannan]

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

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

[Viresh Kumar]

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