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

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

[Viresh Kumar]

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(+)

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;
+			goto no_policy;
+		}
+
 		if (!policy->cur) {
 			pr_debug("Driver did not initialize current freq");
 			policy->cur = new_policy.cur;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/2] cpufreq: don't call cpufreq_update_policy() on CPU addition

[Viresh Kumar]

cpufreq_update_policy() is called from two places currently. From a workqueue
handled queued from cpufreq_bp_resume() for boot CPU and from
cpufreq_cpu_callback() whenever a CPU is added.

The first one makes sure that boot CPU is running on the frequency present in
policy->cpu. But we don't really need a call from cpufreq_cpu_callback(),
because we always call cpufreq_driver->init() (which will set policy->cur
correctly) whenever first CPU of any policy is added back. And so every policy
structure is guaranteed to have the right frequency in policy->cur.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 383362b..b6eb4ed 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2194,7 +2194,6 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,
 		switch (action & ~CPU_TASKS_FROZEN) {
 		case CPU_ONLINE:
 			__cpufreq_add_dev(dev, NULL, frozen);
-			cpufreq_update_policy(cpu);
 			break;
 
 		case CPU_DOWN_PREPARE:
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 2/2] cpufreq: don't call cpufreq_update_policy() on CPU addition

[Rafael J. Wysocki]

On Friday, February 14, 2014 04:30:41 PM Viresh Kumar wrote:
> cpufreq_update_policy() is called from two places currently. From a workqueue
> handled queued from cpufreq_bp_resume() for boot CPU and from
> cpufreq_cpu_callback() whenever a CPU is added.
> 
> The first one makes sure that boot CPU is running on the frequency present in
> policy->cpu. But we don't really need a call from cpufreq_cpu_callback(),
> because we always call cpufreq_driver->init() (which will set policy->cur
> correctly) whenever first CPU of any policy is added back. And so every policy
> structure is guaranteed to have the right frequency in policy->cur.

That sounds good, but doing the extra cpufreq_update_policy() shouldn't actually
hurt, should it?

So, that would be a cleanup rather than a fix, right?

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 383362b..b6eb4ed 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2194,7 +2194,6 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,
>  		switch (action & ~CPU_TASKS_FROZEN) {
>  		case CPU_ONLINE:
>  			__cpufreq_add_dev(dev, NULL, frozen);
> -			cpufreq_update_policy(cpu);
>  			break;
>  
>  		case CPU_DOWN_PREPARE:
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpufreq: don't call cpufreq_update_policy() on CPU addition

[Viresh Kumar]

On 17 February 2014 05:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, February 14, 2014 04:30:41 PM Viresh Kumar wrote:
>> cpufreq_update_policy() is called from two places currently. From a workqueue
>> handled queued from cpufreq_bp_resume() for boot CPU and from
>> cpufreq_cpu_callback() whenever a CPU is added.
>>
>> The first one makes sure that boot CPU is running on the frequency present in
>> policy->cpu. But we don't really need a call from cpufreq_cpu_callback(),
>> because we always call cpufreq_driver->init() (which will set policy->cur
>> correctly) whenever first CPU of any policy is added back. And so every policy
>> structure is guaranteed to have the right frequency in policy->cur.
>
> That sounds good, but doing the extra cpufreq_update_policy() shouldn't actually
> hurt, should it?

Yeah, it shouldn't hurt badly..

> So, that would be a cleanup rather than a fix, right?

Hmm, yeah..

Re: [PATCH 2/2] cpufreq: don't call cpufreq_update_policy() on CPU addition

[Rafael J. Wysocki]

On Monday, February 17, 2014 10:45:41 AM Viresh Kumar wrote:
> On 17 February 2014 05:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, February 14, 2014 04:30:41 PM Viresh Kumar wrote:
> >> cpufreq_update_policy() is called from two places currently. From a workqueue
> >> handled queued from cpufreq_bp_resume() for boot CPU and from
> >> cpufreq_cpu_callback() whenever a CPU is added.
> >>
> >> The first one makes sure that boot CPU is running on the frequency present in
> >> policy->cpu. But we don't really need a call from cpufreq_cpu_callback(),
> >> because we always call cpufreq_driver->init() (which will set policy->cur
> >> correctly) whenever first CPU of any policy is added back. And so every policy
> >> structure is guaranteed to have the right frequency in policy->cur.
> >
> > That sounds good, but doing the extra cpufreq_update_policy() shouldn't actually
> > hurt, should it?
> 
> Yeah, it shouldn't hurt badly..
> 
> > So, that would be a cleanup rather than a fix, right?
> 
> Hmm, yeah..

I've queued this up for 3.15, then.  Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpufreq: don't call cpufreq_update_policy() on CPU addition

[Srivatsa S. Bhat]

On 02/14/2014 04:30 PM, Viresh Kumar wrote:
> cpufreq_update_policy() is called from two places currently. From a workqueue
> handled queued from cpufreq_bp_resume() for boot CPU and from
> cpufreq_cpu_callback() whenever a CPU is added.
> 
> The first one makes sure that boot CPU is running on the frequency present in
> policy->cpu. But we don't really need a call from cpufreq_cpu_callback(),
> because we always call cpufreq_driver->init() (which will set policy->cur
> correctly) whenever first CPU of any policy is added back. And so every policy
> structure is guaranteed to have the right frequency in policy->cur.
>

This wording is slightly inaccurate. ->init() may or may not set policy->cur
(for example, powernowk8 driver doesn't set it in the init routine)..
But we set it for sure in __cpufreq_add_dev():

1117         if (cpufreq_driver->get) {
1118                 policy->cur = cpufreq_driver->get(policy->cpu);
1119                 if (!policy->cur) {
1120                         pr_err("%s: ->get() failed\n", __func__);
1121                         goto err_get_freq;
1122                 }
1123         }

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

The reasoning and the code looks good to me.

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

> ---
>  drivers/cpufreq/cpufreq.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 383362b..b6eb4ed 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2194,7 +2194,6 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,
>  		switch (action & ~CPU_TASKS_FROZEN) {
>  		case CPU_ONLINE:
>  			__cpufreq_add_dev(dev, NULL, frozen);
> -			cpufreq_update_policy(cpu);
>  			break;
> 
>  		case CPU_DOWN_PREPARE:
> 

Re: [PATCH 2/2] cpufreq: don't call cpufreq_update_policy() on CPU addition

[Viresh Kumar]

On 17 February 2014 14:13, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 02/14/2014 04:30 PM, Viresh Kumar wrote:
>> cpufreq_update_policy() is called from two places currently. From a workqueue
>> handled queued from cpufreq_bp_resume() for boot CPU and from
>> cpufreq_cpu_callback() whenever a CPU is added.
>>
>> The first one makes sure that boot CPU is running on the frequency present in
>> policy->cpu. But we don't really need a call from cpufreq_cpu_callback(),
>> because we always call cpufreq_driver->init() (which will set policy->cur
>> correctly) whenever first CPU of any policy is added back. And so every policy
>> structure is guaranteed to have the right frequency in policy->cur.
>>
>
> This wording is slightly inaccurate. ->init() may or may not set policy->cur
> (for example, powernowk8 driver doesn't set it in the init routine)..

Its not the wording that is wrong but this particular driver then :)
This is what Documentation/cpu-drivers.txt says:

1.2 Per-CPU Initialization
Then, the driver must fill in the following values:

policy->cur The current operating frequency of
this CPU (if appropriate)

And so it is supposed to do it.

> But we set it for sure in __cpufreq_add_dev():
>
> 1117         if (cpufreq_driver->get) {
> 1118                 policy->cur = cpufreq_driver->get(policy->cpu);
> 1119                 if (!policy->cur) {
> 1120                         pr_err("%s: ->get() failed\n", __func__);
> 1121                         goto err_get_freq;
> 1122                 }
> 1123         }

Its just about removing that from drivers and doing it once in core :)

>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> The reasoning and the code looks good to me.
>
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Thanks.

Re: [PATCH 2/2] cpufreq: don't call cpufreq_update_policy() on CPU addition

[Srivatsa S. Bhat]

On 02/17/2014 02:24 PM, Viresh Kumar wrote:
> On 17 February 2014 14:13, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> On 02/14/2014 04:30 PM, Viresh Kumar wrote:
>>> cpufreq_update_policy() is called from two places currently. From a workqueue
>>> handled queued from cpufreq_bp_resume() for boot CPU and from
>>> cpufreq_cpu_callback() whenever a CPU is added.
>>>
>>> The first one makes sure that boot CPU is running on the frequency present in
>>> policy->cpu. But we don't really need a call from cpufreq_cpu_callback(),
>>> because we always call cpufreq_driver->init() (which will set policy->cur
>>> correctly) whenever first CPU of any policy is added back. And so every policy
>>> structure is guaranteed to have the right frequency in policy->cur.
>>>
>>
>> This wording is slightly inaccurate. ->init() may or may not set policy->cur
>> (for example, powernowk8 driver doesn't set it in the init routine)..
> 
> Its not the wording that is wrong but this particular driver then :)
> This is what Documentation/cpu-drivers.txt says:
> 
> 1.2 Per-CPU Initialization
> Then, the driver must fill in the following values:
> 
> policy->cur The current operating frequency of
> this CPU (if appropriate)
> 
> And so it is supposed to do it.
>

Ah, I see.
 
>> But we set it for sure in __cpufreq_add_dev():
>>
>> 1117         if (cpufreq_driver->get) {
>> 1118                 policy->cur = cpufreq_driver->get(policy->cpu);
>> 1119                 if (!policy->cur) {
>> 1120                         pr_err("%s: ->get() failed\n", __func__);
>> 1121                         goto err_get_freq;
>> 1122                 }
>> 1123         }
> 
> Its just about removing that from drivers and doing it once in core :)
>

Ok..

Regards,
Srivatsa S. Bhat

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

[Rafael J. Wysocki]

On Friday, February 14, 2014 04:30:40 PM Viresh Kumar wrote:
> 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..

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.

>  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.

> +			goto no_policy;

And is it unsafe to continue here?  Or can we continue regardless of getting 0?

> +		}
> +
>  		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.

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

[Viresh Kumar]

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.

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

[Srivatsa S. Bhat]

On 02/17/2014 10:44 AM, Viresh Kumar wrote:
> 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.
> 

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.

Thoughts?

Regards,
Srivatsa S. Bhat

>>> +             }
>>> +
>>>               if (!policy->cur) {
>>>                       pr_debug("Driver did not initialize current freq");
>>>                       policy->cur = new_policy.cur;
>>>

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

[Viresh Kumar]

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.

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@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.

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

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

[Viresh Kumar]

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.

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

[Rafael J. Wysocki]

On Monday, February 17, 2014 02:25:34 PM Srivatsa S. Bhat wrote:
> On 02/17/2014 02:09 PM, Viresh Kumar wrote:
> > 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.
> 
> 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.

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?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Viresh Kumar]

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?

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

[Viresh Kumar]

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!!

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

[Srivatsa S. Bhat]

On 02/25/2014 10:11 AM, Viresh Kumar wrote:
> 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.

A WARN() would definitely be good.

>>
>> 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?
> 

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.

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.
 
Regards,
Srivatsa S. Bhat

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

[Viresh Kumar]

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..

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

[Rafael J. Wysocki]

On Tuesday, February 25, 2014 11:38:14 AM Viresh Kumar wrote:
> 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.

Well, why exactly does the core need to operate "current frequency" at all?

Rafael

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

[Viresh Kumar]

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?

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

[Rafael J. Wysocki]

On Tuesday, February 25, 2014 08:12:52 PM Viresh Kumar wrote:
> 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?

Yes, what exactly do we need it for in the core?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Viresh Kumar]

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.

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

[Viresh Kumar]

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!!