On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+#ifdef CONFIG_CPU_FREQ_STAT +/* The cpufreq_debugfs is used to create debugfs root directory for CPUFreq. */ +static struct dentry *cpufreq_debugfs;
+static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
struct device *dev)+{
char name[CPUFREQ_NAME_LEN];unsigned int cpus, size, idx;if (!cpufreq_debugfs)return -EINVAL;cpus = cpumask_weight(policy->cpus);
I remember I told you not to use policy->cpus for this purpose?? But related_cpus.
idx = cpus > 1 ? policy->cpu : 0;
policy->cpu_debugfs[idx] = debugfs_create_dir(name, cpufreq_debugfs);
This is broken. A policy may contain cpus 9,10 only.. You will allocate array for 2 cpus and try to access cpu_debugfs[9] :)
if (!policy->cpu_debugfs[idx]) {pr_err("creating debugfs directory failed\n");return -ENODEV;}return 0;+}
+static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
unsigned int src_cpu,unsigned int dest_cpu)
Only use policy and cpu for which symlink has to be created as param to this routine. And create link to policy->cpu.
+{
char symlink_name[CPUFREQ_NAME_LEN];char target_name[CPUFREQ_NAME_LEN];if (!cpufreq_debugfs)return -EINVAL;if (!policy->cpu_debugfs[src_cpu])return -EINVAL;sprintf(symlink_name, "cpu%d", dest_cpu);sprintf(target_name, "./cpu%d", src_cpu);policy->cpu_debugfs[dest_cpu] = debugfs_create_symlink(symlink_name,cpufreq_debugfs,target_name);if (!policy->cpu_debugfs[dest_cpu]) {pr_err("creating debugfs symlink failed\n");return -ENODEV;}return 0;+}
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu)+{
unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;if (!policy->cpu_debugfs[idx])return;debugfs_remove_recursive(policy->cpu_debugfs[idx]);
Whey do we need recursive here? And what exactly does recursive will do?
+}
same problem here too.
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);
Why you need this? We aren't removing the earlier dentry at all here.
}/** Change debugfs directory name from as following:* - old debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${old_cpu}* - new debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${new_cpu}*/sprintf(new_dir_name, "cpu%d", new_cpu);old_entry = policy->cpu_debugfs[old_cpu];new_entry = debugfs_rename(cpufreq_debugfs, old_entry,cpufreq_debugfs, new_dir_name);
This routine returns old_entry only.. and so you can simply create a single routine with name dentry.
if (!new_entry) {pr_err("changing debugfs directory name failed\n");goto err_rename;}policy->cpu_debugfs[new_cpu] = new_entry;policy->cpu_debugfs[old_cpu] = NULL;/* Create again symbolic link of debugfs directory */for_each_present_cpu(j) {
present_cpu?? We discussed this before.. You will break multi cluster systems.
if (new_cpu == j)continue;cpufreq_create_debugfs_symlink(policy, new_cpu, j);}return;+err_rename:
for_each_present_cpu(j) {if (old_cpu == j)continue;cpufreq_create_debugfs_symlink(policy, old_cpu, j);}+}
+static int cpufreq_create_debugfs(void) +{
cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);if (!cpufreq_debugfs) {pr_err("creating debugfs root failed\n");return -ENODEV;}return 0;+}
+static void cpufreq_remove_debugfs(void) +{
if (cpufreq_debugfs)debugfs_remove_recursive(cpufreq_debugfs);+} +#else +static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
struct device *dev) { return 0; }+static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
unsigned int src_cpu,unsigned int dest_cpu) { return 0;}+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu) { }+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu) { }+static int cpufreq_create_debugfs(void) { return 0; } +static void cpufreq_remove_debugfs(void) { }
make all above #else part routines inline.
+#endif
/* symlink affected CPUs */ static int cpufreq_add_dev_symlink(unsigned int cpu, struct cpufreq_policy *policy) @@ -726,6 +885,8 @@ static int cpufreq_add_dev_symlink(unsigned int cpu, cpufreq_cpu_put(managed_policy); return ret; }
cpufreq_create_debugfs_symlink(policy, cpu, j); } return ret;} @@ -777,6 +938,9 @@ static int cpufreq_add_dev_interface(unsigned int cpu, } write_unlock_irqrestore(&cpufreq_driver_lock, flags);
/* prepare interface data for debugfs */cpufreq_create_debugfs_dir(policy, dev);ret = cpufreq_add_dev_symlink(cpu, policy); if (ret) goto err_out_kobj_put;@@ -839,6 +1003,8 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling, return ret; }
cpufreq_create_debugfs_symlink(policy, sibling, cpu);return 0;} #endif @@ -1046,6 +1212,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
if (cpu != data->cpu) { sysfs_remove_link(&dev->kobj, "cpufreq");
cpufreq_remove_debugfs_dir(data, cpu); } else if (cpus > 1) { /* first sibling now owns the new sysfs dir */ cpu_dev = get_cpu_device(cpumask_first(data->cpus));@@ -1068,6 +1235,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif return -EINVAL; }
cpufreq_move_debugfs_dir(data, cpu_dev->id);WARN_ON(lock_policy_rwsem_write(cpu)); update_policy_cpu(data, cpu_dev->id); unlock_policy_rwsem_write(cpu);@@ -1089,6 +1258,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif unlock_policy_rwsem_read(cpu); kobject_put(kobj);
cpufreq_remove_debugfs_dir(data, cpu);/* we need to make sure that the underlying kobj is actually * not referenced anymore by anybody before we proceed with * unloading.@@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
cpufreq_create_debugfs();
Why you moved this to register_driver? It was fine at cpufreq_core_init()
ret = subsys_interface_register(&cpufreq_interface); if (ret) goto err_null_driver;@@ -1918,12 +2091,14 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) }
register_hotcpu_notifier(&cpufreq_cpu_notifier);
unrelated change.
pr_debug("driver %s up and running\n", driver_data->name); return 0;err_if_unreg: subsys_interface_unregister(&cpufreq_interface); err_null_driver:
cpufreq_remove_debugfs(); write_lock_irqsave(&cpufreq_driver_lock, flags); cpufreq_driver = NULL; write_unlock_irqrestore(&cpufreq_driver_lock, flags);@@ -1949,6 +2124,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
pr_debug("unregistering driver %s\n", driver->name);
cpufreq_remove_debugfs();
And so you don't need this.
subsys_interface_unregister(&cpufreq_interface); unregister_hotcpu_notifier(&cpufreq_cpu_notifier);diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..825f379 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -115,6 +115,7 @@ struct cpufreq_policy {
struct kobject kobj; struct completion kobj_unregister;
struct dentry **cpu_debugfs;};
#define CPUFREQ_ADJUST (0)
1.8.0
Hi Viresh,
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+#ifdef CONFIG_CPU_FREQ_STAT +/* The cpufreq_debugfs is used to create debugfs root directory for CPUFreq. */ +static struct dentry *cpufreq_debugfs;
+static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
struct device *dev)+{
char name[CPUFREQ_NAME_LEN];unsigned int cpus, size, idx;if (!cpufreq_debugfs)return -EINVAL;cpus = cpumask_weight(policy->cpus);I remember I told you not to use policy->cpus for this purpose?? But related_cpus.
You're right. I'll use policy->related_cpus instead of policy->cpus.
idx = cpus > 1 ? policy->cpu : 0;
policy->cpu_debugfs[idx] = debugfs_create_dir(name, cpufreq_debugfs);This is broken. A policy may contain cpus 9,10 only.. You will allocate array for 2 cpus and try to access cpu_debugfs[9] :)
Right, I'll consider other method to resolve issue related to index of array.
if (!policy->cpu_debugfs[idx]) {pr_err("creating debugfs directory failed\n");return -ENODEV;}return 0;+}
+static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
unsigned int src_cpu,unsigned int dest_cpu)Only use policy and cpu for which symlink has to be created as param to this routine. And create link to policy->cpu.
OK, I'll simplify function prototype(cpufreq_create_debugfs_symlink) by removing unnecessary parameter.
+{
char symlink_name[CPUFREQ_NAME_LEN];char target_name[CPUFREQ_NAME_LEN];if (!cpufreq_debugfs)return -EINVAL;if (!policy->cpu_debugfs[src_cpu])return -EINVAL;sprintf(symlink_name, "cpu%d", dest_cpu);sprintf(target_name, "./cpu%d", src_cpu);policy->cpu_debugfs[dest_cpu] = debugfs_create_symlink(symlink_name,cpufreq_debugfs,target_name);if (!policy->cpu_debugfs[dest_cpu]) {pr_err("creating debugfs symlink failed\n");return -ENODEV;}return 0;+}
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu)+{
unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;if (!policy->cpu_debugfs[idx])return;debugfs_remove_recursive(policy->cpu_debugfs[idx]);Whey do we need recursive here? And what exactly does recursive will do?
If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
+}
same problem here too.
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);Why you need this? We aren't removing the earlier dentry at all here.
}/** Change debugfs directory name from as following:* - old debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${old_cpu}* - new debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${new_cpu}*/sprintf(new_dir_name, "cpu%d", new_cpu);old_entry = policy->cpu_debugfs[old_cpu];new_entry = debugfs_rename(cpufreq_debugfs, old_entry,cpufreq_debugfs, new_dir_name);This routine returns old_entry only.. and so you can simply create a single routine with name dentry.
I used 'new_entry' variable to improve readability to distinguish between old_entry and new_entry. But, as your comment, I'll simplify this statement to remove unnecessary code.
if (!new_entry) {pr_err("changing debugfs directory name failed\n");goto err_rename;}policy->cpu_debugfs[new_cpu] = new_entry;policy->cpu_debugfs[old_cpu] = NULL;/* Create again symbolic link of debugfs directory */for_each_present_cpu(j) {present_cpu?? We discussed this before.. You will break multi cluster systems.
My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
if (new_cpu == j)continue;cpufreq_create_debugfs_symlink(policy, new_cpu, j);}return;+err_rename:
for_each_present_cpu(j) {if (old_cpu == j)continue;cpufreq_create_debugfs_symlink(policy, old_cpu, j);}+}
+static int cpufreq_create_debugfs(void) +{
cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);if (!cpufreq_debugfs) {pr_err("creating debugfs root failed\n");return -ENODEV;}return 0;+}
+static void cpufreq_remove_debugfs(void) +{
if (cpufreq_debugfs)debugfs_remove_recursive(cpufreq_debugfs);+} +#else +static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
struct device *dev) { return 0; }+static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
unsigned int src_cpu,unsigned int dest_cpu) { return 0;}+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu) { }+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu) { }+static int cpufreq_create_debugfs(void) { return 0; } +static void cpufreq_remove_debugfs(void) { }
make all above #else part routines inline.
OK.
+#endif
/* symlink affected CPUs */ static int cpufreq_add_dev_symlink(unsigned int cpu, struct cpufreq_policy *policy) @@ -726,6 +885,8 @@ static int cpufreq_add_dev_symlink(unsigned int cpu, cpufreq_cpu_put(managed_policy); return ret; }
cpufreq_create_debugfs_symlink(policy, cpu, j); } return ret;} @@ -777,6 +938,9 @@ static int cpufreq_add_dev_interface(unsigned int cpu, } write_unlock_irqrestore(&cpufreq_driver_lock, flags);
/* prepare interface data for debugfs */cpufreq_create_debugfs_dir(policy, dev);ret = cpufreq_add_dev_symlink(cpu, policy); if (ret) goto err_out_kobj_put;@@ -839,6 +1003,8 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling, return ret; }
cpufreq_create_debugfs_symlink(policy, sibling, cpu);return 0;} #endif @@ -1046,6 +1212,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
if (cpu != data->cpu) { sysfs_remove_link(&dev->kobj, "cpufreq");
cpufreq_remove_debugfs_dir(data, cpu); } else if (cpus > 1) { /* first sibling now owns the new sysfs dir */ cpu_dev = get_cpu_device(cpumask_first(data->cpus));@@ -1068,6 +1235,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif return -EINVAL; }
cpufreq_move_debugfs_dir(data, cpu_dev->id);WARN_ON(lock_policy_rwsem_write(cpu)); update_policy_cpu(data, cpu_dev->id); unlock_policy_rwsem_write(cpu);@@ -1089,6 +1258,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif unlock_policy_rwsem_read(cpu); kobject_put(kobj);
cpufreq_remove_debugfs_dir(data, cpu);/* we need to make sure that the underlying kobj is actually * not referenced anymore by anybody before we proceed with * unloading.@@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
cpufreq_create_debugfs();Why you moved this to register_driver? It was fine at cpufreq_core_init()
If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit(). Do you agree about creating cpufreq_core_exit()(?
ret = subsys_interface_register(&cpufreq_interface); if (ret) goto err_null_driver;@@ -1918,12 +2091,14 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) }
register_hotcpu_notifier(&cpufreq_cpu_notifier);
unrelated change.
OK, I'll remove it.
pr_debug("driver %s up and running\n", driver_data->name); return 0;err_if_unreg: subsys_interface_unregister(&cpufreq_interface); err_null_driver:
cpufreq_remove_debugfs(); write_lock_irqsave(&cpufreq_driver_lock, flags); cpufreq_driver = NULL; write_unlock_irqrestore(&cpufreq_driver_lock, flags);@@ -1949,6 +2124,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
pr_debug("unregistering driver %s\n", driver->name);
cpufreq_remove_debugfs();And so you don't need this.
subsys_interface_unregister(&cpufreq_interface); unregister_hotcpu_notifier(&cpufreq_cpu_notifier);diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..825f379 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -115,6 +115,7 @@ struct cpufreq_policy {
struct kobject kobj; struct completion kobj_unregister;
struct dentry **cpu_debugfs;};
#define CPUFREQ_ADJUST (0)
1.8.0
Thanks for your comment.
Best Regards, Chanwoo Choi
On 24 July 2013 06:55, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu)+{
unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;if (!policy->cpu_debugfs[idx])return;debugfs_remove_recursive(policy->cpu_debugfs[idx]);Whey do we need recursive here? And what exactly does recursive will do?
If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
You are calling this routine even when we aren't at the last cpu of a policy. And so, eventually you are calling this routine for a link you have created.
Have you actually tested your code? What kind of platform? What is cpu topology ?? And what exactly you tested..
We are already on v6 and this patch still looks like the v1.. It still has lots of basic mistakes, which I don't expect so later in the series..
Its very difficult for me to review the same patchset again and again.. So, normally people might not review it well after v3-v4 and just trust the sender.. But I am nowhere close to getting that.. And so discouraged to review it..
Please review/test it well on multiple kind of systems if possible. Test on your intel laptop and see if it has multiple policy structures with multiple cpus in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy structure.
+}
same problem here too.
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);Why you need this? We aren't removing the earlier dentry at all here.
haven't answered this.
if (!new_entry) {pr_err("changing debugfs directory name failed\n");goto err_rename;}policy->cpu_debugfs[new_cpu] = new_entry;policy->cpu_debugfs[old_cpu] = NULL;/* Create again symbolic link of debugfs directory */for_each_present_cpu(j) {present_cpu?? We discussed this before.. You will break multi cluster systems.
My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
Go through earlier comments about this.. you are still wrong.. You need to run over cpus that are in this policy.. i.e. policy->cpus.
if (new_cpu == j)continue;
@@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
cpufreq_create_debugfs();Why you moved this to register_driver? It was fine at cpufreq_core_init()
If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit(). Do you agree about creating cpufreq_core_exit()(?
No you don't need that routine. Or in other words there isn't any exit for cpufreq core and so this directory must not be removed.
On 07/24/2013 02:05 PM, Viresh Kumar wrote:
On 24 July 2013 06:55, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu)+{
unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;if (!policy->cpu_debugfs[idx])return;debugfs_remove_recursive(policy->cpu_debugfs[idx]);Whey do we need recursive here? And what exactly does recursive will do?
If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
You are calling this routine even when we aren't at the last cpu of a policy. And so, eventually you are calling this routine for a link you have created.
I'll call proper debugfs_remove*() function according to type of debugfs pointer. - if cpu is last user of policy, call debugfs_remove_recursive() - else, call debugfs_remove().
Have you actually tested your code? What kind of platform? What is cpu topology ?? And what exactly you tested..
I tested quad-core EXYNOS4412 SoC based on Cortex-A9 with Tizen platform. It is opereated on this environment but as you commnet, this test and environment isn't enough to verify this patchset. - Testcase1 : Change cpufreq governor on runtime - Testcase2 : Turn on/off CPU state on runtme
We are already on v6 and this patch still looks like the v1.. It still has lots of basic mistakes, which I don't expect so later in the series..
Its very difficult for me to review the same patchset again and again.. So, normally people might not review it well after v3-v4 and just trust the sender.. But I am nowhere close to getting that.. And so discouraged to review it..
I'm so sorry about this and thanks for previous your review sincerely.
Please review/test it well on multiple kind of systems if possible. Test on your intel laptop and see if it has multiple policy structures with multiple cpus in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy structure.
As you comment, I'll modify/test this patchset on various system with enough testcase and resend this patchset after a thorough review.
+}
same problem here too.
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);Why you need this? We aren't removing the earlier dentry at all here.
haven't answered this.
The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table) If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu, core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and child data for 'old_cpu' to debugfs directory for 'new_cpu'.
If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file. So I didn't remove the earlier dentry of 'old_cpu'.
if (!new_entry) {pr_err("changing debugfs directory name failed\n");goto err_rename;}policy->cpu_debugfs[new_cpu] = new_entry;policy->cpu_debugfs[old_cpu] = NULL;/* Create again symbolic link of debugfs directory */for_each_present_cpu(j) {present_cpu?? We discussed this before.. You will break multi cluster systems.
My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
Go through earlier comments about this.. you are still wrong.. You need to run over cpus that are in this policy.. i.e. policy->cpus.
OK.
if (new_cpu == j)continue;@@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
cpufreq_create_debugfs();Why you moved this to register_driver? It was fine at cpufreq_core_init()
If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit(). Do you agree about creating cpufreq_core_exit()(?
No you don't need that routine. Or in other words there isn't any exit for cpufreq core and so this directory must not be removed.
I understood on your previous comment as You said that I had to remove 'cpufreq' debugfs directory when cpufreq isn't used.
If the core execute cpufreq_create_debugfs() in cpufreq_core_init(), don't I need to remove 'cpufreq' debugfs directory without cpufreq_core_exit()?
Thanks, Chanwoo Choi
On 24 July 2013 13:13, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/24/2013 02:05 PM, Viresh Kumar wrote:
On 24 July 2013 06:55, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu)+{
unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;if (!policy->cpu_debugfs[idx])return;debugfs_remove_recursive(policy->cpu_debugfs[idx]);Whey do we need recursive here? And what exactly does recursive will do?
If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
You are calling this routine even when we aren't at the last cpu of a policy. And so, eventually you are calling this routine for a link you have created.
I'll call proper debugfs_remove*() function according to type of debugfs pointer.
- if cpu is last user of policy, call debugfs_remove_recursive()
- else, call debugfs_remove().
Have you actually tested your code? What kind of platform? What is cpu topology ?? And what exactly you tested..
I tested quad-core EXYNOS4412 SoC based on Cortex-A9 with Tizen platform. It is opereated on this environment but as you commnet, this test and environment isn't enough to verify this patchset.
- Testcase1 : Change cpufreq governor on runtime
- Testcase2 : Turn on/off CPU state on runtme
We are already on v6 and this patch still looks like the v1.. It still has lots of basic mistakes, which I don't expect so later in the series..
Its very difficult for me to review the same patchset again and again.. So, normally people might not review it well after v3-v4 and just trust the sender.. But I am nowhere close to getting that.. And so discouraged to review it..
I'm so sorry about this and thanks for previous your review sincerely.
Please review/test it well on multiple kind of systems if possible. Test on your intel laptop and see if it has multiple policy structures with multiple cpus in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy structure.
As you comment, I'll modify/test this patchset on various system with enough testcase and resend this patchset after a thorough review.
+}
same problem here too.
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);Why you need this? We aren't removing the earlier dentry at all here.
haven't answered this.
The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table) If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu, core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and child data for 'old_cpu' to debugfs directory for 'new_cpu'.
If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file. So I didn't remove the earlier dentry of 'old_cpu'.
if (!new_entry) {pr_err("changing debugfs directory name failed\n");goto err_rename;}policy->cpu_debugfs[new_cpu] = new_entry;policy->cpu_debugfs[old_cpu] = NULL;/* Create again symbolic link of debugfs directory */for_each_present_cpu(j) {present_cpu?? We discussed this before.. You will break multi cluster systems.
My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
Go through earlier comments about this.. you are still wrong.. You need to run over cpus that are in this policy.. i.e. policy->cpus.
OK.
if (new_cpu == j)continue;@@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
cpufreq_create_debugfs();Why you moved this to register_driver? It was fine at cpufreq_core_init()
If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit(). Do you agree about creating cpufreq_core_exit()(?
No you don't need that routine. Or in other words there isn't any exit for cpufreq core and so this directory must not be removed.
I understood on your previous comment as You said that I had to remove 'cpufreq' debugfs directory when cpufreq isn't used.
If the core execute cpufreq_create_debugfs() in cpufreq_core_init(), don't I need to remove 'cpufreq' debugfs directory without cpufreq_core_exit()?
I copied following from your patch sent on 5th july.. It didn't had any version number and so is difficult to distinguish..
@@ -1976,6 +2029,10 @@ static int __init cpufreq_core_init(void) BUG_ON(!cpufreq_global_kobject); register_syscore_ops(&cpufreq_syscore_ops);
cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);if (!cpufreq_debugfs)pr_debug("creating debugfs root failed\n");
So, you just added this directory once.. So you must not remove it.
Where did I say you remove this directory.. To be clear, don't remove cpufreq debugfs directory at all. Play only with cpu directories inside this debugfs directory.
On 07/24/2013 04:51 PM, Viresh Kumar wrote:
On 24 July 2013 13:13, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/24/2013 02:05 PM, Viresh Kumar wrote:
On 24 July 2013 06:55, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
unsigned int cpu)+{
unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;if (!policy->cpu_debugfs[idx])return;debugfs_remove_recursive(policy->cpu_debugfs[idx]);Whey do we need recursive here? And what exactly does recursive will do?
If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
You are calling this routine even when we aren't at the last cpu of a policy. And so, eventually you are calling this routine for a link you have created.
I'll call proper debugfs_remove*() function according to type of debugfs pointer.
- if cpu is last user of policy, call debugfs_remove_recursive()
- else, call debugfs_remove().
Have you actually tested your code? What kind of platform? What is cpu topology ?? And what exactly you tested..
I tested quad-core EXYNOS4412 SoC based on Cortex-A9 with Tizen platform. It is opereated on this environment but as you commnet, this test and environment isn't enough to verify this patchset.
- Testcase1 : Change cpufreq governor on runtime
- Testcase2 : Turn on/off CPU state on runtme
We are already on v6 and this patch still looks like the v1.. It still has lots of basic mistakes, which I don't expect so later in the series..
Its very difficult for me to review the same patchset again and again.. So, normally people might not review it well after v3-v4 and just trust the sender.. But I am nowhere close to getting that.. And so discouraged to review it..
I'm so sorry about this and thanks for previous your review sincerely.
Please review/test it well on multiple kind of systems if possible. Test on your intel laptop and see if it has multiple policy structures with multiple cpus in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy structure.
As you comment, I'll modify/test this patchset on various system with enough testcase and resend this patchset after a thorough review.
+}
same problem here too.
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);Why you need this? We aren't removing the earlier dentry at all here.
haven't answered this.
The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table) If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu, core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and child data for 'old_cpu' to debugfs directory for 'new_cpu'.
If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file. So I didn't remove the earlier dentry of 'old_cpu'.
if (!new_entry) {pr_err("changing debugfs directory name failed\n");goto err_rename;}policy->cpu_debugfs[new_cpu] = new_entry;policy->cpu_debugfs[old_cpu] = NULL;/* Create again symbolic link of debugfs directory */for_each_present_cpu(j) {present_cpu?? We discussed this before.. You will break multi cluster systems.
My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
Go through earlier comments about this.. you are still wrong.. You need to run over cpus that are in this policy.. i.e. policy->cpus.
OK.
if (new_cpu == j)continue;@@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
cpufreq_create_debugfs();Why you moved this to register_driver? It was fine at cpufreq_core_init()
If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit(). Do you agree about creating cpufreq_core_exit()(?
No you don't need that routine. Or in other words there isn't any exit for cpufreq core and so this directory must not be removed.
I understood on your previous comment as You said that I had to remove 'cpufreq' debugfs directory when cpufreq isn't used.
If the core execute cpufreq_create_debugfs() in cpufreq_core_init(), don't I need to remove 'cpufreq' debugfs directory without cpufreq_core_exit()?
I copied following from your patch sent on 5th july.. It didn't had any version number and so is difficult to distinguish..
@@ -1976,6 +2029,10 @@ static int __init cpufreq_core_init(void) BUG_ON(!cpufreq_global_kobject); register_syscore_ops(&cpufreq_syscore_ops);
cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);if (!cpufreq_debugfs)pr_debug("creating debugfs root failed\n");So, you just added this directory once.. So you must not remove it.
Where did I say you remove this directory.. To be clear, don't remove cpufreq debugfs directory at all. Play only with cpu directories inside this debugfs directory.
You're right. I'm sorry and I misunderstood.
Thanks, Chanwoo Choi
I just realized I missed answering few questions:
On 24 July 2013 13:13, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/24/2013 02:05 PM, Viresh Kumar wrote:
On 24 July 2013 06:55, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);Why you need this? We aren't removing the earlier dentry at all here.
haven't answered this.
The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table) If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu, core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and child data for 'old_cpu' to debugfs directory for 'new_cpu'.
If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file. So I didn't remove the earlier dentry of 'old_cpu'.
Okay.. The original question was: why do you need to remove & add entries or links for cpus other than policy->cpu? Because we are renaming the entry, wouldn't that work straight away?
On 07/24/2013 05:07 PM, Viresh Kumar wrote:
I just realized I missed answering few questions:
On 24 July 2013 13:13, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/24/2013 02:05 PM, Viresh Kumar wrote:
On 24 July 2013 06:55, Chanwoo Choi cw00.choi@samsung.com wrote:
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
On 18 July 2013 16:47, Chanwoo Choi cw00.choi@samsung.com wrote:
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
unsigned int new_cpu)+{
struct dentry *old_entry, *new_entry;char new_dir_name[CPUFREQ_NAME_LEN];unsigned int j, old_cpu = policy->cpu;if (!policy->cpu_debugfs[new_cpu])return;/** Remove symbolic link of debugfs directory except for debugfs* directory of old_cpu.*/for_each_present_cpu(j) {if (old_cpu == j)continue;debugfs_remove(policy->cpu_debugfs[j]);Why you need this? We aren't removing the earlier dentry at all here.
haven't answered this.
The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table) If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu, core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and child data for 'old_cpu' to debugfs directory for 'new_cpu'.
If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file. So I didn't remove the earlier dentry of 'old_cpu'.
Okay.. The original question was: why do you need to remove & add entries or links for cpus other than policy->cpu? Because we are renaming the entry, wouldn't that work straight away?
In case that all CPUs share same cpufreq policy. Each debugfs dentry of CPU[1-3] except for CPU0 has symbolic link to CPU0's debugfs directory as following.
-sh-4.1# ls -al /sys/kernel/debug/cpufreq/ total 0 drwxr-xr-x 3 root root 0 Jan 1 09:00 . drwx------ 28 root root 0 Jan 1 09:00 .. drwxr-xr-x 2 root root 0 Jan 1 09:00 cpu0 (policy->cpu is 0) lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu1 -> ./cpu0 lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu2 -> ./cpu0 lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu3 -> ./cpu0
If turn off CPU0 state, I have to move debugfs directory data from cpu0 to cpu1 and again create link to cpu1's debugfs directory for CPU[2-3] debugfs directory. So, I removed dentry link of CPU[1-3] before creating link again.
cpu1 cpu2 -> ./cpu1 cpu3 -> ./cpu1
But I can rewrite new link of CPU[2-3] to previous dentry link(policy->cpu_debugfs[2] or policy->cpu_debugfs[3]) for reducing unnecessary code without revmoval sequence.
On 24 July 2013 14:16, Chanwoo Choi cw00.choi@samsung.com wrote:
In case that all CPUs share same cpufreq policy. Each debugfs dentry of CPU[1-3] except for CPU0 has symbolic link to CPU0's debugfs directory as following.
-sh-4.1# ls -al /sys/kernel/debug/cpufreq/ total 0 drwxr-xr-x 3 root root 0 Jan 1 09:00 . drwx------ 28 root root 0 Jan 1 09:00 .. drwxr-xr-x 2 root root 0 Jan 1 09:00 cpu0 (policy->cpu is 0) lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu1 -> ./cpu0 lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu2 -> ./cpu0 lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu3 -> ./cpu0
If turn off CPU0 state, I have to move debugfs directory data from cpu0 to cpu1 and again create link to cpu1's debugfs directory for CPU[2-3] debugfs directory. So, I removed dentry link of CPU[1-3] before creating link again.
cpu1 cpu2 -> ./cpu1 cpu3 -> ./cpu1
But I can rewrite new link of CPU[2-3] to previous dentry link(policy->cpu_debugfs[2] or policy->cpu_debugfs[3]) for reducing unnecessary code without revmoval sequence.
Because we aren't freeing the debugfs node at all (just renaming it), the links might still be good after renaming.. But please check if it is true.
So, according to me you need to do this: - Remove symlink for new policy->cpu, i..e cpu1 in your example - rename debugfs entry to give it to cpu1 instead of cpu0. - Set cpu0 pointer to NULL.
Probably that's it.
On 07/24/2013 05:51 PM, Viresh Kumar wrote:
On 24 July 2013 14:16, Chanwoo Choi cw00.choi@samsung.com wrote:
In case that all CPUs share same cpufreq policy. Each debugfs dentry of CPU[1-3] except for CPU0 has symbolic link to CPU0's debugfs directory as following.
-sh-4.1# ls -al /sys/kernel/debug/cpufreq/ total 0 drwxr-xr-x 3 root root 0 Jan 1 09:00 . drwx------ 28 root root 0 Jan 1 09:00 .. drwxr-xr-x 2 root root 0 Jan 1 09:00 cpu0 (policy->cpu is 0) lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu1 -> ./cpu0 lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu2 -> ./cpu0 lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu3 -> ./cpu0
If turn off CPU0 state, I have to move debugfs directory data from cpu0 to cpu1 and again create link to cpu1's debugfs directory for CPU[2-3] debugfs directory. So, I removed dentry link of CPU[1-3] before creating link again.
cpu1 cpu2 -> ./cpu1 cpu3 -> ./cpu1
But I can rewrite new link of CPU[2-3] to previous dentry link(policy->cpu_debugfs[2] or policy->cpu_debugfs[3]) for reducing unnecessary code without revmoval sequence.
Because we aren't freeing the debugfs node at all (just renaming it), the links might still be good after renaming.. But please check if it is true.
So, according to me you need to do this:
- Remove symlink for new policy->cpu, i..e cpu1 in your example
- rename debugfs entry to give it to cpu1 instead of cpu0.
- Set cpu0 pointer to NULL.
Probably that's it.
And, I add additional step on below:
- Remove symlink for new policy->cpu, i..e cpu1 in your example
- rename debugfs entry to give it to cpu1 instead of cpu0.
- Store renamed cpu0 pointer to cpu1 pointer - Create new link for CPU[2-3] to CPU1's debugfs directory because debugfs use string path to create symbolic link. It isn't automatically connected with new CPU1 debugfs directory.
- Set cpu0 pointer to NULL.
Thanks, Chanwoo Choi
On 24 July 2013 14:35, Chanwoo Choi cw00.choi@samsung.com wrote:
And, I add additional step on below:
- Remove symlink for new policy->cpu, i..e cpu1 in your example
- rename debugfs entry to give it to cpu1 instead of cpu0.
- Store renamed cpu0 pointer to cpu1 pointer
- Create new link for CPU[2-3] to CPU1's debugfs directory because debugfs use string path to create symbolic link. It isn't automatically connected with new CPU1 debugfs directory.
Honestly speaking I am not the best at debugfs core, but I still think the link is connected to a struct debugfs node and not to any path.. This node is connected to a path though. And so even after rename things should stay stable without your new step...
Just try and see if I am right or wrong.. Otherwise what you already did is correct as you need to remove links for 2-3 as well..
- Set cpu0 pointer to NULL.
On 07/24/2013 06:09 PM, Viresh Kumar wrote:
On 24 July 2013 14:35, Chanwoo Choi cw00.choi@samsung.com wrote:
And, I add additional step on below:
- Remove symlink for new policy->cpu, i..e cpu1 in your example
- rename debugfs entry to give it to cpu1 instead of cpu0.
- Store renamed cpu0 pointer to cpu1 pointer
- Create new link for CPU[2-3] to CPU1's debugfs directory because debugfs use string path to create symbolic link. It isn't automatically connected with new CPU1 debugfs directory.
Honestly speaking I am not the best at debugfs core, but I still think the link is connected to a struct debugfs node and not to any path.. This node is connected to a path though. And so even after rename things should stay stable without your new step...
Just try and see if I am right or wrong.. Otherwise what you already did is correct as you need to remove links for 2-3 as well..
OK, I'll try it according to your suggestion. Thanks.
Best Regards, Chanwoo Choi
Hi Viresh,
if (!policy->cpu_debugfs[idx]) {pr_err("creating debugfs directory failed\n");return -ENODEV;}return 0;+}
+static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
unsigned int src_cpu,unsigned int dest_cpu)Only use policy and cpu for which symlink has to be created as param to this routine. And create link to policy->cpu.
OK, I'll simplify function prototype(cpufreq_create_debugfs_symlink) by removing unnecessary parameter.
I considered the parameter type of cpufreq_create_debugfs_symlink() and then I need following function declaration because this function didn't always need to create symbolic link to policy->cpu. This function declaration is capable of creating symbolic link as 'dest_cpu -> src_cpu'
+static int cpufreq_create_debugfs_symlink(unsigned int src_cpu, + unsigned int dest_cpu)
Thanks, Chanwoo Choi
On Wed, Jul 24, 2013 at 11:44 AM, Chanwoo Choi cw00.choi@samsung.com wrote:
I considered the parameter type of cpufreq_create_debugfs_symlink() and then I need following function declaration because this function didn't always need to create symbolic link to policy->cpu. This function declaration is capable of creating symbolic link as 'dest_cpu -> src_cpu'
+static int cpufreq_create_debugfs_symlink(unsigned int src_cpu, + unsigned int dest_cpu)
You need policy structure as well, right? And src_cpu is always going to be policy->cpu. So you can have policy and cpu as two parameters.. cpu is dest cpu btw.
linaro-kernel@lists.linaro.org
-
Chanwoo Choi -
Viresh Kumar