On Thursday, July 23, 2015 12:09:42 PM Viresh Kumar wrote: > On 23-07-15, 02:04, Rafael J. Wysocki wrote: > > +static int cpufreq_add_dev(struct device *dev, struct subsys_interface > > *sif) > > +{ > > + unsigned int cpu = dev->id; > > + struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); > > + > > + pr_debug("%s: adding CPU %u\n", __func__, cpu); > > + > > + if (policy && policy->kobj_cpu != cpu) { > > Why are you comparing cpu against kobj_cpu ? I don't think it can ever > be false.
It can. When we're adding a CPU that has a policy already, because it is "related" to a previously registered CPU. > > + int ret; > > + > > + pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu); > > dev_dbg OK > > + ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"); > > + if (ret) { > > + dev_dbg(dev, "%s: Failed to create link (%d)\n", > > dev_err Well, I'm wondering about this. Why does this have to be dev_err()? > > + __func__, ret); > > + return ret; > > + } > > + > > + /* Track CPUs for which sysfs links are created */ > > + cpumask_set_cpu(cpu, policy->linked_cpus); > > + } > > + > > + return cpu_online(cpu) ? cpufreq_dev_online(dev, false) : 0; > > +} > > Looks fine otherwise. Thanks for getting your hands dirty :) > > > static void cpufreq_offline_prepare(unsigned int cpu) > > { > > struct cpufreq_policy *policy; > > @@ -2344,31 +2343,35 @@ unlock: > > } > > EXPORT_SYMBOL(cpufreq_update_policy); > > > > +static void cpufreq_cpu_online(unsigned int cpu) > > +{ > > + struct device *dev = get_cpu_device(cpu); > > + > > + if (dev) > > + cpufreq_dev_online(dev, true); > > +} > > What about dropping this wrapper function and ... > > > static int cpufreq_cpu_callback(struct notifier_block *nfb, > > unsigned long action, void *hcpu) > > { > > unsigned int cpu = (unsigned long)hcpu; > > - struct device *dev; > > > > - dev = get_cpu_device(cpu); > > ... keeping this as is? And then we can do > s/cpufreq_dev_online/cpufreq_cpu_online which suits better. Well, we don't need the dev things for DOWN_PREPARE and POST_DEAD. We actually only need it in a few places in cpufreq_dev_online(), or maybe simply cpufreq_online(), so it can take the cpu argument too. > > - if (dev) { > > - switch (action & ~CPU_TASKS_FROZEN) { > > - case CPU_ONLINE: > > - cpufreq_add_dev(dev, NULL); > > - break; > > - > > - case CPU_DOWN_PREPARE: > > - cpufreq_offline_prepare(cpu); > > - break; > > - > > - case CPU_POST_DEAD: > > - cpufreq_offline_finish(cpu); > > - break; > > - > > - case CPU_DOWN_FAILED: > > - cpufreq_add_dev(dev, NULL); > > - break; > > - } > > + switch (action & ~CPU_TASKS_FROZEN) { > > + case CPU_ONLINE: > > + cpufreq_cpu_online(cpu); > > + break; > > + > > + case CPU_DOWN_PREPARE: > > + cpufreq_offline_prepare(cpu); > > + break; > > + > > + case CPU_POST_DEAD: > > + cpufreq_offline_finish(cpu); > > + break; > > + > > + case CPU_DOWN_FAILED: > > + cpufreq_cpu_online(cpu); > > + break; > > } > > return NOTIFY_OK; > > } > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/