Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   zhuguangqing83@gmail.com
To:     viresh.kumar@linaro.org, rjw@rjwysocki.net, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhuguangqing <zhuguangqing@xiaomi.com>
Subject: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the final cpufreq
Date:   Tue, 27 Oct 2020 19:54:59 +0800
Message-Id: <20201027115459.19318-1-zhuguangqing83@gmail.com>
Precedence: bulk

From: zhuguangqing <zhuguangqing@xiaomi.com>

In the following code path, next_freq is clamped between policy->min
and policy->max twice in functions cpufreq_driver_resolve_freq() and
cpufreq_driver_fast_switch(). For there is no update_lock in the code
path, policy->min and policy->max may be modified (one or more times),
so sg_policy->next_freq updated in function sugov_update_next_freq()
may be not the final cpufreq. Next time when we use
"if (sg_policy->next_freq == next_freq)" to judge whether to update
next_freq, we may get a wrong result.

-> sugov_update_single()
  -> get_next_freq()
    -> cpufreq_driver_resolve_freq()
  -> sugov_fast_switch()
    -> sugov_update_next_freq()
    -> cpufreq_driver_fast_switch()

For example, at first sg_policy->next_freq is 1 GHz, but the final
cpufreq is 1.2 GHz because policy->min is modified to 1.2 GHz when
we reached cpufreq_driver_fast_switch(). Then next time, policy->min
is changed before we reached cpufreq_driver_resolve_freq() and (assume)
next_freq is 1 GHz, we find "if (sg_policy->next_freq == next_freq)" is
satisfied so we don't change the cpufreq. Actually we should change
the cpufreq to 1.0 GHz this time.

Signed-off-by: zhuguangqing <zhuguangqing@xiaomi.com>
---
 drivers/cpufreq/cpufreq.c        |  6 +++---
 include/linux/cpufreq.h          |  2 +-
 kernel/sched/cpufreq_schedutil.c | 21 ++++++++++-----------
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f4b60663efe6..7e8e03c7506b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2057,13 +2057,13 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
  * error condition, the hardware configuration must be preserved.
  */
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
-					unsigned int target_freq)
+					unsigned int *target_freq)
 {
 	unsigned int freq;
 	int cpu;
 
-	target_freq = clamp_val(target_freq, policy->min, policy->max);
-	freq = cpufreq_driver->fast_switch(policy, target_freq);
+	*target_freq = clamp_val(*target_freq, policy->min, policy->max);
+	freq = cpufreq_driver->fast_switch(policy, *target_freq);
 
 	if (!freq)
 		return 0;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index fa37b1c66443..790df38d48de 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -569,7 +569,7 @@ struct cpufreq_governor {
 
 /* Pass a target to the cpufreq driver */
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
-					unsigned int target_freq);
+					unsigned int *target_freq);
 int cpufreq_driver_target(struct cpufreq_policy *policy,
 				 unsigned int target_freq,
 				 unsigned int relation);
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e254745a82cb..38d2dc55dd95 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -99,31 +99,30 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
 	return delta_ns >= sg_policy->freq_update_delay_ns;
 }
 
-static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
+static inline bool sugov_update_next_freq(struct sugov_policy *sg_policy,
 				   unsigned int next_freq)
 {
-	if (sg_policy->next_freq == next_freq)
-		return false;
-
-	sg_policy->next_freq = next_freq;
-	sg_policy->last_freq_update_time = time;
-
-	return true;
+	return sg_policy->next_freq == next_freq ? false : true;
 }
 
 static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
 			      unsigned int next_freq)
 {
-	if (sugov_update_next_freq(sg_policy, time, next_freq))
-		cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
+	if (sugov_update_next_freq(sg_policy, next_freq)) {
+		cpufreq_driver_fast_switch(sg_policy->policy, &next_freq);
+		sg_policy->next_freq = next_freq;
+		sg_policy->last_freq_update_time = time;
+	}
 }
 
 static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
 				  unsigned int next_freq)
 {
-	if (!sugov_update_next_freq(sg_policy, time, next_freq))
+	if (!sugov_update_next_freq(sg_policy, next_freq))
 		return;
 
+	sg_policy->next_freq = next_freq;
+	sg_policy->last_freq_update_time = time;
 	if (!sg_policy->work_in_progress) {
 		sg_policy->work_in_progress = true;
 		irq_work_queue(&sg_policy->irq_work);
-- 
2.17.1