Skip to contentSkip to conversation

cpufreq: sysfs cleanup

PATCH In review Viresh Kumar 3 participants 17 messages Oct 15, 2015 – Oct 28, 2015 CPU frequency scaling framework

lore
Share & tools
lore

Viresh Kumar

author maintainer
#0 lore raw

v2 cover cpufreq: sysfs cleanup

Rafael,

This is in response to the other thread where you and Saravana were discussing this. It doesn't allow updating policy for offline CPUs yet, but does cleanup the sysfs stuff.

This is untested by me as I didn't had access to the hardware to test this week. But this is tested by Fengguang's build bot for some time now.

Will be good if one of you can test this out.

V1->V2:
- Change log updated for the first patch
- One patch already applied and the last one is a new patch based on Saravana's comments.
- Reviewed-by tags from Saravana.

Viresh Kumar (5):
cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask
cpufreq: create cpu/cpufreq at boot time
cpufreq: remove cpufreq_sysfs_{create|remove}_file()
cpufreq: create cpu/cpufreq/policyX directories
cpufreq: postfix policy directory with the first CPU in related_cpus

 drivers/cpufreq/cpufreq.c          | 103 +++++++------------------------------
 drivers/cpufreq/cpufreq_governor.c |  20 ++-----
 include/linux/cpufreq.h            |   5 --
 3 files changed, 24 insertions(+), 104 deletions(-)
Signature · 1 line
2.4.0
author maintainer
#1 lore raw

PATCH v2 1/5 cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

->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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/cpufreq.c +1 -1
index 8701dc559850..16b9e811ff01 100644
@@ -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);
 	}
Signature · 1 line
2.4.0
author maintainer
#2 lore raw

PATCH v2 2/5 cpufreq: create cpu/cpufreq at boot time

In reply to 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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c          | 32 ++------------------------------
 drivers/cpufreq/cpufreq_governor.c | 20 +++++---------------
 include/linux/cpufreq.h            |  2 --
 3 files changed, 7 insertions(+), 47 deletions(-)
drivers/cpufreq/cpufreq.c +2 -30
index 16b9e811ff01..c1fd57db50bd 100644
@@ -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);
drivers/cpufreq/cpufreq_governor.c +5 -15
index 750626d8fb03..11258c4c1b17 100644
@@ -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);
include/linux/cpufreq.h +0 -2
index dca22de98d94..338bf0e59bb8 100644
@@ -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);
Signature · 1 line
2.4.0
author maintainer
#3 lore raw

PATCH v2 3/5 cpufreq: remove cpufreq_sysfs_{create|remove}_file()

In reply to Viresh Kumar

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

Reviewed-by Saravana Kannan <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 22 +++++-----------------
 include/linux/cpufreq.h   |  2 --
 2 files changed, 5 insertions(+), 19 deletions(-)
drivers/cpufreq/cpufreq.c +5 -17
index c1fd57db50bd..04222e7bbc73 100644
@@ -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())
include/linux/cpufreq.h +0 -2
index 338bf0e59bb8..9623218d996a 100644
@@ -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);
Signature · 1 line
2.4.0
author maintainer
#4 lore raw

PATCH v2 4/5 cpufreq: create cpu/cpufreq/policyX directories

In reply to 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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 4 insertions(+), 31 deletions(-)
drivers/cpufreq/cpufreq.c +4 -30
index 04222e7bbc73..4fa2215cc6ec 100644
@@ -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)
include/linux/cpufreq.h +0 -1
index 9623218d996a..ef4c5b1a860f 100644
@@ -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 */
Signature · 1 line
2.4.0
Reviewed-by on v2 4/5
#5 lore raw

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

Quoted 6 lines · 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.

Quoted 8 lines

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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>

Since you've added a separate patch for making policyX more consistent:

Reviewed-by Saravana Kannan <[email protected]>

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

Quoted 10 lines
---
  drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
  include/linux/cpufreq.h   |  1 -
  2 files changed, 4 insertions(+), 31 deletions(-)
drivers/cpufreq/cpufreq.c +0 -0
index 04222e7bbc73..4fa2215cc6ec 100644

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy) · line 915

Quoted 9 lines
drivers/cpufreq/cpufreq.c +0 -3
@@ -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

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#6 lore raw

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

Quoted 13 lines · Saravana Kannan
Quoted 12 lines · 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.

Quoted 8 lines
Quoted 2 lines
Suggested-by Saravana Kannan <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>

Since you've added a separate patch for making policyX more consistent:

Reviewed-by Saravana Kannan <[email protected]>

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

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

Thanks,
Rafael

author maintainer
#7 lore raw

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

In reply to Saravana Kannan
Quoted 1 line · Saravana Kannan

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.

Quoted 3 lines

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.

Signature · 1 line
viresh
#8 lore raw

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

Quoted 6 lines · Viresh Kumar
Quoted 1 line · Saravana Kannan

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:

Quoted 7 lines

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

author maintainer
#9 lore raw

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

Quoted 4 lines · Rafael J. Wysocki

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.

Signature · 1 line
viresh
author maintainer
#10 lore raw

PATCH v2 5/5 cpufreq: postfix policy directory with the first CPU in related_cpus

In reply to 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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +10 -9
index 4fa2215cc6ec..3fe13875565d 100644
@@ -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;
+		}
 	}
 
 	/*
Signature · 1 line
2.4.0
#11 lore raw

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

Quoted 40 lines · 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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
  drivers/cpufreq/cpufreq.c | 19 ++++++++++---------
  1 file changed, 10 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +0 -8
index 4fa2215cc6ec..3fe13875565d 100644
@@ -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);

drivers/cpufreq/cpufreq.c static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) · line 1052

Quoted 5 lines
drivers/cpufreq/cpufreq.c +0 -1
@@ -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?

Quoted 5 lines
drivers/cpufreq/cpufreq.c +0 -0
  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. */

drivers/cpufreq/cpufreq.c static int cpufreq_online(unsigned int cpu) · line 1165

Quoted 10 lines
drivers/cpufreq/cpufreq.c +9 -0
  		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

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author maintainer
#12 lore raw

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

Quoted 44 lines · Saravana Kannan
Quoted 43 lines · 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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +0 -8
index 4fa2215cc6ec..3fe13875565d 100644
@@ -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;
Quoted 4 lines

drivers/cpufreq/cpufreq.c static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) · line 1052

Quoted 2 lines
drivers/cpufreq/cpufreq.c +0 -1
-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.

Quoted 27 lines
Quoted 5 lines
drivers/cpufreq/cpufreq.c +0 -0
 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. */

drivers/cpufreq/cpufreq.c static int cpufreq_online(unsigned int cpu) · line 1165

Quoted 10 lines
drivers/cpufreq/cpufreq.c +9 -0
 		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().

Quoted 7 lines

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.

Signature · 1 line
viresh
author maintainer
#13 lore raw

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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>

---
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(-)
drivers/cpufreq/cpufreq.c +11 -10
index 4fa2215cc6ec..7c48e7316d91 100644
@@ -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;
+		}
 	}
 
 	/*
Signature · 1 line
2.4.0
Reviewed-by on v3 5/5
#14 lore raw

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

Quoted 31 lines · 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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>

---
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(-)
drivers/cpufreq/cpufreq.c +0 -1
index 4fa2215cc6ec..7c48e7316d91 100644
@@ -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))

drivers/cpufreq/cpufreq.c static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) · line 1042

Quoted 10 lines
drivers/cpufreq/cpufreq.c +1 -7
  		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.

Quoted 16 lines
drivers/cpufreq/cpufreq.c +0 -2
  	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);

drivers/cpufreq/cpufreq.c static int cpufreq_online(unsigned int cpu) · line 1166

Quoted 10 lines
drivers/cpufreq/cpufreq.c +10 -0
+
+		/* 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

drivers/cpufreq/cpufreq.c static int cpufreq_online(unsigned int cpu) · line 1169

Quoted 4 lines
drivers/cpufreq/cpufreq.c +0 -0
  	}
 
  	/*
Reviewed-by Saravana Kannan <[email protected]>

-Saravana

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author maintainer
#15 lore raw

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

Quoted 5 lines · Saravana Kannan

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.

Signature · 1 line
viresh
author maintainer Tested-by on the v2 series
#16 lore raw
In reply to Viresh Kumar
Quoted 11 lines · Viresh Kumar

Rafael,

This is in response to the other thread where you and Saravana were discussing this. It doesn't allow updating policy for offline CPUs yet, but does cleanup the sysfs stuff.

This is untested by me as I didn't had access to the hardware to test this week. But this is tested by Fengguang's build bot for some time now.

Will be good if one of you can test this out.

I am back now and

Tested-by Viresh Kumar <[email protected]>
Signature · 1 line
viresh