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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@xxxxxxxxxxx> 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
>
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Devel]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Forum]     [Linux SCSI]

  Powered by Linux