On 25 July 2014 06:37, Saravana Kannan <skannan@xxxxxxxxxxxxxx> wrote:
> When CPUs are physically added/removed, its cpuX sysfs directory is
> dynamically added/removed. To handle this correctly, the cpufreq sysfs
> nodes also need to be added/removed dynamically.
Hmm, in that case why should we take this thread? I mean, if we do need
to add/remove sysfs links or move kobjects around, what would we achieve
with this patchset?
> Signed-off-by: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
> ---
> drivers/cpufreq/cpufreq.c | 46 +++++++++++++++++++++++++++++++++-------------
> 1 file changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index d9fc6e5..97edf05 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -41,6 +41,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);
> static DEFINE_RWLOCK(cpufreq_driver_lock);
> DEFINE_MUTEX(cpufreq_governor_lock);
> static LIST_HEAD(cpufreq_policy_list);
> +static cpumask_t has_symlink;
>
> /* This one keeps track of the previously set governor of a removed CPU */
> static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
> @@ -865,7 +866,10 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
> unsigned int j;
> int ret = 0;
>
> - for_each_cpu(j, policy->related_cpus) {
> + /* Only some of the related CPUs might be present. So, create
> + * symlinks only for those.
> + */
Proper styles please.
> + for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
> struct device *cpu_dev;
>
> if (j == policy->kobj_cpu)
> @@ -877,6 +881,7 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
> "cpufreq");
> if (ret)
> break;
> + cpumask_set_cpu(j, &has_symlink);
> }
> return ret;
> }
> @@ -1101,9 +1106,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> unsigned long flags;
> bool recover_policy = cpufreq_suspended;
>
> - if (cpu_is_offline(cpu))
> - return 0;
> -
Why?
> pr_debug("adding CPU %u\n", cpu);
>
> #ifdef CONFIG_SMP
> @@ -1111,7 +1113,19 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> * CPU because it is in the same boat. */
> policy = cpufreq_cpu_get(cpu);
> if (policy) {
> - if (!cpumask_test_cpu(cpu, policy->cpus))
> + /* If a CPU gets physically plugged in after one or more of
> + * its related CPUs are ONLINE, we need to create a symlink
> + * for it since it wouldn't have been created when the policy
> + * was initialized. Do this as soon as it's plugged in.
> + */
> + if (sif && !cpumask_test_cpu(cpu, &has_symlink)) {
Why check for sif?
> + ret = sysfs_create_link(&dev->kobj, &policy->kobj,
> + "cpufreq");
> + if (!ret)
> + cpumask_set_cpu(cpu, &has_symlink);
> + }
> +
Move all this to cpufreq_add_policy_cpu()..
> + if (!cpumask_test_cpu(cpu, policy->cpus) && cpu_online(cpu))
> ret = cpufreq_add_policy_cpu(policy, cpu, dev);
> else
> ret = 0;
> @@ -1120,6 +1134,9 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> }
> #endif
>
> + if (cpu_is_offline(cpu))
> + return 0;
> +
Don't know why we moved it here.. cpufreq_add_dev will only be called for
online CPUs..
> if (!down_read_trylock(&cpufreq_rwsem))
> return 0;
>
> @@ -1303,25 +1320,24 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
> unsigned int old_cpu)
> {
> struct device *cpu_dev;
> + unsigned int new_cpu;
> int ret;
>
> /* first sibling now owns the new sysfs dir */
> - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu));
> + for_each_cpu_and(new_cpu, policy->related_cpus, cpu_present_mask)
> + if (new_cpu != old_cpu)
> + break;
> + cpu_dev = get_cpu_device(new_cpu);
>
> sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
> if (ret) {
> pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
> -
> - down_write(&policy->rwsem);
> - cpumask_set_cpu(old_cpu, policy->cpus);
> - up_write(&policy->rwsem);
> -
> ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
> "cpufreq");
> -
> return -EINVAL;
> }
> + cpumask_clear_cpu(new_cpu, &has_symlink);
> policy->kobj_cpu = cpu_dev->id;
>
> return cpu_dev->id;
> @@ -1407,8 +1423,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
> cpus = cpumask_weight(policy->cpus);
> up_read(&policy->rwsem);
>
> - if (cpu != policy->kobj_cpu)
> + if (cpu != policy->kobj_cpu) {
> sysfs_remove_link(&dev->kobj, "cpufreq");
> + cpumask_clear_cpu(cpu, &has_symlink);
> + } else {
> + cpufreq_nominate_new_policy_cpu(policy, cpu);
> + }
This has_symlink thing has made it much more complicated..
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
