cpufreq: intel_pstate: Avoid missing HWP max limit updates with powersave governor

From: Rafael J. Wysocki <[email protected]>

Generally, a cpufreq driver may need to update some internal upper and lower frequency boundaries on policy max and min changes, respectively, but currently this does not work if the target frequency does not change along with the policy limit.

Namely, if the target frequency does not change along with the policy min or max, the "target_freq == policy->cur" check in __cpufreq_driver_target() prevents driver callbacks from being invoked and they do not even have a chance to update the corresponding internal boundary.

This particularly affects the "powersave" and "performance" governors that always set the target frequency to one of the policy limits and it never changes when the other limit is updated.

To allow cpufreq the drivers needing to update internal frequency boundaries on policy limits changes to avoid this issue, introduce a new driver flag, CPUFREQ_NEED_UPDATE_LIMITS, that (when set) will neutralize the check mentioned above.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

New patch in v2.

---
 drivers/cpufreq/cpufreq.c |    3 ++-
 include/linux/cpufreq.h   |   10 +++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

Index: linux-pm/include/linux/cpufreq.h
===================================================================

include/linux/cpufreq.h +9 -1

@@ -297,7 +297,7 @@ __ATTR(_name, 0644, show_##_name, store_
 
 struct cpufreq_driver {
 	char		name[CPUFREQ_NAME_LEN];
-	u8		flags;
+	u16		flags;
 	void		*driver_data;
 
 	/* needed by all drivers */

@@ -417,6 +417,14 @@ struct cpufreq_driver {
  */
 #define CPUFREQ_IS_COOLING_DEV			BIT(7)
 
+/*
+ * Set by drivers that need to update internale upper and lower boundaries along
+ * with the target frequency and so the core and governors should also invoke
+ * the diver if the target frequency does not change, but the policy min or max
+ * may have changed.
+ */
+#define CPUFREQ_NEED_UPDATE_LIMITS		BIT(8)
+
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
 

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================

From: Rafael J. Wysocki <[email protected]>

If the cpufreq policy max limit is changed when intel_pstate operates in the passive mode with HWP enabled and the "powersave" governor is used on top of it, the HWP max limit is not updated as appropriate.

Namely, in the "powersave" governor case, the target P-state is always equal to the policy min limit, so if the latter does not change, intel_cpufreq_adjust_hwp() is not invoked to update the HWP Request MSR due to the "target_pstate != old_pstate" check in intel_cpufreq_update_pstate(), so the HWP max limit is not updated as a result.

Also, if the CPUFREQ_NEED_UPDATE_LIMITS flag is not set for the driver and the target frequency does not change along with the policy max limit, the "target_freq == policy->cur" check in __cpufreq_driver_target() prevents the driver's ->target() callback from being invoked at all, so the HWP max limit is not updated.

To prevent that occurring, set the CPUFREQ_NEED_UPDATE_LIMITS flag in the intel_cpufreq driver structure if HWP is enabled and modify intel_cpufreq_update_pstate() to do the "target_pstate != old_pstate" check only in the non-HWP case and let intel_cpufreq_adjust_hwp() always run in the HWP case (it will update HWP Request only if the cached value of the register is different from the new one including the limits, so if neither the target P-state value nor the max limit changes, the register write will still be avoided).

Fixes f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode with HWP enabled")

Reported-by Zhang Rui <[email protected]>

Cc 5.9+ <[email protected]> # 5.9+

Signed-off-by Rafael J. Wysocki <[email protected]>

---

The v2 is just the intel_pstate changes (without the core changes) and setting the new flag.

---
 drivers/cpufreq/intel_pstate.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================

drivers/cpufreq/intel_pstate.c +5 -7

@@ -2550,14 +2550,12 @@ static int intel_cpufreq_update_pstate(s
 	int old_pstate = cpu->pstate.current_pstate;
 
 	target_pstate = intel_pstate_prepare_request(cpu, target_pstate);
-	if (target_pstate != old_pstate) {
+	if (hwp_active) {
+		intel_cpufreq_adjust_hwp(cpu, target_pstate, fast_switch);
+		cpu->pstate.current_pstate = target_pstate;
+	} else if (target_pstate != old_pstate) {
+		intel_cpufreq_adjust_perf_ctl(cpu, target_pstate, fast_switch);
 		cpu->pstate.current_pstate = target_pstate;
-		if (hwp_active)
-			intel_cpufreq_adjust_hwp(cpu, target_pstate,
-						 fast_switch);
-		else
-			intel_cpufreq_adjust_perf_ctl(cpu, target_pstate,
-						      fast_switch);
 	}
 
 	intel_cpufreq_trace(cpu, fast_switch ? INTEL_PSTATE_TRACE_FAST_SWITCH :

From: Rafael J. Wysocki <[email protected]>

If the cpufreq policy max limit is changed when intel_pstate operates in the passive mode with HWP enabled and the "powersave" governor is used on top of it, the HWP max limit is not updated as appropriate.

Namely, in the "powersave" governor case, the target P-state is always equal to the policy min limit, so if the latter does not change, intel_cpufreq_adjust_hwp() is not invoked to update the HWP Request MSR due to the "target_pstate != old_pstate" check in intel_cpufreq_update_pstate(), so the HWP max limit is not updated as a result.

Also, if the CPUFREQ_NEED_UPDATE_LIMITS flag is not set for the driver and the target frequency does not change along with the policy max limit, the "target_freq == policy->cur" check in __cpufreq_driver_target() prevents the driver's ->target() callback from being invoked at all, so the HWP max limit is not updated.

To prevent that occurring, set the CPUFREQ_NEED_UPDATE_LIMITS flag in the intel_cpufreq driver structure if HWP is enabled and modify intel_cpufreq_update_pstate() to do the "target_pstate != old_pstate" check only in the non-HWP case and let intel_cpufreq_adjust_hwp() always run in the HWP case (it will update HWP Request only if the cached value of the register is different from the new one including the limits, so if neither the target P-state value nor the max limit changes, the register write will still be avoided).

Fixes f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode

with HWP enabled")

Reported-by Zhang Rui <[email protected]>

Cc 5.9+ <[email protected]> # 5.9+

Signed-off-by Rafael J. Wysocki <[email protected]>

---

The v2 is just the intel_pstate changes (without the core changes) and setting
the new flag.

---
 drivers/cpufreq/intel_pstate.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================

drivers/cpufreq/intel_pstate.c +5 -6

@@ -2550,14 +2550,12 @@ static int intel_cpufreq_update_pstate(s
 	int old_pstate = cpu->pstate.current_pstate;
 
 	target_pstate = intel_pstate_prepare_request(cpu,
 target_pstate);
-	if (target_pstate != old_pstate) {
+	if (hwp_active) {
+		intel_cpufreq_adjust_hwp(cpu, target_pstate,
 fast_switch);
+		cpu->pstate.current_pstate = target_pstate;
+	} else if (target_pstate != old_pstate) {
+		intel_cpufreq_adjust_perf_ctl(cpu, target_pstate,
 fast_switch);
 		cpu->pstate.current_pstate = target_pstate;
-		if (hwp_active)
-			intel_cpufreq_adjust_hwp(cpu, target_pstate,
-						 fast_switch);
-		else
-			intel_cpufreq_adjust_perf_ctl(cpu,

target_pstate,

drivers/cpufreq/intel_pstate.c +0 -1

-						      fast_switch);
 	}
 
 	intel_cpufreq_trace(cpu, fast_switch ?

INTEL_PSTATE_TRACE_FAST_SWITCH :

drivers/cpufreq/intel_pstate.c +1 -0

@@ -3014,6 +3012,7 @@ static int __init intel_pstate_init(void
 			hwp_mode_bdw = id->driver_data;
 			intel_pstate.attr = hwp_cpufreq_attrs;
 			intel_cpufreq.attr = hwp_cpufreq_attrs;
+			intel_cpufreq.flags |=
 CPUFREQ_NEED_UPDATE_LIMITS;
 			if (!default_driver)
 				default_driver = &intel_pstate;

From: Rafael J. Wysocki <[email protected]>

Because sugov_update_next_freq() may skip a frequency update even if the need_freq_update flag has been set for the policy at hand, policy limits updates may not take effect as expected.

For example, if the intel_pstate driver operates in the passive mode with HWP enabled, it needs to update the HWP min and max limits when the policy min and max limits change, respectively, but that may not happen if the target frequency does not change along with the limit at hand. In particular, if the policy min is changed first, causing the target frequency to be adjusted to it, and the policy max limit is changed later to the same value, the HWP max limit will not be updated to follow it as expected, because the target frequency is still equal to the policy min limit and it will not change until that limit is updated.

To address this issue, modify get_next_freq() to clear need_freq_update only if the CPUFREQ_NEED_UPDATE_LIMITS flag is not set for the cpufreq driver in use (and it should be set for all potentially affected drivers) and make sugov_update_next_freq() check need_freq_update and continue when it is set regardless of whether or not the new target frequency is equal to the old one.

Fixes f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode with HWP enabled")

Reported-by Zhang Rui <[email protected]>

Cc 5.9+ <[email protected]> # 5.9+

Signed-off-by Rafael J. Wysocki <[email protected]>

---

New patch in v2.

---
 kernel/sched/cpufreq_schedutil.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +2 -1

@@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
 				   unsigned int next_freq)
 {
-	if (sg_policy->next_freq == next_freq)
+	if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
 		return false;
 
 	sg_policy->next_freq = next_freq;
 	sg_policy->last_freq_update_time = time;
+	sg_policy->need_freq_update = false;
 
 	return true;
 }

Spelling mistake in $subject (driver)

Quoted 58 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Because sugov_update_next_freq() may skip a frequency update even if the need_freq_update flag has been set for the policy at hand, policy limits updates may not take effect as expected.

For example, if the intel_pstate driver operates in the passive mode with HWP enabled, it needs to update the HWP min and max limits when the policy min and max limits change, respectively, but that may not happen if the target frequency does not change along with the limit at hand. In particular, if the policy min is changed first, causing the target frequency to be adjusted to it, and the policy max limit is changed later to the same value, the HWP max limit will not be updated to follow it as expected, because the target frequency is still equal to the policy min limit and it will not change until that limit is updated.

To address this issue, modify get_next_freq() to clear need_freq_update only if the CPUFREQ_NEED_UPDATE_LIMITS flag is not set for the cpufreq driver in use (and it should be set for all potentially affected drivers) and make sugov_update_next_freq() check need_freq_update and continue when it is set regardless of whether or not the new target frequency is equal to the old one.

Fixes f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode with HWP enabled")

Reported-by Zhang Rui <[email protected]>

Cc 5.9+ <[email protected]> # 5.9+

Signed-off-by Rafael J. Wysocki <[email protected]>

---

New patch in v2.

---
 kernel/sched/cpufreq_schedutil.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +2 -1

@@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
                                 unsigned int next_freq)
 {
-     if (sg_policy->next_freq == next_freq)
+     if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
              return false;
 
      sg_policy->next_freq = next_freq;
      sg_policy->last_freq_update_time = time;
+     sg_policy->need_freq_update = false;
 
      return true;
 }

@@ -164,7 +165,10 @@ static unsigned int get_next_freq(struct
      if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
              return sg_policy->next_freq;
 

kernel/sched/cpufreq_schedutil.c static unsigned int get_next_freq(struct · line 171

Quoted 5 lines

kernel/sched/cpufreq_schedutil.c +4 -1

-     sg_policy->need_freq_update = false;
+     if (sg_policy->need_freq_update)
+             sg_policy->need_freq_update =
+                     cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
+

The behavior here is a bit different from what we did in cpufreq.c. In cpufreq core we are _always_ allowing the call to reach the driver's target() routine, but here we do it only if limits have changed. Wonder if we should have similar behavior here as well ?

Over that the code here can be rewritten a bit like:

        if (sg_policy->need_freq_update)
                sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
        else if (freq == sg_policy->cached_raw_freq)
                return sg_policy->next_freq;

From: Rafael J. Wysocki <[email protected]>

Because sugov_update_next_freq() may skip a frequency update even if the need_freq_update flag has been set for the policy at hand, policy limits updates may not take effect as expected.

For example, if the intel_pstate driver operates in the passive mode with HWP enabled, it needs to update the HWP min and max limits when the policy min and max limits change, respectively, but that may not happen if the target frequency does not change along with the limit at hand. In particular, if the policy min is changed first, causing the target frequency to be adjusted to it, and the policy max limit is changed later to the same value, the HWP max limit will not be updated to follow it as expected, because the target frequency is still equal to the policy min limit and it will not change until that limit is updated.

To address this issue, modify get_next_freq() to clear need_freq_update only if the CPUFREQ_NEED_UPDATE_LIMITS flag is not set for the cpufreq driver in use (and it should be set for all potentially affected drivers) and make sugov_update_next_freq() check need_freq_update and continue when it is set regardless of whether or not the new target frequency is equal to the old one.

Fixes f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode

with HWP enabled")

Reported-by Zhang Rui <[email protected]>

Cc 5.9+ <[email protected]> # 5.9+

Signed-off-by Rafael J. Wysocki <[email protected]>

---

New patch in v2.

---
 kernel/sched/cpufreq_schedutil.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +2 -1

@@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy,
 u64 time,
 				   unsigned int next_freq)
 {
-	if (sg_policy->next_freq == next_freq)
+	if (sg_policy->next_freq == next_freq && !sg_policy-
 >need_freq_update)
 		return false;
 
 	sg_policy->next_freq = next_freq;
 	sg_policy->last_freq_update_time = time;
+	sg_policy->need_freq_update = false;
 

kernel/sched/cpufreq_schedutil.c +4 -1

 	return true;
 }

@@ -164,7 +165,10 @@ static unsigned int get_next_freq(struct
 	if (freq == sg_policy->cached_raw_freq && !sg_policy-
 >need_freq_update)
 		return sg_policy->next_freq;
 
-	sg_policy->need_freq_update = false;
+	if (sg_policy->need_freq_update)
+		sg_policy->need_freq_update =
+			cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_L
 IMITS);
+
 	sg_policy->cached_raw_freq = freq;

return cpufreq_driver_resolve_freq(policy, freq);
}

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +2 -2

@@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
 				   unsigned int next_freq)
 {
-	if (sg_policy->next_freq == next_freq)
+	if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
 		return false;
 
 	sg_policy->next_freq = next_freq;
 	sg_policy->last_freq_update_time = time;
+	sg_policy->need_freq_update = false;
 
 	return true;
 }

@@ -161,10 +162,12 @@ static unsigned int get_next_freq(struct
 
 	freq = map_util_freq(util, freq, max);
 
-	if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)

Quoted 23 lines · Rafael J. Wysocki

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +2 -2

@@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
                                 unsigned int next_freq)
 {
-     if (sg_policy->next_freq == next_freq)
+     if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
              return false;
 
      sg_policy->next_freq = next_freq;
      sg_policy->last_freq_update_time = time;
+     sg_policy->need_freq_update = false;
 
      return true;
 }

@@ -161,10 +162,12 @@ static unsigned int get_next_freq(struct
 
      freq = map_util_freq(util, freq, max);
 
-     if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)

kernel/sched/cpufreq_schedutil.c static unsigned int get_next_freq(struct · line 173

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +4 -1

+     if (cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
+             sg_policy->need_freq_update = true;
+     else if (freq == sg_policy->cached_raw_freq &&
+              !sg_policy->need_freq_update)
              return sg_policy->next_freq;
 
-     sg_policy->need_freq_update = false;
      sg_policy->cached_raw_freq = freq;
      return cpufreq_driver_resolve_freq(policy, freq);
 }

What about just this instead ?

static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
unsigned int next_freq)
{
- if (sg_policy->next_freq == next_freq)
+ if (sg_policy->next_freq == next_freq &&
+ !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

                return false;

        sg_policy->next_freq = next_freq;
        sg_policy->last_freq_update_time = time;

        return true;

}

Quoted 44 lines · Viresh Kumar

Quoted 23 lines · Rafael J. Wysocki

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +2 -2

@@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
                                 unsigned int next_freq)
 {
-     if (sg_policy->next_freq == next_freq)
+     if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
              return false;
 
      sg_policy->next_freq = next_freq;
      sg_policy->last_freq_update_time = time;
+     sg_policy->need_freq_update = false;
 
      return true;
 }

@@ -161,10 +162,12 @@ static unsigned int get_next_freq(struct
 
      freq = map_util_freq(util, freq, max);
 
-     if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)

kernel/sched/cpufreq_schedutil.c static unsigned int get_next_freq(struct · line 173

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +4 -1

+     if (cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
+             sg_policy->need_freq_update = true;
+     else if (freq == sg_policy->cached_raw_freq &&
+              !sg_policy->need_freq_update)
              return sg_policy->next_freq;
 
-     sg_policy->need_freq_update = false;
      sg_policy->cached_raw_freq = freq;
      return cpufreq_driver_resolve_freq(policy, freq);
 }

What about just this instead ?

static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
unsigned int next_freq)
{
- if (sg_policy->next_freq == next_freq)
+ if (sg_policy->next_freq == next_freq &&
+ !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

Quoted 8 lines

                return false;

        sg_policy->next_freq = next_freq;
        sg_policy->last_freq_update_time = time;

        return true;

}

Without any changes in get_next_freq() this is not sufficient, because get_next_freq() may skip the update too.

If the intention is to always let the driver callback run when CPUFREQ_NEED_UPDATE_LIMITS is set, then both get_next_freq() and sugov_update_next_freq() need to be modified.

From: Rafael J. Wysocki <[email protected]>

Because sugov_update_next_freq() may skip a frequency update even if the need_freq_update flag has been set for the policy at hand, policy limits updates may not take effect as expected.

For example, if the intel_pstate driver operates in the passive mode with HWP enabled, it needs to update the HWP min and max limits when the policy min and max limits change, respectively, but that may not happen if the target frequency does not change along with the limit at hand. In particular, if the policy min is changed first, causing the target frequency to be adjusted to it, and the policy max limit is changed later to the same value, the HWP max limit will not be updated to follow it as expected, because the target frequency is still equal to the policy min limit and it will not change until that limit is updated.

To address this issue, modify get_next_freq() to let the driver callback run if the CPUFREQ_NEED_UPDATE_LIMITS cpufreq driver flag is set regardless of whether or not the new frequency to set is equal to the previous one.

Fixes f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode with HWP enabled")

Reported-by Zhang Rui <[email protected]>

Tested-by Zhang Rui <[email protected]>

Cc 5.9+ <[email protected]> # 5.9+

Signed-off-by Rafael J. Wysocki <[email protected]>

---

v2.1 -> v2.2:
* Instead of updating need_freq_update if CPUFREQ_NEED_UPDATE_LIMITS is set in get_next_freq() and checking it again in sugov_update_next_freq(), check CPUFREQ_NEED_UPDATE_LIMITS directly in sugov_update_next_freq().
* Update the subject.

v2 -> v2.1:
* Fix typo in the subject.
* Make get_next_freq() and sugov_update_next_freq() ignore the sg_policy->next_freq == next_freq case when CPUFREQ_NEED_UPDATE_LIMITS is set for the driver.
* Add Tested-by from Rui (this version lets the driver callback run more often than the v2, so the behavior in the Rui's case doesn't change).

---
 kernel/sched/cpufreq_schedutil.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

Quoted 52 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Because sugov_update_next_freq() may skip a frequency update even if the need_freq_update flag has been set for the policy at hand, policy limits updates may not take effect as expected.

For example, if the intel_pstate driver operates in the passive mode with HWP enabled, it needs to update the HWP min and max limits when the policy min and max limits change, respectively, but that may not happen if the target frequency does not change along with the limit at hand. In particular, if the policy min is changed first, causing the target frequency to be adjusted to it, and the policy max limit is changed later to the same value, the HWP max limit will not be updated to follow it as expected, because the target frequency is still equal to the policy min limit and it will not change until that limit is updated.

To address this issue, modify get_next_freq() to let the driver callback run if the CPUFREQ_NEED_UPDATE_LIMITS cpufreq driver flag is set regardless of whether or not the new frequency to set is equal to the previous one.

Fixes f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode with HWP enabled")

Reported-by Zhang Rui <[email protected]>

Tested-by Zhang Rui <[email protected]>

Cc 5.9+ <[email protected]> # 5.9+

Signed-off-by Rafael J. Wysocki <[email protected]>

---

v2.1 -> v2.2:
* Instead of updating need_freq_update if CPUFREQ_NEED_UPDATE_LIMITS is set in get_next_freq() and checking it again in sugov_update_next_freq(), check CPUFREQ_NEED_UPDATE_LIMITS directly in sugov_update_next_freq().
* Update the subject.

v2 -> v2.1:
* Fix typo in the subject.
* Make get_next_freq() and sugov_update_next_freq() ignore the sg_policy->next_freq == next_freq case when CPUFREQ_NEED_UPDATE_LIMITS is set for the driver.
* Add Tested-by from Rui (this version lets the driver callback run more often than the v2, so the behavior in the Rui's case doesn't change).

---
 kernel/sched/cpufreq_schedutil.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c static bool sugov_should_update_freq(str · line 107

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c +2 -1

@@ -102,7 +102,8 @@ static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
                                 unsigned int next_freq)
 {
-     if (sg_policy->next_freq == next_freq)
+     if (sg_policy->next_freq == next_freq &&
+         !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
              return false;

Since sg_policy->next_freq is used elsewhere as well, this is the best we can do here.

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +2 -1

      sg_policy->next_freq = next_freq;

@@ -161,7 +162,8 @@ static unsigned int get_next_freq(struct
 
      freq = map_util_freq(util, freq, max);
 
-     if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+     if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update &&
+         !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
              return sg_policy->next_freq;
 

kernel/sched/cpufreq_schedutil.c static unsigned int get_next_freq(struct · line 169

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +0 -0

      sg_policy->need_freq_update = false;

But I was wondering if instead of this we just do this here:

        if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
                sg_policy->cached_raw_freq = freq;

And so the above check will always fail.

Acked-by Viresh Kumar <[email protected]>