* [PATCH 1/6] cpufreq: Remove the redundant !setpolicy check
2019-06-19 11:35 [PATCH 0/6] cpufreq: cleanups Viresh Kumar
@ 2019-06-19 11:35 ` Viresh Kumar
2019-06-19 11:35 ` [PATCH 2/6] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target() Viresh Kumar
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 11:35 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
cpufreq_start_governor() is only called for !setpolicy case, checking it
again is not required.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 85ff958e01f1..54befd775bd6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2153,7 +2153,7 @@ static int cpufreq_start_governor(struct cpufreq_policy *policy)
pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
- if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
+ if (cpufreq_driver->get)
cpufreq_update_current_freq(policy);
if (policy->governor->start) {
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/6] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()
2019-06-19 11:35 [PATCH 0/6] cpufreq: cleanups Viresh Kumar
2019-06-19 11:35 ` [PATCH 1/6] cpufreq: Remove the redundant !setpolicy check Viresh Kumar
@ 2019-06-19 11:35 ` Viresh Kumar
2019-06-19 12:20 ` Rafael J. Wysocki
2019-06-19 11:35 ` [PATCH 3/6] cpufreq: Remove the has_target() check from notifier handler Viresh Kumar
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 11:35 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
CPUFREQ_CONST_LOOPS was introduced in a very old commit from pre-2.6
kernel release commit 6a4a93f9c0d5 ("[CPUFREQ] Fix 'out of sync'
issue").
Probably the initial idea was to just avoid these checks for set_policy
type drivers and then things got changed over the years. And it is very
unclear why these checks are there at all.
Replace the CPUFREQ_CONST_LOOPS check with has_target(), which makes
more sense now.
Also remove () around freq comparison statement as they aren't required
and checkpatch also warns for them.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 54befd775bd6..e59194c2c613 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -359,12 +359,10 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
* which is not equal to what the cpufreq core thinks is
* "old frequency".
*/
- if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
- if (policy->cur && (policy->cur != freqs->old)) {
- pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
- freqs->old, policy->cur);
- freqs->old = policy->cur;
- }
+ if (has_target() && policy->cur && policy->cur != freqs->old) {
+ pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
+ freqs->old, policy->cur);
+ freqs->old = policy->cur;
}
srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
@@ -1618,8 +1616,7 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
if (policy->fast_switch_enabled)
return ret_freq;
- if (ret_freq && policy->cur &&
- !(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
+ if (has_target() && ret_freq && policy->cur) {
/* verify no discrepancy between actual and
saved value exists */
if (unlikely(ret_freq != policy->cur)) {
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()
2019-06-19 11:35 ` [PATCH 2/6] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target() Viresh Kumar
@ 2019-06-19 12:20 ` Rafael J. Wysocki
2019-06-19 14:18 ` Viresh Kumar
0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-06-19 12:20 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Linux Kernel Mailing List
On Wed, Jun 19, 2019 at 1:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> CPUFREQ_CONST_LOOPS was introduced in a very old commit from pre-2.6
> kernel release commit 6a4a93f9c0d5 ("[CPUFREQ] Fix 'out of sync'
> issue").
>
> Probably the initial idea was to just avoid these checks for set_policy
> type drivers and then things got changed over the years. And it is very
> unclear why these checks are there at all.
>
> Replace the CPUFREQ_CONST_LOOPS check with has_target(), which makes
> more sense now.
>
> Also remove () around freq comparison statement as they aren't required
> and checkpatch also warns for them.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/cpufreq.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 54befd775bd6..e59194c2c613 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -359,12 +359,10 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
> * which is not equal to what the cpufreq core thinks is
> * "old frequency".
> */
> - if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
> - if (policy->cur && (policy->cur != freqs->old)) {
> - pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> - freqs->old, policy->cur);
> - freqs->old = policy->cur;
> - }
> + if (has_target() && policy->cur && policy->cur != freqs->old) {
> + pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> + freqs->old, policy->cur);
> + freqs->old = policy->cur;
Is cpufreq_notify_transition() ever called if ->setpolicy drivers are in use?
> }
>
> srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
> @@ -1618,8 +1616,7 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
> if (policy->fast_switch_enabled)
> return ret_freq;
>
> - if (ret_freq && policy->cur &&
> - !(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
> + if (has_target() && ret_freq && policy->cur) {
> /* verify no discrepancy between actual and
> saved value exists */
> if (unlikely(ret_freq != policy->cur)) {
> --
> 2.21.0.rc0.269.g1a574e7a288b
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()
2019-06-19 12:20 ` Rafael J. Wysocki
@ 2019-06-19 14:18 ` Viresh Kumar
0 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 14:18 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Linux Kernel Mailing List
On 19-06-19, 14:20, Rafael J. Wysocki wrote:
> On Wed, Jun 19, 2019 at 1:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 54befd775bd6..e59194c2c613 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -359,12 +359,10 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
> > * which is not equal to what the cpufreq core thinks is
> > * "old frequency".
> > */
> > - if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
> > - if (policy->cur && (policy->cur != freqs->old)) {
> > - pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> > - freqs->old, policy->cur);
> > - freqs->old = policy->cur;
> > - }
> > + if (has_target() && policy->cur && policy->cur != freqs->old) {
> > + pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> > + freqs->old, policy->cur);
> > + freqs->old = policy->cur;
>
> Is cpufreq_notify_transition() ever called if ->setpolicy drivers are in use?
I tried to find it, but I couldn't find any driver from where we can
get this called for setpolicy drivers.
--
viresh
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/6] cpufreq: Remove the has_target() check from notifier handler
2019-06-19 11:35 [PATCH 0/6] cpufreq: cleanups Viresh Kumar
2019-06-19 11:35 ` [PATCH 1/6] cpufreq: Remove the redundant !setpolicy check Viresh Kumar
2019-06-19 11:35 ` [PATCH 2/6] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target() Viresh Kumar
@ 2019-06-19 11:35 ` Viresh Kumar
2019-06-19 12:25 ` Rafael J. Wysocki
2019-06-19 11:35 ` [PATCH 4/6] cpufreq: Use has_target() instead of !setpolicy Viresh Kumar
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 11:35 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
cpufreq_notify_transition() is only called for has_target() type driver
and not for set_policy type, and the check is simply redundant. Remove
it.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e59194c2c613..41ac701e324f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -359,7 +359,7 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
* which is not equal to what the cpufreq core thinks is
* "old frequency".
*/
- if (has_target() && policy->cur && policy->cur != freqs->old) {
+ if (policy->cur && policy->cur != freqs->old) {
pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
freqs->old, policy->cur);
freqs->old = policy->cur;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/6] cpufreq: Remove the has_target() check from notifier handler
2019-06-19 11:35 ` [PATCH 3/6] cpufreq: Remove the has_target() check from notifier handler Viresh Kumar
@ 2019-06-19 12:25 ` Rafael J. Wysocki
2019-06-19 14:19 ` Viresh Kumar
0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-06-19 12:25 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Linux Kernel Mailing List
On Wed, Jun 19, 2019 at 1:35 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> cpufreq_notify_transition() is only called for has_target() type driver
> and not for set_policy type, and the check is simply redundant. Remove
> it.
Ah, OK
So this patch removes a check introduced by the previous one. :-)
Please merge them together.
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/cpufreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e59194c2c613..41ac701e324f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -359,7 +359,7 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
> * which is not equal to what the cpufreq core thinks is
> * "old frequency".
> */
> - if (has_target() && policy->cur && policy->cur != freqs->old) {
> + if (policy->cur && policy->cur != freqs->old) {
> pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> freqs->old, policy->cur);
> freqs->old = policy->cur;
> --
> 2.21.0.rc0.269.g1a574e7a288b
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/6] cpufreq: Remove the has_target() check from notifier handler
2019-06-19 12:25 ` Rafael J. Wysocki
@ 2019-06-19 14:19 ` Viresh Kumar
0 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 14:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Linux Kernel Mailing List
On 19-06-19, 14:25, Rafael J. Wysocki wrote:
> On Wed, Jun 19, 2019 at 1:35 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > cpufreq_notify_transition() is only called for has_target() type driver
> > and not for set_policy type, and the check is simply redundant. Remove
> > it.
>
> Ah, OK
>
> So this patch removes a check introduced by the previous one. :-)
>
> Please merge them together.
It made sense to keep them separate because there are two different
issues I am fixing here. But if that is what you want, I will merge
them.
--
viresh
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/6] cpufreq: Use has_target() instead of !setpolicy
2019-06-19 11:35 [PATCH 0/6] cpufreq: cleanups Viresh Kumar
` (2 preceding siblings ...)
2019-06-19 11:35 ` [PATCH 3/6] cpufreq: Remove the has_target() check from notifier handler Viresh Kumar
@ 2019-06-19 11:35 ` Viresh Kumar
2019-06-19 12:28 ` Rafael J. Wysocki
2019-06-19 11:35 ` [PATCH 5/6] cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get() Viresh Kumar
2019-06-19 11:35 ` [PATCH 6/6] cpufreq: Avoid calling cpufreq_verify_current_freq() from handle_update() Viresh Kumar
5 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 11:35 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
For code consistency, use has_target() instead of !setpolicy everywhere,
as it is already done at several places. Maybe we should also use
!has_target() for setpolicy case to use only one expression for this
differentiation.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 41ac701e324f..5f5c7a516c74 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -632,7 +632,7 @@ static int cpufreq_parse_policy(char *str_governor,
}
/**
- * cpufreq_parse_governor - parse a governor string only for !setpolicy
+ * cpufreq_parse_governor - parse a governor string only for has_target()
*/
static int cpufreq_parse_governor(char *str_governor,
struct cpufreq_policy *policy)
@@ -1301,7 +1301,7 @@ static int cpufreq_online(unsigned int cpu)
policy->max = policy->user_policy.max;
}
- if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
+ if (cpufreq_driver->get && has_target()) {
policy->cur = cpufreq_driver->get(policy->cpu);
if (!policy->cur) {
pr_err("%s: ->get() failed\n", __func__);
@@ -2401,7 +2401,7 @@ void cpufreq_update_policy(unsigned int cpu)
* BIOS might change freq behind our back
* -> ask driver for current freq and notify governors about a change
*/
- if (cpufreq_driver->get && !cpufreq_driver->setpolicy &&
+ if (cpufreq_driver->get && has_target() &&
(cpufreq_suspended || WARN_ON(!cpufreq_update_current_freq(policy))))
goto unlock;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 4/6] cpufreq: Use has_target() instead of !setpolicy
2019-06-19 11:35 ` [PATCH 4/6] cpufreq: Use has_target() instead of !setpolicy Viresh Kumar
@ 2019-06-19 12:28 ` Rafael J. Wysocki
2019-06-19 14:20 ` Viresh Kumar
0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-06-19 12:28 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Linux Kernel Mailing List
On Wed, Jun 19, 2019 at 1:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> For code consistency, use has_target() instead of !setpolicy everywhere,
> as it is already done at several places.
That's OK
> Maybe we should also use !has_target() for setpolicy case to use only one expression
> for this differentiation.
But I'm not sure what you mean here?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/6] cpufreq: Use has_target() instead of !setpolicy
2019-06-19 12:28 ` Rafael J. Wysocki
@ 2019-06-19 14:20 ` Viresh Kumar
0 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 14:20 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Linux Kernel Mailing List
On 19-06-19, 14:28, Rafael J. Wysocki wrote:
> On Wed, Jun 19, 2019 at 1:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > For code consistency, use has_target() instead of !setpolicy everywhere,
> > as it is already done at several places.
>
> That's OK
>
> > Maybe we should also use !has_target() for setpolicy case to use only one expression
> > for this differentiation.
>
> But I'm not sure what you mean here?
At many places in code we are doing tests like:
if (cpufreq_driver->setpolicy) {
xxx
}
Maybe we can write them as well like:
if (!has_target()) {
xxx
}
--
viresh
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/6] cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get()
2019-06-19 11:35 [PATCH 0/6] cpufreq: cleanups Viresh Kumar
` (3 preceding siblings ...)
2019-06-19 11:35 ` [PATCH 4/6] cpufreq: Use has_target() instead of !setpolicy Viresh Kumar
@ 2019-06-19 11:35 ` Viresh Kumar
2019-06-19 11:35 ` [PATCH 6/6] cpufreq: Avoid calling cpufreq_verify_current_freq() from handle_update() Viresh Kumar
5 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 11:35 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
Their implementations are quite similar, lets modify
cpufreq_update_current_freq() a little and use it from __cpufreq_get().
Also rename cpufreq_update_current_freq() to
cpufreq_verify_current_freq(), as that's what it is doing.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 70 ++++++++++++++++-----------------------
1 file changed, 28 insertions(+), 42 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5f5c7a516c74..4556a53fc764 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1547,6 +1547,30 @@ static void cpufreq_out_of_sync(struct cpufreq_policy *policy,
cpufreq_freq_transition_end(policy, &freqs, 0);
}
+static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, bool update)
+{
+ unsigned int new_freq;
+
+ new_freq = cpufreq_driver->get(policy->cpu);
+ if (!new_freq)
+ return 0;
+
+ /*
+ * If fast frequency switching is used with the given policy, the check
+ * against policy->cur is pointless, so skip it in that case.
+ */
+ if (policy->fast_switch_enabled || !has_target())
+ return new_freq;
+
+ if (policy->cur != new_freq) {
+ cpufreq_out_of_sync(policy, new_freq);
+ if (update)
+ schedule_work(&policy->update);
+ }
+
+ return new_freq;
+}
+
/**
* cpufreq_quick_get - get the CPU frequency (in kHz) from policy->cur
* @cpu: CPU number
@@ -1602,30 +1626,10 @@ EXPORT_SYMBOL(cpufreq_quick_get_max);
static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
{
- unsigned int ret_freq = 0;
-
if (unlikely(policy_is_inactive(policy)))
- return ret_freq;
-
- ret_freq = cpufreq_driver->get(policy->cpu);
-
- /*
- * If fast frequency switching is used with the given policy, the check
- * against policy->cur is pointless, so skip it in that case too.
- */
- if (policy->fast_switch_enabled)
- return ret_freq;
-
- if (has_target() && ret_freq && policy->cur) {
- /* verify no discrepancy between actual and
- saved value exists */
- if (unlikely(ret_freq != policy->cur)) {
- cpufreq_out_of_sync(policy, ret_freq);
- schedule_work(&policy->update);
- }
- }
+ return 0;
- return ret_freq;
+ return cpufreq_verify_current_freq(policy, true);
}
/**
@@ -1652,24 +1656,6 @@ unsigned int cpufreq_get(unsigned int cpu)
}
EXPORT_SYMBOL(cpufreq_get);
-static unsigned int cpufreq_update_current_freq(struct cpufreq_policy *policy)
-{
- unsigned int new_freq;
-
- new_freq = cpufreq_driver->get(policy->cpu);
- if (!new_freq)
- return 0;
-
- if (!policy->cur) {
- pr_debug("cpufreq: Driver did not initialize current freq\n");
- policy->cur = new_freq;
- } else if (policy->cur != new_freq && has_target()) {
- cpufreq_out_of_sync(policy, new_freq);
- }
-
- return new_freq;
-}
-
static struct subsys_interface cpufreq_interface = {
.name = "cpufreq",
.subsys = &cpu_subsys,
@@ -2151,7 +2137,7 @@ static int cpufreq_start_governor(struct cpufreq_policy *policy)
pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
if (cpufreq_driver->get)
- cpufreq_update_current_freq(policy);
+ cpufreq_verify_current_freq(policy, false);
if (policy->governor->start) {
ret = policy->governor->start(policy);
@@ -2402,7 +2388,7 @@ void cpufreq_update_policy(unsigned int cpu)
* -> ask driver for current freq and notify governors about a change
*/
if (cpufreq_driver->get && has_target() &&
- (cpufreq_suspended || WARN_ON(!cpufreq_update_current_freq(policy))))
+ (cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
goto unlock;
pr_debug("updating policy for CPU %u\n", cpu);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 6/6] cpufreq: Avoid calling cpufreq_verify_current_freq() from handle_update()
2019-06-19 11:35 [PATCH 0/6] cpufreq: cleanups Viresh Kumar
` (4 preceding siblings ...)
2019-06-19 11:35 ` [PATCH 5/6] cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get() Viresh Kumar
@ 2019-06-19 11:35 ` Viresh Kumar
5 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2019-06-19 11:35 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
On some occasions cpufreq_verify_current_freq() schedules a work whose
callback is handle_update(), which further calls cpufreq_update_policy()
which may end up calling cpufreq_verify_current_freq() again.
On the other hand, when cpufreq_update_policy() is called from
handle_update(), the pointer to the cpufreq policy is already available
but we still call cpufreq_cpu_acquire() to get it in
cpufreq_update_policy(), which should be avoided as well.
Fix both the issues by creating another helper
reeval_frequency_limits().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4556a53fc764..0a73de7aae54 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1115,13 +1115,25 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
return ret;
}
+static void reeval_frequency_limits(struct cpufreq_policy *policy)
+{
+ struct cpufreq_policy new_policy = *policy;
+
+ pr_debug("updating policy for CPU %u\n", policy->cpu);
+
+ new_policy.min = policy->user_policy.min;
+ new_policy.max = policy->user_policy.max;
+
+ cpufreq_set_policy(policy, &new_policy);
+}
+
static void handle_update(struct work_struct *work)
{
struct cpufreq_policy *policy =
container_of(work, struct cpufreq_policy, update);
- unsigned int cpu = policy->cpu;
- pr_debug("handle_update for cpu %u called\n", cpu);
- cpufreq_update_policy(cpu);
+
+ pr_debug("handle_update for cpu %u called\n", policy->cpu);
+ reeval_frequency_limits(policy);
}
static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
@@ -2378,7 +2390,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
void cpufreq_update_policy(unsigned int cpu)
{
struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
- struct cpufreq_policy new_policy;
if (!policy)
return;
@@ -2391,12 +2402,7 @@ void cpufreq_update_policy(unsigned int cpu)
(cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
goto unlock;
- pr_debug("updating policy for CPU %u\n", cpu);
- memcpy(&new_policy, policy, sizeof(*policy));
- new_policy.min = policy->user_policy.min;
- new_policy.max = policy->user_policy.max;
-
- cpufreq_set_policy(policy, &new_policy);
+ reeval_frequency_limits(policy);
unlock:
cpufreq_cpu_release(policy);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 13+ messages in thread