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;
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:
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:
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..
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!
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:
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.
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
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;
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.
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;
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.
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
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.
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?
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?
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!!
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
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..
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
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?
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?
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.
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!!
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Srivatsa S. Bhat -
Viresh Kumar