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(-)
->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); }
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);
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);
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 */
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
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
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.
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
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.
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; + } }
/*
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
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.
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; + } }
/*
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
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.
On 15-10-15, 21:35, Viresh Kumar wrote:
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 viresh.kumar@linaro.org
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Saravana Kannan -
Viresh Kumar