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

cpufreq_update_policy() calls cpufreq_driver->get() to get current frequency of
a CPU and it is not supposed to fail or return zero. Return error in case that
happens.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Pierre,

I don't think this will fix the issue you were facing but might supress it :)..
And so you need to understand what causes your ->get() to return zero.

@Rafael: I got to these patches while looking at code recently after Pierre
complained about. Came to this conclusion after having discussions with Srivatsa
over IRC..

 drivers/cpufreq/cpufreq.c | 7 +++++++
 1 file changed, 7 insertions(+)

On 17 February 2014 05:58, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, February 14, 2014 04:30:40 PM Viresh Kumar wrote:

> Good to know that you chat with each other, but it really is not a useful piece
> of information until you say what *exactly* you were talking about.

All that is mentioned in commit logs of both the patches :) .. That's all we
discussed.

>>  drivers/cpufreq/cpufreq.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 08ca8c9..383362b 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -2151,6 +2151,13 @@ int cpufreq_update_policy(unsigned int cpu)
>>        */
>>       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;
>
> That isn't -EINVAL.  It may be -EIO or -ENODEV, but not -EINVAL.  Please.

Hmm.. Correct. I will use EIO then..

>> +                     goto no_policy;
>
> And is it unsafe to continue here?  Or can we continue regardless of getting 0?

We were supposed to set this frequency as the current frequency in policy->cur,
what else can we do now in this function when we aren't able to read current
freq? And so I thought that's all we can do here.

>> +             }
>> +
>>               if (!policy->cur) {
>>                       pr_debug("Driver did not initialize current freq");
>>                       policy->cur = new_policy.cur;
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 17 February 2014 13:49, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 17 February 2014 14:25, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> 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.

I think get() returning zero isn't acceptable at all. How can current freq be
zero :) .. There has to be a bug somewhere and so we shouldn't really
try to get working here.. That's what I thought.
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 18 February 2014 03:30, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, February 17, 2014 02:25:34 PM Srivatsa S. Bhat wrote:
>> 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.
>
> And I agree with that.  In particular, since we're going to set the new
> policy *anyway* at this point, we can adjust the current frequency just fine
> in the process, can't we?

Though I still feel that it wouldn't be the right thing to do as get()
just can't
return zero. Actually I was planning to place a WARN() over its return value
of zero.

Anyway, as two of the three are in favor of this we can get that in.. But how
would that work?

- What frequency should we call cpufreq_driver_target ?
- Remember that it wouldn't do anything if policy->cur is same as new freq.
- Also remember that drivers need special attention if new freq is > old
freq or vice versa. As they will change voltage before or after change here.
And because we actually don't know what frequency we are at currently, how
will we decide that?
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 18 February 2014 07:49, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 18 February 2014 03:30, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Monday, February 17, 2014 02:25:34 PM Srivatsa S. Bhat wrote:
>>> 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.
>>
>> And I agree with that.  In particular, since we're going to set the new
>> policy *anyway* at this point, we can adjust the current frequency just fine
>> in the process, can't we?
>
> Though I still feel that it wouldn't be the right thing to do as get()
> just can't
> return zero. Actually I was planning to place a WARN() over its return value
> of zero.
>
> Anyway, as two of the three are in favor of this we can get that in.. But how
> would that work?
>
> - What frequency should we call cpufreq_driver_target ?
> - Remember that it wouldn't do anything if policy->cur is same as new freq.
> - Also remember that drivers need special attention if new freq is > old
> freq or vice versa. As they will change voltage before or after change here.
> And because we actually don't know what frequency we are at currently, how
> will we decide that?

@Rafael/Srivatsa: Ping!!
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 25 February 2014 11:23, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Hmm, that's a good point. However, lets first think about the simple scenario
> that the driver _is_ able to detect the current frequency from the hardware
> (a non-zero, sane value) say X KHz, and that frequency is different from what
> the cpufreq subsystem thinks it is (Y KHz).
>
> In the current code, when we observe this, we send out a notification and try
> to adjust to X KHz. Instead, what I'm suggesting is to invoke the driver to
> set the frequency to Y KHz, since that's what the cpufreq subsystems wants the
> frequency to be at.

Actually we don't know at this point what cpufreq wants :)
Governor will decide what frequency to run CPU at and lets leave it to
that point.
As the transition that we might end up doing here would be simply overridden
very soon. And to be honest this decision must be taken by governor and not
core. We just want to make sure core is in sync with hardware.

> As for the case where the driver reports the frequency to be 0 KHz, we can
> print a WARN() and try to force set the frequency to Y KHz. But if that turns
> out to be too tricky to handle, we can just put a WARN() and error out, as you
> proposed earlier.

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

On 25 February 2014 18:40, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, why exactly does the core need to operate "current frequency" at all?

I don't think I understood what you meant here. Do you mean why should
we maintain policy->cur?
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 26 February 2014 03:59, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Yes, what exactly do we need it for in the core?

Its probably there to make things faster. We cache the value so that we
don't go to the hardware to read/calculate that again. Isn't it?

And we need to know current freq on many occasions. One of that is that
many drivers need to know the relation between current and new freq before
they can make the change. As they might need to play with volt regulators
before or after the freq change. Also it is used mainly in our loops_per_jiffiy
calculations.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 26 February 2014 13:15, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26 February 2014 03:59, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> Yes, what exactly do we need it for in the core?
>
> Its probably there to make things faster. We cache the value so that we
> don't go to the hardware to read/calculate that again. Isn't it?
>
> And we need to know current freq on many occasions. One of that is that
> many drivers need to know the relation between current and new freq before
> they can make the change. As they might need to play with volt regulators
> before or after the freq change. Also it is used mainly in our loops_per_jiffiy
> calculations.

Ping!!
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html