Quite frankly, to me this patch just does too much in one go without
describing about a half of things it is doing.
I would just add the kobject_move() call to __cpufreq_add_dev() to
start with (because that's what you really want if I'm not mistaken)
and do the cleanups separately, later.
On Thu, Jul 17, 2014 at 2:21 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> On 17 July 2014 04:48, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>> +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
>>> + struct device *cpu_dev)
>>> {
>>> + int ret;
>>> +
>>> if (WARN_ON(cpu == policy->cpu))
>>> - return;
>>> + return 0;
>>> +
>>> + /* Move kobject to the new policy->cpu */
>>> + ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
>>> + if (ret) {
>>> + pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
>>> + return ret;
>>
>> Previously, we returned -EINVAL in the kobject_move() failure case. Why are
>> we changing that now?
>
> We should have preserved return value of kobject_move() earlier in
> cpufreq_nominate_new_policy_cpu() and sent that, but we returned
> -EINVAL. And I realized that its more appropriate to return the error
> returned by kobject_move().
>
>>> static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>>> @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>>> * by invoking update_policy_cpu().
>>> */
>>> if (recover_policy && cpu != policy->cpu)
>>> - update_policy_cpu(policy, cpu);
>>> + WARN_ON(update_policy_cpu(policy, cpu, dev));
>>
>> This is an arbitrary difference in the handling of update_policy_cpu() return
>> value. Why do we want the WARN_ON() here and not in the other place?
>
> We really can't recover in this case. We have reached here after a suspend/
> resume, and probing first cpu of a non-boot cluster. And we *have* to make
> it policy-master.
>
> But in the other case, we are removing a CPU in PREPARE stage and so
> we can actually fail from there and let everybody know. Though I am not
> aware of anycase in which kobject_move() can fail there.
>
>> Don't we want to recover from kobject_move() failures here as well?
>
> In the other case, we have just removed the link from the new policy->cpu
> and so we try to recover for that in failures, but don't have something
> similar here.
>
>>> else
>>> policy->cpu = cpu;
>>>
>>> @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>>> return __cpufreq_add_dev(dev, sif);
>>> }
>>>
>>> -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
>>> - unsigned int old_cpu)
>>> -{
>>> - struct device *cpu_dev;
>>> - int ret;
>>> -
>>> - /* first sibling now owns the new sysfs dir */
>>> - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_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);
>>
>> Why don't we need the above three lines in the new code?
>
> It was probably meaningful when this was added initially, but later
> some commit moved the cpumask_clear_cpu() to
> __cpufreq_remove_dev_finish(). And so we don't really need to
> set the cpu to policy->cpus again, as it was never cleared by this
> stage..
>
>>> -
>>> - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
>>> - "cpufreq");
>>> -
>>> - return -EINVAL;
>>> - }
>>> -
>>> - return cpu_dev->id;
>>> -}
>>> -
>>> static int __cpufreq_remove_dev_prepare(struct device *dev,
>>> struct subsys_interface *sif)
>>> {
>>> unsigned int cpu = dev->id, cpus;
>>> - int new_cpu, ret;
>>> + int ret;
>>> unsigned long flags;
>>> struct cpufreq_policy *policy;
>>>
>>> @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>>> if (cpu != policy->cpu) {
>>> sysfs_remove_link(&dev->kobj, "cpufreq");
>>> } else if (cpus > 1) {
>>> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu);
>>> - if (new_cpu >= 0) {
>>> - update_policy_cpu(policy, new_cpu);
>>> + /* Nominate new CPU */
>>> + int new_cpu = cpumask_any_but(policy->cpus, cpu);
>>> + struct device *cpu_dev = get_cpu_device(new_cpu);
>>>
>>> - if (!cpufreq_suspended)
>>> - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
>>> - __func__, new_cpu, cpu);
>>> + sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
>>> + ret = update_policy_cpu(policy, new_cpu, cpu_dev);
>>> + if (ret) {
>>> + /*
>>> + * To supress compilation warning about return value of
>>> + * sysfs_create_link().
>>> + */
>>> + int temp;
>>> +
>>> + /* Create link again if we failed. */
>>> + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
>>> + "cpufreq");
>>
>> And this is *ugly*.
>
> Absolutely, Let me know what can we do to work around this.
> It was like this earlier as well, just that I added a descriptive
> comment this time.
>
> --
> viresh
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html