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.
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: Drop redundant check for inactive policies
drivers/cpufreq/cpufreq.c | 95 +++++--------------------------------- drivers/cpufreq/cpufreq_governor.c | 20 ++------ include/linux/cpufreq.h | 5 -- 3 files changed, 16 insertions(+), 104 deletions(-)
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 25c4c15103a0..b32521432db4 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1221,7 +1221,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); }
On 10/11/2015 10:21 AM, Viresh Kumar wrote:
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
The commit text should explain the why you are doing this.
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 25c4c15103a0..b32521432db4 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1221,7 +1221,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);
Again, why? It actually seems wrong. A 4 core cluster could come up with just 2 cores when the policy is added. But the related CPUs would be 4 CPUs.
/* Remember CPUs present at the policy creation time. */ cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);}
-Saravana
On 12-10-15, 12:12, Saravana Kannan wrote:
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);Again, why? It actually seems wrong. A 4 core cluster could come up with just 2 cores when the policy is added. But the related CPUs would be 4 CPUs.
Firstly, the patch hasn't changed anything at all. related_cpus was empty until this point, and orring or setting it with ->cpus will result in the same output.
Secondly, this is what we always wanted. related_cpus should contain the mask of all possible CPUs for that cluster.
On 10/12/2015 08:23 PM, Viresh Kumar wrote:
On 12-10-15, 12:12, Saravana Kannan wrote:
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);Again, why? It actually seems wrong. A 4 core cluster could come up with just 2 cores when the policy is added. But the related CPUs would be 4 CPUs.
Firstly, the patch hasn't changed anything at all. related_cpus was empty until this point, and orring or setting it with ->cpus will result in the same output.
I was under the impression that the CPUfreq drivers were expected to fill in related_cpus and the or-ing was a safety net. If that's not the case, then this change is fine.
Secondly, this is what we always wanted. related_cpus should contain the mask of all possible CPUs for that cluster.
I think the confusion was that I thought the drivers are supposed to do this. If this doesn't mess up other CPUfreq drivers that I'm not familiar with, then I don't have concerns.
Can you still explain the why in the commit text though? If it's just that related_cpus is always empty and copying is more efficient than or-ing, then mention that?
Thanks, Saravana
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.
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 b32521432db4..db688d18a189 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -883,43 +883,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);
@@ -2589,7 +2561,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.
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 db688d18a189..2cb0e3b8170e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -880,21 +880,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; @@ -2397,7 +2382,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__); @@ -2408,7 +2393,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) @@ -2556,6 +2541,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.
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 2cb0e3b8170e..58aabe0f2d2c 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -917,9 +917,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; @@ -933,12 +930,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) @@ -1054,8 +1047,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; @@ -1069,10 +1062,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: @@ -1424,22 +1413,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/11/2015 10:21 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.
This should actually make hotplug a bit faster too.
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.
Can we use the first CPU in the related CPUs mask? Instead of the first CPU that the policy got created on? The policyX numbering would be a bit more consistent that way.
This also removes the need of keeping kobj_cpu and we can remove it now.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Suggested-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 2cb0e3b8170e..58aabe0f2d2c 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -917,9 +917,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) {
Didn't notice when this got added. Do we really need this anymore if we don't care about moving the directory and creating symlinks? I don't think we need it anymore. And if we really need to know related - offline, we can use for_each_cpu_and(related, online/present mask)
if (j == policy->kobj_cpu)continue;- ret = add_cpu_dev_symlink(policy, j); if (ret) break;
@@ -933,12 +930,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)
@@ -1054,8 +1047,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);@@ -1069,10 +1062,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:
@@ -1424,22 +1413,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 */
-Saravana
On 12-10-15, 12:31, Saravana Kannan wrote:
Can we use the first CPU in the related CPUs mask? Instead of the first CPU that the policy got created on? The policyX numbering would be a bit more consistent that way.
Okay..
Suggested-by: ?
Will add. Though me/Rafael thought about it long back, but then dropped the idea :)
Didn't notice when this got added. Do we really need this anymore if we don't care about moving the directory and creating symlinks? I don't think we need it anymore. And if we really need to know related - offline, we can use for_each_cpu_and(related, online/present mask)
Its about tracking present-cpus, for which the link is present. So, it is still required.
On 10/12/2015 08:39 PM, Viresh Kumar wrote:
On 12-10-15, 12:31, Saravana Kannan wrote:
Can we use the first CPU in the related CPUs mask? Instead of the first CPU that the policy got created on? The policyX numbering would be a bit more consistent that way.
Okay..
Suggested-by: ?
Will add. Though me/Rafael thought about it long back, but then dropped the idea :)
Didn't notice when this got added. Do we really need this anymore if we don't care about moving the directory and creating symlinks? I don't think we need it anymore. And if we really need to know related - offline, we can use for_each_cpu_and(related, online/present mask)
Its about tracking present-cpus, for which the link is present. So, it is still required.
But we don't need to track track of "present-cpus" separately though. We could do the for_each_cpu_and() when we create the symlinks for the first time. And after that, we can just use the subsystem interface callbacks (cpufreq_add_dev() and cpufreq_remove_dev()) to keep the symlinks updated.
I don't see any place where keeping track of this separately is more efficient. This would save some memory savings when the number of CPUs is large and also simplify the code because we won't have to keep another field up to date.
-Saravana
On 13-10-15, 12:29, Saravana Kannan wrote:
But we don't need to track track of "present-cpus" separately though. We could do the for_each_cpu_and() when we create the symlinks for the first time. And after that, we can just use the subsystem interface callbacks (cpufreq_add_dev() and cpufreq_remove_dev()) to keep the symlinks updated.
I don't see any place where keeping track of this separately is more efficient. This would save some memory savings when the number of CPUs is large and also simplify the code because we won't have to keep another field up to date.
It is still required to track when can we free the policy.
On 10/14/2015 11:55 PM, Viresh Kumar wrote:
On 13-10-15, 12:29, Saravana Kannan wrote:
But we don't need to track track of "present-cpus" separately though. We could do the for_each_cpu_and() when we create the symlinks for the first time. And after that, we can just use the subsystem interface callbacks (cpufreq_add_dev() and cpufreq_remove_dev()) to keep the symlinks updated.
I don't see any place where keeping track of this separately is more efficient. This would save some memory savings when the number of CPUs is large and also simplify the code because we won't have to keep another field up to date.
It is still required to track when can we free the policy.
Ok, I'm not very familiar with this new field's uses. I'll take a closer look later and respond if I have other thoughts.
Thanks, Saravana
On 12-10-15, 12:31, Saravana Kannan wrote:
Can we use the first CPU in the related CPUs mask? Instead of the first CPU that the policy got created on? The policyX numbering would be a bit more consistent that way.
Okay, checked this again. The problem is that ->init() isn't called yet and we are very early in the initialization sequence. So, we can't really know related_cpus yet. So I will keep it unchanged for now.
On 10/12/2015 11:15 PM, Viresh Kumar wrote:
On 12-10-15, 12:31, Saravana Kannan wrote:
Can we use the first CPU in the related CPUs mask? Instead of the first CPU that the policy got created on? The policyX numbering would be a bit more consistent that way.
Okay, checked this again. The problem is that ->init() isn't called yet and we are very early in the initialization sequence. So, we can't really know related_cpus yet. So I will keep it unchanged for now.
Can we move the sysfs add to the end so that by the time we add sysfs, we'll have all the details?
-Saravana
We just made sure policy->cpu is online and this check will always fail as the policy is active. Drop it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 58aabe0f2d2c..4fa2215cc6ec 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -843,18 +843,11 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
down_write(&policy->rwsem);
- /* Updating inactive policies is invalid, so avoid doing that. */ - if (unlikely(policy_is_inactive(policy))) { - ret = -EBUSY; - goto unlock_policy_rwsem; - } - if (fattr->store) ret = fattr->store(policy, buf, count); else ret = -EIO;
-unlock_policy_rwsem: up_write(&policy->rwsem); unlock: put_online_cpus();
On 10/11/2015 10:21 AM, Viresh Kumar wrote:
We just made sure policy->cpu is online and this check will always fail as the policy is active. Drop it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 58aabe0f2d2c..4fa2215cc6ec 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -843,18 +843,11 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
down_write(&policy->rwsem);
- /* Updating inactive policies is invalid, so avoid doing that. */
- if (unlikely(policy_is_inactive(policy))) {
ret = -EBUSY;goto unlock_policy_rwsem;- }
- if (fattr->store) ret = fattr->store(policy, buf, count); else ret = -EIO;
-unlock_policy_rwsem: up_write(&policy->rwsem); unlock: put_online_cpus();
Doesn't really seem related to the sysfs reorg/clean up. Should it be a separate patch outside of this series?
Acked-by: Saravana Kannan skannan@codeaurora.org
-Saravana
On 12-10-15, 12:35, Saravana Kannan wrote:
Doesn't really seem related to the sysfs reorg/clean up. Should it be a separate patch outside of this series?
Sent it separately now ..
Acked-by: Saravana Kannan skannan@codeaurora.org
A reviewed-by would have been more appropriate here though.
On 2105.10.11 10:21 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.
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: Drop redundant check for inactive policies
drivers/cpufreq/cpufreq.c | 95 +++++--------------------------------- drivers/cpufreq/cpufreq_governor.c | 20 ++------ include/linux/cpufreq.h | 5 -- 3 files changed, 16 insertions(+), 104 deletions(-)
-- 2.4.0
For my part of it, this patch set fixes the user space regression that was introduced into the pm-utils with kernel 4.2. If it ends up merged, I'll go back and cancel the bug reports I submitted.
References (mainly for myself): https://bugs.launchpad.net/ubuntu/+source/pm-utils/+bug/1493156 https://bugs.freedesktop.org/show_bug.cgi?id=91914
linaro-kernel@lists.linaro.org
-
Doug Smythies -
Saravana Kannan -
Viresh Kumar