[RFC PATCH 0/4] ACPI: processor: thermal: Recompute the frequency cap when boost changes

[RFC PATCH 0/4] ACPI: processor: thermal: Recompute the frequency cap when boost changes

[Hugo Sales]

thermal_req is a fixed fraction of policy->cpuinfo.max_freq, computed
only when the cooling state is written, so it keeps the boost state of
that moment. The ACPI thermal zone writes step 0 on every resume, and
step 0 caps at cpuinfo.max_freq, so if boost was off at suspend, turning
it on after resume leaves the CPUs at their base clock. Steps 1 to 3 go
stale the same way

Ashton A. sent a one-line fix for the resume case, which stops step 0
from setting a cap. It fixes the resume case on my laptop, and my
Tested-by is in that thread:

  https://lore.kernel.org/all/20261028-acpi-thermal-stale-limit-v1-1-4b0ab3230f01@ashtonantila.com/

That patch is small, easy to backport and enough to fix the biggest
issue. This series applies on its own to bleeding-edge and fixes the
resume case and steps 1 to 3 by recomputing the cap when boost changes,
so please take one or the other, not both

Before d15ce412737a the CPUFREQ_ADJUST notifier recomputed the cap on
every policy update. This series brings that back without letting a
notifier touch the policy:

  1. cpufreq core: a CPUFREQ_UPDATE_CPUINFO policy notifier event, sent
     from policy_set_boost() after ->set_boost(). That covers
     amd-pstate, cppc_cpufreq and the cpufreq_boost_set_sw() drivers
  2. intel_pstate: send it from __intel_pstate_update_max_freq(), since
     no_turbo and HWP capability changes don't go through ->set_boost()
  3. processor_thermal: factor out the thermal_req update
  4. processor_thermal: recompute thermal_req on the new event, from the
     step each request was last set to, under a mutex shared with
     cpufreq_set_cur_state() and the request add and remove

Step 0 still sets a cap here, and patch 4 recomputes it with the other
steps, so the resume case needs no special case

The freq QoS notifiers can't do this. They fire only when the aggregate
limit changes, and while the thermal cap is below both maximums,
turning boost on moves boost_freq_req without moving the aggregate

I also looked at basing the steps on a fixed reference frequency, the
way cpufreq_cooling uses absolute table frequencies. There is no generic
way to read the boost maximum while boost is off (amd-pstate and cppc
keep it in driver data), and with boost off step 1 would often not
throttle at all

Rafael, Viresh, which do you prefer? This series adds a cpufreq event
with one user. If that isn't worth it, I'm happy to drop it and keep my
Tested-by on Ashton's patch

Tested on my laptop (Ryzen 9 5980HS, amd-pstate-epp) with the series
backported to 7.3. With boost off and cooling state 1, cpu0's
scaling_max_freq is 2640800, 80% of 3301000. Turning boost on raises it
to 3879405, 80% of 4849257, and turning boost off brings it back down.
Step 2 follows boost the same way, and at step 0 turning boost on gives
the full 4849257. Toggling boost while writing cooling states on all 16
CPUs left nothing in the kernel log, on a kernel without lockdep

Patch 1 changes Documentation/cpu-freq/core.rst only. Its zh_CN and
zh_TW translations need the same update from someone who can review
them

Hugo Sales (4):
  cpufreq: Add a CPUFREQ_UPDATE_CPUINFO policy notifier event
  cpufreq: intel_pstate: Send CPUFREQ_UPDATE_CPUINFO on max_freq changes
  ACPI: processor: thermal: Factor out the thermal_req update
  ACPI: processor: thermal: Recompute the frequency cap when boost
    changes

 Documentation/cpu-freq/core.rst  |  9 +++--
 drivers/acpi/processor_driver.c  |  2 ++
 drivers/acpi/processor_thermal.c | 58 ++++++++++++++++++++++++++------
 drivers/cpufreq/cpufreq.c        | 16 +++++++++
 drivers/cpufreq/intel_pstate.c   |  8 ++++-
 include/acpi/processor.h         |  4 +++
 include/linux/cpufreq.h          |  2 ++
 7 files changed, 85 insertions(+), 14 deletions(-)


base-commit: 5854d2ce4b0b9ccb37b154319d8283bad889a694
-- 
2.56.0

[RFC PATCH 1/4] cpufreq: Add a CPUFREQ_UPDATE_CPUINFO policy notifier event

[Hugo Sales]

Turning boost on or off changes policy->cpuinfo.max_freq in the
->set_boost() callbacks of amd-pstate, cppc_cpufreq and the drivers that
use cpufreq_boost_set_sw(). Users that derive a QoS request from
cpuinfo.max_freq have no way to learn about it. The ACPI processor
thermal code is one: its throttling steps are fixed fractions of
cpuinfo.max_freq, computed only when the cooling state is written

Its freq QoS notifiers don't help, since they only fire when the
aggregate limit changes. While a thermal cap is lower than both maximums,
turning boost on changes boost_freq_req but not the aggregate

Add a CPUFREQ_UPDATE_CPUINFO policy notifier event and send it from
policy_set_boost() once the driver has updated cpuinfo. The helper that
sends it is global, so a driver that changes cpuinfo.max_freq outside
->set_boost() can send it too

Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 Documentation/cpu-freq/core.rst |  9 ++++++---
 drivers/cpufreq/cpufreq.c       | 16 ++++++++++++++++
 include/linux/cpufreq.h         |  2 ++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/Documentation/cpu-freq/core.rst b/Documentation/cpu-freq/core.rst
index 4ceef8e7217c3..946913c024121 100644
--- a/Documentation/cpu-freq/core.rst
+++ b/Documentation/cpu-freq/core.rst
@@ -49,11 +49,14 @@ transition notifiers.
 2.1 CPUFreq policy notifiers
 ----------------------------
 
-These are notified when a new policy is created or removed.
+These are notified when a new policy is created or removed, and when boost
+is turned on or off or the driver reports a change of cpuinfo.max_freq.
 
 The phase is specified in the second argument to the notifier.  The phase is
-CPUFREQ_CREATE_POLICY when the policy is first created and it is
-CPUFREQ_REMOVE_POLICY when the policy is removed.
+CPUFREQ_CREATE_POLICY when the policy is first created, it is
+CPUFREQ_REMOVE_POLICY when the policy is removed and it is
+CPUFREQ_UPDATE_CPUINFO when boost has been turned on or off or the driver
+reports that cpuinfo.max_freq has changed.
 
 The third argument, a ``void *pointer``, points to a struct cpufreq_policy
 consisting of several values, including min, max (the lower and upper
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 54dde8419bdc4..4d974dac4dff5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -594,6 +594,8 @@ static int policy_set_boost(struct cpufreq_policy *policy, bool enable)
 		return ret;
 	}
 
+	cpufreq_notify_cpuinfo_update(policy);
+
 	return 0;
 }
 
@@ -1184,6 +1186,20 @@ void refresh_frequency_limits(struct cpufreq_policy *policy)
 }
 EXPORT_SYMBOL(refresh_frequency_limits);
 
+/**
+ * cpufreq_notify_cpuinfo_update - Notify policy notifiers of a cpuinfo change
+ * @policy: Policy whose cpuinfo limits have changed
+ *
+ * Send CPUFREQ_UPDATE_CPUINFO to the policy notifiers, so that QoS requests
+ * derived from @policy->cpuinfo can be recomputed. An inactive policy is
+ * notified too, since its QoS requests stay until CPUFREQ_REMOVE_POLICY
+ */
+void cpufreq_notify_cpuinfo_update(struct cpufreq_policy *policy)
+{
+	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+				     CPUFREQ_UPDATE_CPUINFO, policy);
+}
+
 static void handle_update(struct work_struct *work)
 {
 	struct cpufreq_policy *policy =
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d3d0d9d02aa4a..f3acdcee7183f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -249,6 +249,7 @@ void disable_cpufreq(void);
 u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy);
 
 void refresh_frequency_limits(struct cpufreq_policy *policy);
+void cpufreq_notify_cpuinfo_update(struct cpufreq_policy *policy);
 void cpufreq_update_policy(unsigned int cpu);
 void cpufreq_update_limits(unsigned int cpu);
 bool have_governor_per_policy(void);
@@ -536,6 +537,7 @@ static inline void cpufreq_resume(void) {}
 /* Policy Notifiers  */
 #define CPUFREQ_CREATE_POLICY		(0)
 #define CPUFREQ_REMOVE_POLICY		(1)
+#define CPUFREQ_UPDATE_CPUINFO		(2)
 
 #ifdef CONFIG_CPU_FREQ
 int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);

base-commit: 5854d2ce4b0b9ccb37b154319d8283bad889a694
-- 
2.56.0

[RFC PATCH 2/4] cpufreq: intel_pstate: Send CPUFREQ_UPDATE_CPUINFO on max_freq changes

[Hugo Sales]

intel_pstate has no ->set_boost() callback. It changes
policy->cpuinfo.max_freq in __intel_pstate_update_max_freq() when
no_turbo is written and when the HWP capabilities change, so
policy_set_boost() never sends CPUFREQ_UPDATE_CPUINFO for it

Send the event from there when the value changes, before the policy
limits are refreshed

Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 drivers/cpufreq/intel_pstate.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 0e6ae4cdda5de..f9e356409f199 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1429,13 +1429,19 @@ static void intel_pstate_update_policies(void)
 static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
 					   struct cpudata *cpudata)
 {
+	unsigned int max_freq;
+
 	guard(cpufreq_policy_write)(policy);
 
 	if (hwp_active)
 		intel_pstate_get_hwp_cap(cpudata);
 
-	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
-			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
+	max_freq = READ_ONCE(global.no_turbo) ?
+		   cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
+	if (policy->cpuinfo.max_freq != max_freq) {
+		policy->cpuinfo.max_freq = max_freq;
+		cpufreq_notify_cpuinfo_update(policy);
+	}
 
 	refresh_frequency_limits(policy);
 }
-- 
2.56.0

[RFC PATCH 3/4] ACPI: processor: thermal: Factor out the thermal_req update

[Hugo Sales]

Split the cap computation and the thermal_req update out of
cpufreq_update_thermal_limit() into cpufreq_update_thermal_req(), which
takes the policy and the step as arguments. This prepares for
recomputing the cap on CPUFREQ_UPDATE_CPUINFO, where the notifier
passes the policy and the step is stored for each CPU

No functional change

Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 drivers/acpi/processor_thermal.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index 11036f0d2b942..25c4b0ad64e36 100644
--- a/drivers/acpi/processor_thermal.c
+++ b/drivers/acpi/processor_thermal.c
@@ -88,23 +88,30 @@ static int cpufreq_get_cur_state(unsigned int cpu)
 	return reduction_step(cpu);
 }
 
-static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
+static void cpufreq_update_thermal_req(struct cpufreq_policy *policy,
+				       struct acpi_processor *pr,
+				       unsigned int step)
 {
 	unsigned long max_freq;
 	int ret;
 
-	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
-	if (!policy)
-		return false;
-
 	max_freq = (policy->cpuinfo.max_freq *
-		(100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
+		(100 - step * cpufreq_thermal_reduction_pctg)) / 100;
 
 	ret = freq_qos_update_request(&pr->thermal_req, max_freq);
 	if (ret < 0) {
 		pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
 			pr->id, ret);
 	}
+}
+
+static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
+{
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
+	if (!policy)
+		return false;
+
+	cpufreq_update_thermal_req(policy, pr, reduction_step(cpu));
 
 	return true;
 }
-- 
2.56.0

[RFC PATCH 4/4] ACPI: processor: thermal: Recompute the frequency cap when boost changes

[Hugo Sales]

thermal_req is a fixed fraction of policy->cpuinfo.max_freq, computed
only when the cooling state is written, so it keeps the boost state of
that moment. That includes step 0, which caps at cpuinfo.max_freq
itself. The ACPI thermal zone writes step 0 on every resume, so if boost
was off at suspend, turning it on after resume leaves the CPUs at their
base clock

Recompute the cap on CPUFREQ_UPDATE_CPUINFO, as the CPUFREQ_ADJUST
notifier did before d15ce412737a, with the step each CPU's request was
last set to

Fixes: d15ce412737a ("ACPI: cpufreq: Switch to QoS requests instead of cpufreq notifier")
Link: https://lore.kernel.org/all/20261028-acpi-thermal-stale-limit-v1-1-4b0ab3230f01@ashtonantila.com/
Link: https://bugzilla.kernel.org/show_bug.cgi?id=221795
Signed-off-by: Hugo Sales <hugo@hsal.es>
---
This is an alternative to Ashton's one-line fix linked above, which
stops step 0 from setting a cap. Here step 0 still sets one, at
cpuinfo.max_freq, and it is recomputed with the other steps when boost
changes

The step is kept per CPU because reduction_step()'s per-package slot
moves when the first CPU of the package goes offline

A new mutex serializes the recompute with cpufreq_set_cur_state() and
with adding and removing thermal_req. Boost can be toggled while a
policy is being removed, and a cooling state write that read the old
cpuinfo.max_freq must not overwrite the recomputed cap
---
 drivers/acpi/processor_driver.c  |  2 ++
 drivers/acpi/processor_thermal.c | 48 +++++++++++++++++++++++++++++---
 include/acpi/processor.h         |  4 +++
 3 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index bdd1529d79f03..c3929425a88b6 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -221,6 +221,8 @@ static int acpi_processor_notifier(struct notifier_block *nb,
 	} else if (event == CPUFREQ_REMOVE_POLICY) {
 		acpi_processor_ppc_exit(policy);
 		acpi_thermal_cpufreq_exit(policy);
+	} else if (event == CPUFREQ_UPDATE_CPUINFO) {
+		acpi_thermal_cpufreq_update(policy);
 	}
 
 	return 0;
diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index 25c4b0ad64e36..8b1a3cb004571 100644
--- a/drivers/acpi/processor_thermal.c
+++ b/drivers/acpi/processor_thermal.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/cpufreq.h>
 #include <linux/acpi.h>
+#include <linux/mutex.h>
 #include <acpi/processor.h>
 #include <linux/uaccess.h>
 
@@ -40,6 +41,16 @@ static int cpufreq_thermal_max_step __read_mostly = 3;
 static int cpufreq_thermal_reduction_pctg __read_mostly = 20;
 
 static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_step);
+/* The step each CPU's thermal_req was last set to */
+static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_req_step);
+
+/*
+ * Serializes adding, updating and removing thermal_req, and the step saved
+ * with it, so recomputing the cap after a boost change can't race a cooling
+ * state write. It nests inside policy->rwsem and the cooling device lock,
+ * so take neither while holding it
+ */
+static DEFINE_MUTEX(cpufreq_thermal_lock);
 
 #define reduction_step(cpu) \
 	per_cpu(cpufreq_thermal_reduction_step, phys_package_first_cpu(cpu))
@@ -95,6 +106,8 @@ static void cpufreq_update_thermal_req(struct cpufreq_policy *policy,
 	unsigned long max_freq;
 	int ret;
 
+	lockdep_assert_held(&cpufreq_thermal_lock);
+
 	max_freq = (policy->cpuinfo.max_freq *
 		(100 - step * cpufreq_thermal_reduction_pctg)) / 100;
 
@@ -102,7 +115,10 @@ static void cpufreq_update_thermal_req(struct cpufreq_policy *policy,
 	if (ret < 0) {
 		pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
 			pr->id, ret);
+		return;
 	}
+
+	per_cpu(cpufreq_thermal_req_step, pr->id) = step;
 }
 
 static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
@@ -124,6 +140,8 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
 	if (!cpu_has_cpufreq(cpu))
 		return 0;
 
+	guard(mutex)(&cpufreq_thermal_lock);
+
 	reduction_step(cpu) = state;
 
 	/*
@@ -177,9 +195,13 @@ void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy)
 		if (!pr)
 			continue;
 
-		ret = freq_qos_add_request(&policy->constraints,
-					   &pr->thermal_req,
-					   FREQ_QOS_MAX, INT_MAX);
+		scoped_guard(mutex, &cpufreq_thermal_lock) {
+			ret = freq_qos_add_request(&policy->constraints,
+						   &pr->thermal_req,
+						   FREQ_QOS_MAX, INT_MAX);
+			if (ret >= 0)
+				per_cpu(cpufreq_thermal_req_step, cpu) = 0;
+		}
 		if (ret < 0) {
 			pr_err("Failed to add freq constraint for CPU%d (%d)\n",
 			       cpu, ret);
@@ -190,6 +212,23 @@ void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy)
 	}
 }
 
+void acpi_thermal_cpufreq_update(struct cpufreq_policy *policy)
+{
+	unsigned int cpu;
+
+	guard(mutex)(&cpufreq_thermal_lock);
+
+	for_each_cpu(cpu, policy->related_cpus) {
+		struct acpi_processor *pr = per_cpu(processors, cpu);
+
+		if (!pr || !freq_qos_request_active(&pr->thermal_req))
+			continue;
+
+		cpufreq_update_thermal_req(policy, pr,
+					   per_cpu(cpufreq_thermal_req_step, cpu));
+	}
+}
+
 void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy)
 {
 	unsigned int cpu;
@@ -200,7 +239,8 @@ void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy)
 		if (!pr)
 			continue;
 
-		freq_qos_remove_request(&pr->thermal_req);
+		scoped_guard(mutex, &cpufreq_thermal_lock)
+			freq_qos_remove_request(&pr->thermal_req);
 
 		thermal_cooling_device_update(pr->cdev);
 	}
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index b5447af4d40b4..ccd5ac6acb57e 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -432,12 +432,16 @@ void acpi_processor_thermal_exit(struct acpi_processor *pr);
 extern const struct thermal_cooling_device_ops processor_cooling_ops;
 #ifdef CONFIG_CPU_FREQ
 void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy);
+void acpi_thermal_cpufreq_update(struct cpufreq_policy *policy);
 void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy);
 #else
 static inline void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy)
 {
 	return;
 }
+
+static inline void acpi_thermal_cpufreq_update(struct cpufreq_policy *policy) { }
+
 static inline void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy)
 {
 	return;
-- 
2.56.0

Re: [RFC PATCH 2/4] cpufreq: intel_pstate: Send CPUFREQ_UPDATE_CPUINFO on max_freq changes

[Ximing Zhang]

Hi Hugo,

I tested the four patches unchanged on an Intel Core i7-10510U using
intel_pstate in active/HWP mode, on the stated base commit
5854d2ce4b0b9ccb37b154319d8283bad889a694.

Cooling states 0 through 3 followed Turbo changes in both directions.
I also ran 100 CPU 7 offline/online cycles concurrently with Turbo and
cooling-state writes, followed by the same sequential checks, and a
suspend/resume via rtcwake -m mem -s 15. Turbo was disabled before suspend
and enabled afterward. Those checks passed with lockdep enabled and no
new warnings. The test kernel was already tainted by unsigned modules;
that taint did not change during the tests.

There is one remaining Intel hotplug case: changing Turbo while CPU 7
is offline leaves that policy's cpuinfo.max_freq stale when it returns.
I reproduced the same stale CPU information on the stock
7.2.5-1-cachyos kernel, so this behavior predates the RFC.

On the patched kernel, starting with all CPUs online, Turbo enabled and
cooling state 0, I ran the following as root:

    cpu=/sys/devices/system/cpu
    cdev=/sys/class/thermal/cooling_device0
    echo 1 > $cpu/intel_pstate/no_turbo
    echo 1 > $cdev/cur_state
    echo 0 > $cpu/cpu7/online
    echo 0 > $cpu/intel_pstate/no_turbo
    echo 1 > $cpu/cpu7/online
    cat $cpu/cpufreq/policy7/cpuinfo_max_freq
    cat $cpu/cpufreq/policy7/scaling_max_freq

These report 1800000 and 1440000 kHz. The policies for the other CPUs
report 4900000 and 3920000 kHz. With the Turbo transition reversed,
CPU 7 returns with a cpuinfo maximum of 4900000 instead of 1800000,
and an effective cap of 1800000 instead of 1440000 kHz.

I restored the starting state with all CPUs online:

    echo 1 > $cpu/intel_pstate/no_turbo
    echo 0 > $cpu/intel_pstate/no_turbo
    echo 0 > $cdev/cur_state

intel_pstate_update_max_freq() uses cpufreq_cpu_get(), whose raw lookup
rejects an offline CPU. The reused policy then takes
intel_pstate_cpu_online(), which does not refresh cpuinfo.max_freq.
This appears to prevent the new notification from covering that case.

Is this case intended to be covered by this series, or would a separate
intel_pstate follow-up be more appropriate?

Thanks,
Ximing