Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq

[Chanwoo Choi <cw00.choi@samsung.com>]

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