[PATCH V2 0/5] cpufreq: cleanups

[PATCH V2 0/5] cpufreq: cleanups

[Viresh Kumar]

Hi Rafael,

I accumulated these while reworking the freq-constraint series and it
would be nice if these can get in before I send the next version of
freq-constraint stuff.

These are mostly cleanups and code consolidation for better management
of code. Compile and boot tested only.

Thanks.

V1->V2:
- Merged patch 2/6 and 3/6 (now called 2/5).
- Updated commit log of 3/5 as it wasn't clear enough earlier.

Viresh Kumar (5):
  cpufreq: Remove the redundant !setpolicy check
  cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()
  cpufreq: Use has_target() instead of !setpolicy
  cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get()
  cpufreq: Avoid calling cpufreq_verify_current_freq() from
    handle_update()

 drivers/cpufreq/cpufreq.c | 115 +++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 63 deletions(-)

-- 
2.21.0.rc0.269.g1a574e7a288b

[PATCH V2 1/5] cpufreq: Remove the redundant !setpolicy check

[Viresh Kumar]

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

[PATCH V2 2/5] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()

[Viresh Kumar]

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.

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

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..41ac701e324f 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 (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

[PATCH V2 3/5] cpufreq: Use has_target() instead of !setpolicy

[Viresh Kumar]

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()" instead of "cpufreq_driver->setpolicy" where we need to
check if the driver supports setpolicy, so to use only one expression
for this kind of 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

[PATCH V2 4/5] cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get()

[Viresh Kumar]

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

[PATCH V2 5/5] cpufreq: Avoid calling cpufreq_verify_current_freq() from handle_update()

[Viresh Kumar]

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

Re: [PATCH V2 2/5] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()

[Viresh Kumar]

On 20-06-19, 08:35, Viresh Kumar 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.
> > 
> > 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 as well.
> > 
> > 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..41ac701e324f 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 (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)) {

@Rafael: Here are your comments from the IRC exchange we had
yesterday:

> <rafael>:
> 
> so the problem is that, because of the CPUFREQ_CONST_LOOPS check in
> __cpufreq_get(), it almost never does the cpufreq_out_of_sync() thing
> now. Because many drivers set CPUFREQ_CONST_LOOPS most of the time,
> some of them even unconditionally. This patch changes the code that
> runs very rarely into code that runs relatively often.

Right, we will do the frequency verification on has_target() platforms
with CPUFREQ_CONST_LOOPS set after this patch. But why is it the wrong
thing to do ?

What we do here is that we verify that the cached value of current
frequency is same as the real frequency the hardware is running at. It
makes sense to not do this check for setpolicy type drivers as the
cpufreq core isn't always aware of what the driver will end up doing
with the frequency and so no verification.

But for has_target() type drivers, cpufreq core caches the value with
it and it should check it to make sure everything is fine. I don't see
a correlation with CPUFREQ_CONST_LOOPS flag here, that's it. Either we
do this verification or we don't, but there is no reason (as per my
understanding) of skipping it using this flag.

So if you look at the commit I pointed in the history git [1], it does
two things:
- It adds the verification code (which is quite similar today as
  well).
- And it sets the CPUFREQ_CONST_LOOPS flag only for setpolicy drivers,
  rightly so.

The problem happened when we started to use CPUFREQ_CONST_LOOPS for
constant loops-per-jiffy thing as well and many has_target() drivers
started using the same flag and unknowingly skipped the verification
of frequency.

So, I think the current code is doing the wrong thing by skipping the
verification using CPUFREQ_CONST_LOOPS flag.

-- 
viresh

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=6a4a93f9c0d51b5f4ac1bd3efab53e43584330dd

Re: [PATCH V2 2/5] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()

[Rafael J. Wysocki]

On Thu, Jun 27, 2019 at 7:00 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-06-19, 08:35, Viresh Kumar 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.
> > >
> > > 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 as well.
> > >
> > > 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..41ac701e324f 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 (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)) {
>
> @Rafael: Here are your comments from the IRC exchange we had
> yesterday:
>
> > <rafael>:
> >
> > so the problem is that, because of the CPUFREQ_CONST_LOOPS check in
> > __cpufreq_get(), it almost never does the cpufreq_out_of_sync() thing
> > now. Because many drivers set CPUFREQ_CONST_LOOPS most of the time,
> > some of them even unconditionally. This patch changes the code that
> > runs very rarely into code that runs relatively often.
>
> Right, we will do the frequency verification on has_target() platforms
> with CPUFREQ_CONST_LOOPS set after this patch. But why is it the wrong
> thing to do ?

Well, my point was exactly what I said.

The patch pretended to be a cleanup and changed the code in a
meaningful way (at least for some drivers).

> What we do here is that we verify that the cached value of current
> frequency is same as the real frequency the hardware is running at. It
> makes sense to not do this check for setpolicy type drivers as the
> cpufreq core isn't always aware of what the driver will end up doing
> with the frequency and so no verification.
>
> But for has_target() type drivers, cpufreq core caches the value with
> it and it should check it to make sure everything is fine. I don't see
> a correlation with CPUFREQ_CONST_LOOPS flag here, that's it. Either we
> do this verification or we don't, but there is no reason (as per my
> understanding) of skipping it using this flag.
>
> So if you look at the commit I pointed in the history git [1], it does
> two things:
> - It adds the verification code (which is quite similar today as
>   well).
> - And it sets the CPUFREQ_CONST_LOOPS flag only for setpolicy drivers,
>   rightly so.
>
> The problem happened when we started to use CPUFREQ_CONST_LOOPS for
> constant loops-per-jiffy thing as well and many has_target() drivers
> started using the same flag and unknowingly skipped the verification
> of frequency.
>
> So, I think the current code is doing the wrong thing by skipping the
> verification using CPUFREQ_CONST_LOOPS flag.

All right then, thanks for explaining it here.

The patch is a bug fix, not a cleanup, and it fixes the changes that
caused CPUFREQ_CONST_LOOPS to be used for a different purpose without
adjusting the original code accordingly.

I can agree with this rationale, but please fix the changelog.

Re: [PATCH V2 1/5] cpufreq: Remove the redundant !setpolicy check

[Rafael J. Wysocki]

On Thursday, June 20, 2019 5:05:46 AM CEST Viresh Kumar wrote:
> 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) {
> 

Applied, thanks!

Re: [PATCH V2 3/5] cpufreq: Use has_target() instead of !setpolicy

[Rafael J. Wysocki]

On Thursday, June 20, 2019 5:05:48 AM CEST Viresh Kumar wrote:
> 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()" instead of "cpufreq_driver->setpolicy" where we need to
> check if the driver supports setpolicy, so to use only one expression
> for this kind of 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;
>  
> 

Applied, thanks!

[PATCH V3 2/5] cpufreq: Don't skip frequency validation for has_target() drivers

[Viresh Kumar]

CPUFREQ_CONST_LOOPS was introduced in a very old commit from pre-2.6
kernel release by commit 6a4a93f9c0d5 ("[CPUFREQ] Fix 'out of sync'
issue").

If we you look at that commit, it does two things:

- It adds the frequency verification code (which is quite similar to
  what we have today as well).

- And it sets the CPUFREQ_CONST_LOOPS flag only for setpolicy drivers,
  rightly so based on the code we had then. The idea was to avoid
  frequency validation for setpolicy drivers as the cpufreq core doesn't
  know what frequency the hardware is running at and so no point in
  doing frequency verification.

The problem happened when we started to use the same CPUFREQ_CONST_LOOPS
flag for constant loops-per-jiffy thing as well and many has_target()
drivers started using the same flag and unknowingly skipped the
verification of frequency. There is no logical reason behind skipping
frequency validation because of the presence of CPUFREQ_CONST_LOOPS
flag otherwise.

This patch fixes this issue by skipping frequency validation only for
setpolicy drivers and always doing it for has_target() drivers
irrespective of the presence or absence of CPUFREQ_CONST_LOOPS flag.

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

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>
---
V2->V3:
- Updated commit log and $subject.

 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..41ac701e324f 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 (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