Re: [PATCH 1/2] cpufreq: Return error if ->get() failed in cpufreq_update_policy()

[Srivatsa S. Bhat]

On 02/17/2014 02:09 PM, Viresh Kumar wrote:
> On 17 February 2014 13:49, Srivatsa S. Bhat
> <srivatsa.bhat@xxxxxxxxxxxxxxxxxx> wrote:
>> Quick question: Looking at cpufreq_update_policy() and cpufreq_out_of_sync(),
>> I understand that if the cpufreq subsystem's notion of the current frequency
>> does not match with the actual frequency of the CPU, it tries to adjust and
>> notify everyone that the current frequency is so-and-so, as read from the
>> hardware. Instead, why can't we simply set the frequency to the value that
>> we _want_ it to be at? I mean, if cpufreq subsystem thinks it is X KHz and
>> the actual frequency is Y KHz, we can as well fix the anomaly by setting the
>> frequency immediately to X KHz right?
>>
>> The reason I ask this is that, if we follow this approach, then we can avoid
>> ambiguities in dealing with the out-of-sync situation. That is, it becomes
>> very straightforward to decide _what_ to do, when we detect scenarios where
>> the frequency goes out of sync.
> 
> Hmm, it is just about doing all that stuff in a single line, like:
> __cpufreq_driver_target(...) ??
> 
> There are few problems here:
> - If we simply call above routine with X, then it will simply return as
> X == policy->cur. And I don't want to hack this code in a bad way now :)
> 
> - So, probably the way it is implemented is right, as we do that the most
> efficient way. We just broadcast the new freq in case there is a difference
> otherwise nothing.

Specifically, I'm referring to the problem where there _is_ a difference,
but the ->get() is not reporting it properly, like returning a 0 for example.
In such a case, instead of erroring out (and thereby perhaps opening the doors
to more problems down the line), won't it be better to simply set the CPU's
frequency to what we want it to be?

That is, I'm concerned about this part of your patch:

 	if (cpufreq_driver->get) {
 		new_policy.cur = cpufreq_driver->get(cpu);
+
+		if (!new_policy.cur) {
+			pr_err("%s: ->get() returned 0 KHz\n", __func__);
+			ret = -EINVAL;
+			goto no_policy;
+		}
+

Why go to no_policy when we can actually set things right?

Anyway, I am not arguing against this strongly. I just wanted to share my
thoughts, since this is the approach that made more sense to me.

Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/