[PATCH v1 00/10] cpufreq: cpufreq_update_limits() fix and some cleanups

[PATCH v1 00/10] cpufreq: cpufreq_update_limits() fix and some cleanups

[Rafael J. Wysocki]

Hi Everyone,

This series of patches consists of a fix for a possible (and actually happening
in some cases) NULL pointer dereference in cpufreq_update_limits() (patch
[01/10]) and a bunch of tangentially related fixes inspired by it, mostly about
using __free() for cleanup and locking guards.

The first patch is definitely for 6.15, but the rest is rather 6.16 material.

Please refer to the individual patch changelogs for more information.

Thanks!

[PATCH v1 01/10] cpufreq: Reference count policy in cpufreq_update_limits()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since acpi_processor_notify() can be called before registering a cpufreq
driver or even in cases when a cpufreq driver is not registered at all,
cpufreq_update_limits() needs to check if a cpufreq driver is present
and prevent it from being unregistered.

For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
policy pointer for the given CPU and reference count the corresponding
policy object, if present.

Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
Reporetd-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |    6 ++++++
 1 file changed, 6 insertions(+)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2781,6 +2781,12 @@
  */
 void cpufreq_update_limits(unsigned int cpu)
 {
+	struct cpufreq_policy *policy __free(put_cpufreq_policy);
+
+	policy = cpufreq_cpu_get(cpu);
+	if (!policy)
+		return;
+
 	if (cpufreq_driver->update_limits)
 		cpufreq_driver->update_limits(cpu);
 	else

[PATCH v1 02/10] cpufreq: Consolidate some code in cpufreq_online()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that the policy->cpu update in cpufreq_policy_alloc() can be
moved to cpufreq_online() and then it can be carried out under the
policy rwsem, along with the clearing of policy->governor (unnecessary
in the "new policy" code branch, but also not harmful).  If this is
done, the bottom parts of the "if (policy)" branches become identical
and they can be collapsed and moved below the conditional.

Modify the code accordingly which makes it somewhat easier to follow.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1306,7 +1306,6 @@
 	init_waitqueue_head(&policy->transition_wait);
 	INIT_WORK(&policy->update, handle_update);
 
-	policy->cpu = cpu;
 	return policy;
 
 err_min_qos_notifier:
@@ -1394,17 +1393,18 @@
 
 		/* This is the only online CPU for the policy.  Start over. */
 		new_policy = false;
-		down_write(&policy->rwsem);
-		policy->cpu = cpu;
-		policy->governor = NULL;
 	} else {
 		new_policy = true;
 		policy = cpufreq_policy_alloc(cpu);
 		if (!policy)
 			return -ENOMEM;
-		down_write(&policy->rwsem);
 	}
 
+	down_write(&policy->rwsem);
+
+	policy->cpu = cpu;
+	policy->governor = NULL;
+
 	if (!new_policy && cpufreq_driver->online) {
 		/* Recover policy->cpus using related_cpus */
 		cpumask_copy(policy->cpus, policy->related_cpus);

[PATCH v1 03/10] cpufreq: Split cpufreq_online()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In preparation for the introduction of cpufreq policy locking guards,
move the part of cpufreq_online() that is carried out under the policy
rwsem into a separate function called cpufreq_policy_online().

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   95 ++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 41 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1374,32 +1374,13 @@
 	kfree(policy);
 }
 
-static int cpufreq_online(unsigned int cpu)
+static int cpufreq_policy_online(struct cpufreq_policy *policy,
+				 unsigned int cpu, bool new_policy)
 {
-	struct cpufreq_policy *policy;
-	bool new_policy;
 	unsigned long flags;
 	unsigned int j;
 	int ret;
 
-	pr_debug("%s: bringing CPU%u online\n", __func__, cpu);
-
-	/* Check if this CPU already has a policy to manage it */
-	policy = per_cpu(cpufreq_cpu_data, cpu);
-	if (policy) {
-		WARN_ON(!cpumask_test_cpu(cpu, policy->related_cpus));
-		if (!policy_is_inactive(policy))
-			return cpufreq_add_policy_cpu(policy, cpu);
-
-		/* This is the only online CPU for the policy.  Start over. */
-		new_policy = false;
-	} else {
-		new_policy = true;
-		policy = cpufreq_policy_alloc(cpu);
-		if (!policy)
-			return -ENOMEM;
-	}
-
 	down_write(&policy->rwsem);
 
 	policy->cpu = cpu;
@@ -1426,7 +1407,7 @@
 		if (ret) {
 			pr_debug("%s: %d: initialization failed\n", __func__,
 				 __LINE__);
-			goto out_free_policy;
+			goto out_clear_policy;
 		}
 
 		/*
@@ -1577,8 +1558,59 @@
 		goto out_destroy_policy;
 	}
 
+out_unlock:
 	up_write(&policy->rwsem);
 
+	return ret;
+
+out_destroy_policy:
+	for_each_cpu(j, policy->real_cpus)
+		remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
+
+out_offline_policy:
+	if (cpufreq_driver->offline)
+		cpufreq_driver->offline(policy);
+
+out_exit_policy:
+	if (cpufreq_driver->exit)
+		cpufreq_driver->exit(policy);
+
+out_clear_policy:
+	cpumask_clear(policy->cpus);
+
+	goto out_unlock;
+}
+
+static int cpufreq_online(unsigned int cpu)
+{
+	struct cpufreq_policy *policy;
+	bool new_policy;
+	int ret;
+
+	pr_debug("%s: bringing CPU%u online\n", __func__, cpu);
+
+	/* Check if this CPU already has a policy to manage it */
+	policy = per_cpu(cpufreq_cpu_data, cpu);
+	if (policy) {
+		WARN_ON(!cpumask_test_cpu(cpu, policy->related_cpus));
+		if (!policy_is_inactive(policy))
+			return cpufreq_add_policy_cpu(policy, cpu);
+
+		/* This is the only online CPU for the policy.  Start over. */
+		new_policy = false;
+	} else {
+		new_policy = true;
+		policy = cpufreq_policy_alloc(cpu);
+		if (!policy)
+			return -ENOMEM;
+	}
+
+	ret = cpufreq_policy_online(policy, cpu, new_policy);
+	if (ret) {
+		cpufreq_policy_free(policy);
+		return ret;
+	}
+
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
 
 	/* Callback for handling stuff after policy is ready */
@@ -1605,25 +1637,6 @@
 	pr_debug("initialization complete\n");
 
 	return 0;
-
-out_destroy_policy:
-	for_each_cpu(j, policy->real_cpus)
-		remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
-
-out_offline_policy:
-	if (cpufreq_driver->offline)
-		cpufreq_driver->offline(policy);
-
-out_exit_policy:
-	if (cpufreq_driver->exit)
-		cpufreq_driver->exit(policy);
-
-out_free_policy:
-	cpumask_clear(policy->cpus);
-	up_write(&policy->rwsem);
-
-	cpufreq_policy_free(policy);
-	return ret;
 }
 
 /**

[PATCH v1 04/10] cpufreq: Add and use cpufreq policy locking guards

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce "read" and "write" locking guards for cpufreq policies and use
them where applicable in the cpufreq core.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |  121 ++++++++++++++++++++--------------------------
 include/linux/cpufreq.h   |    6 ++
 2 files changed, 60 insertions(+), 67 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -987,17 +987,16 @@
 {
 	struct cpufreq_policy *policy = to_policy(kobj);
 	struct freq_attr *fattr = to_attr(attr);
-	ssize_t ret = -EBUSY;
 
 	if (!fattr->show)
 		return -EIO;
 
-	down_read(&policy->rwsem);
+	guard(cpufreq_policy_read)(policy);
+
 	if (likely(!policy_is_inactive(policy)))
-		ret = fattr->show(policy, buf);
-	up_read(&policy->rwsem);
+		return fattr->show(policy, buf);
 
-	return ret;
+	return -EBUSY;
 }
 
 static ssize_t store(struct kobject *kobj, struct attribute *attr,
@@ -1005,17 +1004,16 @@
 {
 	struct cpufreq_policy *policy = to_policy(kobj);
 	struct freq_attr *fattr = to_attr(attr);
-	ssize_t ret = -EBUSY;
 
 	if (!fattr->store)
 		return -EIO;
 
-	down_write(&policy->rwsem);
+	guard(cpufreq_policy_write)(policy);
+
 	if (likely(!policy_is_inactive(policy)))
-		ret = fattr->store(policy, buf, count);
-	up_write(&policy->rwsem);
+		return fattr->store(policy, buf, count);
 
-	return ret;
+	return -EBUSY;
 }
 
 static void cpufreq_sysfs_release(struct kobject *kobj)
@@ -1167,7 +1165,8 @@
 	if (cpumask_test_cpu(cpu, policy->cpus))
 		return 0;
 
-	down_write(&policy->rwsem);
+	guard(cpufreq_policy_write)(policy);
+
 	if (has_target())
 		cpufreq_stop_governor(policy);
 
@@ -1178,7 +1177,7 @@
 		if (ret)
 			pr_err("%s: Failed to start governor\n", __func__);
 	}
-	up_write(&policy->rwsem);
+
 	return ret;
 }
 
@@ -1198,9 +1197,10 @@
 		container_of(work, struct cpufreq_policy, update);
 
 	pr_debug("handle_update for cpu %u called\n", policy->cpu);
-	down_write(&policy->rwsem);
+
+	guard(cpufreq_policy_write)(policy);
+
 	refresh_frequency_limits(policy);
-	up_write(&policy->rwsem);
 }
 
 static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq,
@@ -1226,11 +1226,11 @@
 	struct kobject *kobj;
 	struct completion *cmp;
 
-	down_write(&policy->rwsem);
-	cpufreq_stats_free_table(policy);
-	kobj = &policy->kobj;
-	cmp = &policy->kobj_unregister;
-	up_write(&policy->rwsem);
+	scoped_guard(cpufreq_policy_write, policy) {
+		cpufreq_stats_free_table(policy);
+		kobj = &policy->kobj;
+		cmp = &policy->kobj_unregister;
+	}
 	kobject_put(kobj);
 
 	/*
@@ -1381,7 +1381,7 @@
 	unsigned int j;
 	int ret;
 
-	down_write(&policy->rwsem);
+	guard(cpufreq_policy_write)(policy);
 
 	policy->cpu = cpu;
 	policy->governor = NULL;
@@ -1558,10 +1558,7 @@
 		goto out_destroy_policy;
 	}
 
-out_unlock:
-	up_write(&policy->rwsem);
-
-	return ret;
+	return 0;
 
 out_destroy_policy:
 	for_each_cpu(j, policy->real_cpus)
@@ -1578,7 +1575,7 @@
 out_clear_policy:
 	cpumask_clear(policy->cpus);
 
-	goto out_unlock;
+	return ret;
 }
 
 static int cpufreq_online(unsigned int cpu)
@@ -1726,11 +1723,10 @@
 		return 0;
 	}
 
-	down_write(&policy->rwsem);
+	guard(cpufreq_policy_write)(policy);
 
 	__cpufreq_offline(cpu, policy);
 
-	up_write(&policy->rwsem);
 	return 0;
 }
 
@@ -1747,33 +1743,29 @@
 	if (!policy)
 		return;
 
-	down_write(&policy->rwsem);
+	scoped_guard(cpufreq_policy_write, policy) {
+		if (cpu_online(cpu))
+			__cpufreq_offline(cpu, policy);
 
-	if (cpu_online(cpu))
-		__cpufreq_offline(cpu, policy);
+		remove_cpu_dev_symlink(policy, cpu, dev);
 
-	remove_cpu_dev_symlink(policy, cpu, dev);
+		if (!cpumask_empty(policy->real_cpus))
+			return;
 
-	if (!cpumask_empty(policy->real_cpus)) {
-		up_write(&policy->rwsem);
-		return;
-	}
+		/*
+		 * Unregister cpufreq cooling once all the CPUs of the policy
+		 * are removed.
+		 */
+		if (cpufreq_thermal_control_enabled(cpufreq_driver)) {
+			cpufreq_cooling_unregister(policy->cdev);
+			policy->cdev = NULL;
+		}
 
-	/*
-	 * Unregister cpufreq cooling once all the CPUs of the policy are
-	 * removed.
-	 */
-	if (cpufreq_thermal_control_enabled(cpufreq_driver)) {
-		cpufreq_cooling_unregister(policy->cdev);
-		policy->cdev = NULL;
+		/* We did light-weight exit earlier, do full tear down now */
+		if (cpufreq_driver->offline && cpufreq_driver->exit)
+			cpufreq_driver->exit(policy);
 	}
 
-	/* We did light-weight exit earlier, do full tear down now */
-	if (cpufreq_driver->offline && cpufreq_driver->exit)
-		cpufreq_driver->exit(policy);
-
-	up_write(&policy->rwsem);
-
 	cpufreq_policy_free(policy);
 }
 
@@ -1926,15 +1918,16 @@
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 	unsigned int ret_freq = 0;
 
-	if (policy) {
-		down_read(&policy->rwsem);
+	if (!policy)
+		return 0;
+
+	scoped_guard(cpufreq_policy_read, policy) {
 		if (cpufreq_driver->get)
 			ret_freq = __cpufreq_get(policy);
-		up_read(&policy->rwsem);
-
-		cpufreq_cpu_put(policy);
 	}
 
+	cpufreq_cpu_put(policy);
+
 	return ret_freq;
 }
 EXPORT_SYMBOL(cpufreq_get);
@@ -1994,9 +1987,9 @@
 
 	for_each_active_policy(policy) {
 		if (has_target()) {
-			down_write(&policy->rwsem);
-			cpufreq_stop_governor(policy);
-			up_write(&policy->rwsem);
+			scoped_guard(cpufreq_policy_write, policy) {
+				cpufreq_stop_governor(policy);
+			}
 		}
 
 		if (cpufreq_driver->suspend && cpufreq_driver->suspend(policy))
@@ -2037,9 +2030,9 @@
 			pr_err("%s: Failed to resume driver: %s\n", __func__,
 				cpufreq_driver->name);
 		} else if (has_target()) {
-			down_write(&policy->rwsem);
-			ret = cpufreq_start_governor(policy);
-			up_write(&policy->rwsem);
+			scoped_guard(cpufreq_policy_write, policy) {
+				ret = cpufreq_start_governor(policy);
+			}
 
 			if (ret)
 				pr_err("%s: Failed to start governor for CPU%u's policy\n",
@@ -2406,15 +2399,9 @@
 			  unsigned int target_freq,
 			  unsigned int relation)
 {
-	int ret;
+	guard(cpufreq_policy_write)(policy);
 
-	down_write(&policy->rwsem);
-
-	ret = __cpufreq_driver_target(policy, target_freq, relation);
-
-	up_write(&policy->rwsem);
-
-	return ret;
+	return __cpufreq_driver_target(policy, target_freq, relation);
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_target);
 
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -170,6 +170,12 @@
 	struct notifier_block nb_max;
 };
 
+DEFINE_GUARD(cpufreq_policy_write, struct cpufreq_policy *,
+	     down_write(&_T->rwsem), up_write(&_T->rwsem))
+
+DEFINE_GUARD(cpufreq_policy_read, struct cpufreq_policy *,
+	     down_read(&_T->rwsem), up_read(&_T->rwsem))
+
 /*
  * Used for passing new cpufreq policy data to the cpufreq driver's ->verify()
  * callback for sanitization.  That callback is only expected to modify the min

[PATCH v1 05/10] cpufreq: intel_pstate: Rearrange max frequency updates handling code

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rename __intel_pstate_update_max_freq() to intel_pstate_update_max_freq()
and move the cpufreq policy reference counting and locking into it (and
implement the locking with the recently introduced cpufreq policy "write"
locking guard).

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   52 +++++++++++++----------------------------
 1 file changed, 17 insertions(+), 35 deletions(-)

--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1353,9 +1353,16 @@
 		cpufreq_update_policy(cpu);
 }
 
-static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
-					   struct cpufreq_policy *policy)
+static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
 {
+	struct cpufreq_policy *policy __free(put_cpufreq_policy);
+
+	policy = cpufreq_cpu_get(cpudata->cpu);
+	if (!policy)
+		return false;
+
+	guard(cpufreq_policy_write)(policy);
+
 	if (hwp_active)
 		intel_pstate_get_hwp_cap(cpudata);
 
@@ -1363,44 +1370,24 @@
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
 
 	refresh_frequency_limits(policy);
+
+	return true;
 }
 
 static void intel_pstate_update_limits(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
-	struct cpudata *cpudata;
-
-	if (!policy)
-		return;
-
-	cpudata = all_cpu_data[cpu];
-
-	__intel_pstate_update_max_freq(cpudata, policy);
-
-	/* Prevent the driver from being unregistered now. */
-	mutex_lock(&intel_pstate_driver_lock);
+	struct cpudata *cpudata = all_cpu_data[cpu];
 
-	cpufreq_cpu_release(policy);
-
-	hybrid_update_capacity(cpudata);
-
-	mutex_unlock(&intel_pstate_driver_lock);
+	if (intel_pstate_update_max_freq(cpudata))
+		hybrid_update_capacity(cpudata);
 }
 
 static void intel_pstate_update_limits_for_all(void)
 {
 	int cpu;
 
-	for_each_possible_cpu(cpu) {
-		struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
-
-		if (!policy)
-			continue;
-
-		__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
-
-		cpufreq_cpu_release(policy);
-	}
+	for_each_possible_cpu(cpu)
+		intel_pstate_update_max_freq(all_cpu_data[cpu]);
 
 	mutex_lock(&hybrid_capacity_lock);
 
@@ -1840,13 +1827,8 @@
 {
 	struct cpudata *cpudata =
 		container_of(to_delayed_work(work), struct cpudata, hwp_notify_work);
-	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata->cpu);
-
-	if (policy) {
-		__intel_pstate_update_max_freq(cpudata, policy);
-
-		cpufreq_cpu_release(policy);
 
+	if (intel_pstate_update_max_freq(cpudata)) {
 		/*
 		 * The driver will not be unregistered while this function is
 		 * running, so update the capacity without acquiring the driver

[PATCH v1 06/10] cpufreq: Use locking guard and __free() in cpufreq_update_policy()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Instead of using cpufreq_cpu_acquire() and cpufreq_cpu_release() in
cpufreq_update_policy(), which is the last user of these functions,
make it use __free() for policy reference counting cleanup and the
"write" locking guard for policy locking.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2752,23 +2752,23 @@
  */
 void cpufreq_update_policy(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
+	struct cpufreq_policy *policy __free(put_cpufreq_policy);
 
+	policy = cpufreq_cpu_get(cpu);
 	if (!policy)
 		return;
 
+	guard(cpufreq_policy_write)(policy);
+
 	/*
 	 * BIOS might change freq behind our back
 	 * -> ask driver for current freq and notify governors about a change
 	 */
 	if (cpufreq_driver->get && has_target() &&
 	    (cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
-		goto unlock;
+		return;
 
 	refresh_frequency_limits(policy);
-
-unlock:
-	cpufreq_cpu_release(policy);
 }
 EXPORT_SYMBOL(cpufreq_update_policy);
 

[PATCH v1 07/10] cpufreq: Drop cpufreq_cpu_acquire() and cpufreq_cpu_release()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since cpufreq_cpu_acquire() and cpufreq_cpu_release() have no more
users in the tree, remove them.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   45 ---------------------------------------------
 include/linux/cpufreq.h   |    2 --
 2 files changed, 47 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -255,51 +255,6 @@
 }
 EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
 
-/**
- * cpufreq_cpu_release - Unlock a policy and decrement its usage counter.
- * @policy: cpufreq policy returned by cpufreq_cpu_acquire().
- */
-void cpufreq_cpu_release(struct cpufreq_policy *policy)
-{
-	if (WARN_ON(!policy))
-		return;
-
-	lockdep_assert_held(&policy->rwsem);
-
-	up_write(&policy->rwsem);
-
-	cpufreq_cpu_put(policy);
-}
-
-/**
- * cpufreq_cpu_acquire - Find policy for a CPU, mark it as busy and lock it.
- * @cpu: CPU to find the policy for.
- *
- * Call cpufreq_cpu_get() to get a reference on the cpufreq policy for @cpu and
- * if the policy returned by it is not NULL, acquire its rwsem for writing.
- * Return the policy if it is active or release it and return NULL otherwise.
- *
- * The policy returned by this function has to be released with the help of
- * cpufreq_cpu_release() in order to release its rwsem and balance its usage
- * counter properly.
- */
-struct cpufreq_policy *cpufreq_cpu_acquire(unsigned int cpu)
-{
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-
-	if (!policy)
-		return NULL;
-
-	down_write(&policy->rwsem);
-
-	if (policy_is_inactive(policy)) {
-		cpufreq_cpu_release(policy);
-		return NULL;
-	}
-
-	return policy;
-}
-
 /*********************************************************************
  *            EXTERNALLY AFFECTING FREQUENCY CHANGES                 *
  *********************************************************************/
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -241,8 +241,6 @@
 
 u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy);
 
-struct cpufreq_policy *cpufreq_cpu_acquire(unsigned int cpu);
-void cpufreq_cpu_release(struct cpufreq_policy *policy);
 int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu);
 void refresh_frequency_limits(struct cpufreq_policy *policy);
 void cpufreq_update_policy(unsigned int cpu);

[PATCH v1 08/10] cpufreq: Use __free() for policy reference counting cleanup

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Use __free() for policy reference counting cleanup where applicable in
the cpufreq core.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   57 ++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1790,27 +1790,26 @@
  */
 unsigned int cpufreq_quick_get(unsigned int cpu)
 {
-	struct cpufreq_policy *policy;
-	unsigned int ret_freq = 0;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
 	unsigned long flags;
 
 	read_lock_irqsave(&cpufreq_driver_lock, flags);
 
 	if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) {
-		ret_freq = cpufreq_driver->get(cpu);
+		unsigned int ret_freq = cpufreq_driver->get(cpu);
+
 		read_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 		return ret_freq;
 	}
 
 	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	policy = cpufreq_cpu_get(cpu);
-	if (policy) {
-		ret_freq = policy->cur;
-		cpufreq_cpu_put(policy);
-	}
+	if (policy)
+		return policy->cur;
 
-	return ret_freq;
+	return 0;
 }
 EXPORT_SYMBOL(cpufreq_quick_get);
 
@@ -1822,15 +1821,13 @@
  */
 unsigned int cpufreq_quick_get_max(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-	unsigned int ret_freq = 0;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy);
 
-	if (policy) {
-		ret_freq = policy->max;
-		cpufreq_cpu_put(policy);
-	}
+	policy = cpufreq_cpu_get(cpu);
+	if (policy)
+		return policy->max;
 
-	return ret_freq;
+	return 0;
 }
 EXPORT_SYMBOL(cpufreq_quick_get_max);
 
@@ -1842,15 +1839,13 @@
  */
 __weak unsigned int cpufreq_get_hw_max_freq(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-	unsigned int ret_freq = 0;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy);
 
-	if (policy) {
-		ret_freq = policy->cpuinfo.max_freq;
-		cpufreq_cpu_put(policy);
-	}
+	policy = cpufreq_cpu_get(cpu);
+	if (policy)
+		return policy->cpuinfo.max_freq;
 
-	return ret_freq;
+	return 0;
 }
 EXPORT_SYMBOL(cpufreq_get_hw_max_freq);
 
@@ -1870,20 +1865,18 @@
  */
 unsigned int cpufreq_get(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-	unsigned int ret_freq = 0;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy);
 
+	policy = cpufreq_cpu_get(cpu);
 	if (!policy)
 		return 0;
 
-	scoped_guard(cpufreq_policy_read, policy) {
-		if (cpufreq_driver->get)
-			ret_freq = __cpufreq_get(policy);
-	}
+	guard(cpufreq_policy_read)(policy);
 
-	cpufreq_cpu_put(policy);
+	if (cpufreq_driver->get)
+		return __cpufreq_get(policy);
 
-	return ret_freq;
+	return 0;
 }
 EXPORT_SYMBOL(cpufreq_get);
 
@@ -2538,7 +2531,8 @@
  */
 int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu)
 {
-	struct cpufreq_policy *cpu_policy;
+	struct cpufreq_policy *cpu_policy __free(put_cpufreq_policy);
+
 	if (!policy)
 		return -EINVAL;
 
@@ -2548,7 +2542,6 @@
 
 	memcpy(policy, cpu_policy, sizeof(*policy));
 
-	cpufreq_cpu_put(cpu_policy);
 	return 0;
 }
 EXPORT_SYMBOL(cpufreq_get_policy);

[PATCH v1 09/10] cpufreq: Introduce cpufreq_policy_refresh()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since cpufreq_update_limits() obtains a cpufreq policy pointer for the
given CPU and reference counts the object pointed to by it, calling
cpufreq_update_policy() from cpufreq_update_limits() is somewhat
wasteful because that function calls cpufreq_cpu_get() on the same
CPU again.

To avoid that unnecessary overhead, move the part of the code running
under the policy rwsem from cpufreq_update_policy() to a new function
called cpufreq_policy_refresh() and invoke that new function from
both cpufreq_update_policy() and cpufreq_update_limits().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2689,6 +2689,21 @@
 	return ret;
 }
 
+static void cpufreq_policy_refresh(struct cpufreq_policy *policy)
+{
+	guard(cpufreq_policy_write)(policy);
+
+	/*
+	 * BIOS might change freq behind our back
+	 * -> ask driver for current freq and notify governors about a change
+	 */
+	if (cpufreq_driver->get && has_target() &&
+	    (cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
+		return;
+
+	refresh_frequency_limits(policy);
+}
+
 /**
  * cpufreq_update_policy - Re-evaluate an existing cpufreq policy.
  * @cpu: CPU to re-evaluate the policy for.
@@ -2706,17 +2721,7 @@
 	if (!policy)
 		return;
 
-	guard(cpufreq_policy_write)(policy);
-
-	/*
-	 * BIOS might change freq behind our back
-	 * -> ask driver for current freq and notify governors about a change
-	 */
-	if (cpufreq_driver->get && has_target() &&
-	    (cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
-		return;
-
-	refresh_frequency_limits(policy);
+	cpufreq_policy_refresh(policy);
 }
 EXPORT_SYMBOL(cpufreq_update_policy);
 
@@ -2725,7 +2730,7 @@
  * @cpu: CPU to update the policy limits for.
  *
  * Invoke the driver's ->update_limits callback if present or call
- * cpufreq_update_policy() for @cpu.
+ * cpufreq_policy_refresh() for @cpu.
  */
 void cpufreq_update_limits(unsigned int cpu)
 {
@@ -2738,7 +2743,7 @@
 	if (cpufreq_driver->update_limits)
 		cpufreq_driver->update_limits(cpu);
 	else
-		cpufreq_update_policy(cpu);
+		cpufreq_policy_refresh(policy);
 }
 EXPORT_SYMBOL_GPL(cpufreq_update_limits);
 

[PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since cpufreq_update_limits() obtains a cpufreq policy pointer for the
given CPU and reference counts the corresponding policy object, it may
as well pass the policy pointer to the cpufreq driver's ->update_limits()
callback which allows that callback to avoid invoking cpufreq_cpu_get()
for the same CPU.

Accordingly, redefine ->update_limits() to take a policy pointer instead
of a CPU number and update both drivers implementing it, intel_pstate
and amd-pstate, as needed.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/amd-pstate.c   |    7 ++-----
 drivers/cpufreq/cpufreq.c      |    2 +-
 drivers/cpufreq/intel_pstate.c |   29 ++++++++++++++++++-----------
 include/linux/cpufreq.h        |    2 +-
 4 files changed, 22 insertions(+), 18 deletions(-)

--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -821,19 +821,16 @@
 	schedule_work(&sched_prefcore_work);
 }
 
-static void amd_pstate_update_limits(unsigned int cpu)
+static void amd_pstate_update_limits(struct cpufreq_policy *policy)
 {
-	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
 	struct amd_cpudata *cpudata;
 	u32 prev_high = 0, cur_high = 0;
 	bool highest_perf_changed = false;
+	unsigned int cpu = policy->cpu;
 
 	if (!amd_pstate_prefcore)
 		return;
 
-	if (!policy)
-		return;
-
 	if (amd_get_highest_perf(cpu, &cur_high))
 		return;
 
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2741,7 +2741,7 @@
 		return;
 
 	if (cpufreq_driver->update_limits)
-		cpufreq_driver->update_limits(cpu);
+		cpufreq_driver->update_limits(policy);
 	else
 		cpufreq_policy_refresh(policy);
 }
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1353,14 +1353,9 @@
 		cpufreq_update_policy(cpu);
 }
 
-static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
+static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
+					   struct cpudata *cpudata)
 {
-	struct cpufreq_policy *policy __free(put_cpufreq_policy);
-
-	policy = cpufreq_cpu_get(cpudata->cpu);
-	if (!policy)
-		return false;
-
 	guard(cpufreq_policy_write)(policy);
 
 	if (hwp_active)
@@ -1370,16 +1365,28 @@
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
 
 	refresh_frequency_limits(policy);
+}
+
+static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
+{
+	struct cpufreq_policy *policy __free(put_cpufreq_policy);
+
+	policy = cpufreq_cpu_get(cpudata->cpu);
+	if (!policy)
+		return false;
+
+	__intel_pstate_update_max_freq(policy, cpudata);
 
 	return true;
 }
 
-static void intel_pstate_update_limits(unsigned int cpu)
+static void intel_pstate_update_limits(struct cpufreq_policy *policy)
 {
-	struct cpudata *cpudata = all_cpu_data[cpu];
+	struct cpudata *cpudata = all_cpu_data[policy->cpu];
+
+	__intel_pstate_update_max_freq(policy, cpudata);
 
-	if (intel_pstate_update_max_freq(cpudata))
-		hybrid_update_capacity(cpudata);
+	hybrid_update_capacity(cpudata);
 }
 
 static void intel_pstate_update_limits_for_all(void)
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -399,7 +399,7 @@
 	unsigned int	(*get)(unsigned int cpu);
 
 	/* Called to update policy limits on firmware notifications. */
-	void		(*update_limits)(unsigned int cpu);
+	void		(*update_limits)(struct cpufreq_policy *policy);
 
 	/* optional */
 	int		(*bios_limit)(int cpu, unsigned int *limit);

Re: [PATCH v1 01/10] cpufreq: Reference count policy in cpufreq_update_limits()

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

On Fri, Mar 28, 2025 at 09:39:08PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since acpi_processor_notify() can be called before registering a cpufreq
> driver or even in cases when a cpufreq driver is not registered at all,
> cpufreq_update_limits() needs to check if a cpufreq driver is present
> and prevent it from being unregistered.
> 
> For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
> policy pointer for the given CPU and reference count the corresponding
> policy object, if present.
> 
> Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
> Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
> Reporetd-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reported

I wanted to propose also Tested-by tag, but technically it's not me who
tested it: https://forum.qubes-os.org/t/kernel-latest-6-13-6-boot-loop/32926/18

> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2781,6 +2781,12 @@
>   */
>  void cpufreq_update_limits(unsigned int cpu)
>  {
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy);
> +
> +	policy = cpufreq_cpu_get(cpu);
> +	if (!policy)
> +		return;
> +
>  	if (cpufreq_driver->update_limits)
>  		cpufreq_driver->update_limits(cpu);
>  	else
> 
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 01/10] cpufreq: Reference count policy in cpufreq_update_limits()

[Rafael J. Wysocki]

On Sat, Mar 29, 2025 at 3:02 AM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Fri, Mar 28, 2025 at 09:39:08PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Since acpi_processor_notify() can be called before registering a cpufreq
> > driver or even in cases when a cpufreq driver is not registered at all,
> > cpufreq_update_limits() needs to check if a cpufreq driver is present
> > and prevent it from being unregistered.
> >
> > For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
> > policy pointer for the given CPU and reference count the corresponding
> > policy object, if present.
> >
> > Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
> > Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
> > Reporetd-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>
> Reported

Right, thanks!

> I wanted to propose also Tested-by tag, but technically it's not me who
> tested it: https://forum.qubes-os.org/t/kernel-latest-6-13-6-boot-loop/32926/18

You can ask the original tester whether or not they would be willing
to give a tag, though.

> > Cc: All applicable <stable@vger.kernel.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2781,6 +2781,12 @@
> >   */
> >  void cpufreq_update_limits(unsigned int cpu)
> >  {
> > +     struct cpufreq_policy *policy __free(put_cpufreq_policy);
> > +
> > +     policy = cpufreq_cpu_get(cpu);
> > +     if (!policy)
> > +             return;
> > +
> >       if (cpufreq_driver->update_limits)
> >               cpufreq_driver->update_limits(cpu);
> >       else
> >
> >
> >
>
> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab

Re: [PATCH v1 01/10] cpufreq: Reference count policy in cpufreq_update_limits()

[Viresh Kumar]

On 28-03-25, 21:39, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since acpi_processor_notify() can be called before registering a cpufreq
> driver or even in cases when a cpufreq driver is not registered at all,
> cpufreq_update_limits() needs to check if a cpufreq driver is present
> and prevent it from being unregistered.
> 
> For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
> policy pointer for the given CPU and reference count the corresponding
> policy object, if present.
> 
> Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
> Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
> Reporetd-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2781,6 +2781,12 @@
>   */
>  void cpufreq_update_limits(unsigned int cpu)
>  {
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy);
> +
> +	policy = cpufreq_cpu_get(cpu);
> +	if (!policy)
> +		return;
> +
>  	if (cpufreq_driver->update_limits)
>  		cpufreq_driver->update_limits(cpu);
>  	else

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 02/10] cpufreq: Consolidate some code in cpufreq_online()

[Viresh Kumar]

On 28-03-25, 21:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that the policy->cpu update in cpufreq_policy_alloc() can be
> moved to cpufreq_online() and then it can be carried out under the
> policy rwsem, along with the clearing of policy->governor (unnecessary
> in the "new policy" code branch, but also not harmful).  If this is
> done, the bottom parts of the "if (policy)" branches become identical
> and they can be collapsed and moved below the conditional.
> 
> Modify the code accordingly which makes it somewhat easier to follow.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1306,7 +1306,6 @@
>  	init_waitqueue_head(&policy->transition_wait);
>  	INIT_WORK(&policy->update, handle_update);
>  
> -	policy->cpu = cpu;
>  	return policy;
>  
>  err_min_qos_notifier:
> @@ -1394,17 +1393,18 @@
>  
>  		/* This is the only online CPU for the policy.  Start over. */
>  		new_policy = false;
> -		down_write(&policy->rwsem);
> -		policy->cpu = cpu;
> -		policy->governor = NULL;
>  	} else {
>  		new_policy = true;
>  		policy = cpufreq_policy_alloc(cpu);
>  		if (!policy)
>  			return -ENOMEM;
> -		down_write(&policy->rwsem);
>  	}
>  
> +	down_write(&policy->rwsem);
> +
> +	policy->cpu = cpu;
> +	policy->governor = NULL;
> +
>  	if (!new_policy && cpufreq_driver->online) {
>  		/* Recover policy->cpus using related_cpus */
>  		cpumask_copy(policy->cpus, policy->related_cpus);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 03/10] cpufreq: Split cpufreq_online()

[Viresh Kumar]

On 28-03-25, 21:41, Rafael J. Wysocki wrote:
>  
> +out_unlock:
>  	up_write(&policy->rwsem);
>  
> +	return ret;
> +
> +out_destroy_policy:
> +	for_each_cpu(j, policy->real_cpus)
> +		remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
> +
> +out_offline_policy:
> +	if (cpufreq_driver->offline)
> +		cpufreq_driver->offline(policy);
> +
> +out_exit_policy:
> +	if (cpufreq_driver->exit)
> +		cpufreq_driver->exit(policy);
> +
> +out_clear_policy:
> +	cpumask_clear(policy->cpus);
> +
> +	goto out_unlock;

Instead of jumping back to the function, won't declaring the label here and
jumping from the earlier code to the end of function more readable ?

                goto out_unlock;
        
        out_destroy_policy:
        	for_each_cpu(j, policy->real_cpus)
        		remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
        
        out_offline_policy:
        	if (cpufreq_driver->offline)
        		cpufreq_driver->offline(policy);
        
        out_exit_policy:
        	if (cpufreq_driver->exit)
        		cpufreq_driver->exit(policy);
        
        out_clear_policy:
        	cpumask_clear(policy->cpus);
        
        out_unlock:
	up_write(&policy->rwsem);
	return ret;
        
Either ways:

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 04/10] cpufreq: Add and use cpufreq policy locking guards

[Viresh Kumar]

On 28-03-25, 21:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce "read" and "write" locking guards for cpufreq policies and use
> them where applicable in the cpufreq core.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |  121 ++++++++++++++++++++--------------------------
>  include/linux/cpufreq.h   |    6 ++
>  2 files changed, 60 insertions(+), 67 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 03/10] cpufreq: Split cpufreq_online()

[Viresh Kumar]

On 01-04-25, 14:08, Viresh Kumar wrote:
> Instead of jumping back to the function, won't declaring the label here and
> jumping from the earlier code to the end of function more readable ?
> 
>                 goto out_unlock;
>         
>         out_destroy_policy:
>         	for_each_cpu(j, policy->real_cpus)
>         		remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
>         
>         out_offline_policy:
>         	if (cpufreq_driver->offline)
>         		cpufreq_driver->offline(policy);
>         
>         out_exit_policy:
>         	if (cpufreq_driver->exit)
>         		cpufreq_driver->exit(policy);
>         
>         out_clear_policy:
>         	cpumask_clear(policy->cpus);
>         
>         out_unlock:
>               up_write(&policy->rwsem);
>               return ret;

And now I see that after 4/10, it doesn't matter anymore.

-- 
viresh

Re: [PATCH v1 06/10] cpufreq: Use locking guard and __free() in cpufreq_update_policy()

[Viresh Kumar]

On 28-03-25, 21:44, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of using cpufreq_cpu_acquire() and cpufreq_cpu_release() in
> cpufreq_update_policy(), which is the last user of these functions,
> make it use __free() for policy reference counting cleanup and the
> "write" locking guard for policy locking.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2752,23 +2752,23 @@
>   */
>  void cpufreq_update_policy(unsigned int cpu)
>  {
> -	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy);
>  
> +	policy = cpufreq_cpu_get(cpu);
>  	if (!policy)
>  		return;
>  
> +	guard(cpufreq_policy_write)(policy);
> +
>  	/*
>  	 * BIOS might change freq behind our back
>  	 * -> ask driver for current freq and notify governors about a change
>  	 */
>  	if (cpufreq_driver->get && has_target() &&
>  	    (cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
> -		goto unlock;
> +		return;
>  
>  	refresh_frequency_limits(policy);
> -
> -unlock:
> -	cpufreq_cpu_release(policy);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 07/10] cpufreq: Drop cpufreq_cpu_acquire() and cpufreq_cpu_release()

[Viresh Kumar]

On 28-03-25, 21:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since cpufreq_cpu_acquire() and cpufreq_cpu_release() have no more
> users in the tree, remove them.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   45 ---------------------------------------------
>  include/linux/cpufreq.h   |    2 --
>  2 files changed, 47 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 08/10] cpufreq: Use __free() for policy reference counting cleanup

[Viresh Kumar]

On 28-03-25, 21:46, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Use __free() for policy reference counting cleanup where applicable in
> the cpufreq core.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   57 ++++++++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 32 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 09/10] cpufreq: Introduce cpufreq_policy_refresh()

[Viresh Kumar]

On 28-03-25, 21:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since cpufreq_update_limits() obtains a cpufreq policy pointer for the
> given CPU and reference counts the object pointed to by it, calling
> cpufreq_update_policy() from cpufreq_update_limits() is somewhat
> wasteful because that function calls cpufreq_cpu_get() on the same
> CPU again.
> 
> To avoid that unnecessary overhead, move the part of the code running
> under the policy rwsem from cpufreq_update_policy() to a new function
> called cpufreq_policy_refresh() and invoke that new function from
> both cpufreq_update_policy() and cpufreq_update_limits().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[Viresh Kumar]

On 28-03-25, 21:48, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since cpufreq_update_limits() obtains a cpufreq policy pointer for the
> given CPU and reference counts the corresponding policy object, it may
> as well pass the policy pointer to the cpufreq driver's ->update_limits()
> callback which allows that callback to avoid invoking cpufreq_cpu_get()
> for the same CPU.
> 
> Accordingly, redefine ->update_limits() to take a policy pointer instead
> of a CPU number and update both drivers implementing it, intel_pstate
> and amd-pstate, as needed.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/amd-pstate.c   |    7 ++-----
>  drivers/cpufreq/cpufreq.c      |    2 +-
>  drivers/cpufreq/intel_pstate.c |   29 ++++++++++++++++++-----------
>  include/linux/cpufreq.h        |    2 +-
>  4 files changed, 22 insertions(+), 18 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v1 01/10] cpufreq: Reference count policy in cpufreq_update_limits()

[Rafael J. Wysocki]

On Tue, Apr 1, 2025 at 10:30 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 28-03-25, 21:39, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Since acpi_processor_notify() can be called before registering a cpufreq
> > driver or even in cases when a cpufreq driver is not registered at all,
> > cpufreq_update_limits() needs to check if a cpufreq driver is present
> > and prevent it from being unregistered.
> >
> > For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
> > policy pointer for the given CPU and reference count the corresponding
> > policy object, if present.
> >
> > Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
> > Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
> > Reporetd-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Cc: All applicable <stable@vger.kernel.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2781,6 +2781,12 @@
> >   */
> >  void cpufreq_update_limits(unsigned int cpu)
> >  {
> > +     struct cpufreq_policy *policy __free(put_cpufreq_policy);
> > +
> > +     policy = cpufreq_cpu_get(cpu);
> > +     if (!policy)
> > +             return;
> > +
> >       if (cpufreq_driver->update_limits)
> >               cpufreq_driver->update_limits(cpu);
> >       else
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks for all of the ACKs!

Re: [PATCH v1 00/10] cpufreq: cpufreq_update_limits() fix and some cleanups

[Mario Limonciello]

On 3/28/2025 3:36 PM, Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> This series of patches consists of a fix for a possible (and actually happening
> in some cases) NULL pointer dereference in cpufreq_update_limits() (patch
> [01/10]) and a bunch of tangentially related fixes inspired by it, mostly about
> using __free() for cleanup and locking guards.
> 
> The first patch is definitely for 6.15, but the rest is rather 6.16 material.
> 
> Please refer to the individual patch changelogs for more information.
> 
> Thanks!
> 
> 
> 

It's a great set of cleanups, makes the code a lot easier to follow.

For the series:

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Re: [PATCH v1 05/10] cpufreq: intel_pstate: Rearrange max frequency updates handling code

[Rafael J. Wysocki]

On Fri, Mar 28, 2025 at 9:58 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Rename __intel_pstate_update_max_freq() to intel_pstate_update_max_freq()
> and move the cpufreq policy reference counting and locking into it (and
> implement the locking with the recently introduced cpufreq policy "write"
> locking guard).
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Hi Srinivas,

If you have any concerns regarding this patch, please let me know.

> ---
>  drivers/cpufreq/intel_pstate.c |   52 +++++++++++++----------------------------
>  1 file changed, 17 insertions(+), 35 deletions(-)
>
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1353,9 +1353,16 @@
>                 cpufreq_update_policy(cpu);
>  }
>
> -static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
> -                                          struct cpufreq_policy *policy)
> +static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
>  {
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> +
> +       policy = cpufreq_cpu_get(cpudata->cpu);
> +       if (!policy)
> +               return false;
> +
> +       guard(cpufreq_policy_write)(policy);
> +
>         if (hwp_active)
>                 intel_pstate_get_hwp_cap(cpudata);
>
> @@ -1363,44 +1370,24 @@
>                         cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
>
>         refresh_frequency_limits(policy);
> +
> +       return true;
>  }
>
>  static void intel_pstate_update_limits(unsigned int cpu)
>  {
> -       struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
> -       struct cpudata *cpudata;
> -
> -       if (!policy)
> -               return;
> -
> -       cpudata = all_cpu_data[cpu];
> -
> -       __intel_pstate_update_max_freq(cpudata, policy);
> -
> -       /* Prevent the driver from being unregistered now. */
> -       mutex_lock(&intel_pstate_driver_lock);
> +       struct cpudata *cpudata = all_cpu_data[cpu];
>
> -       cpufreq_cpu_release(policy);
> -
> -       hybrid_update_capacity(cpudata);
> -
> -       mutex_unlock(&intel_pstate_driver_lock);
> +       if (intel_pstate_update_max_freq(cpudata))
> +               hybrid_update_capacity(cpudata);
>  }
>
>  static void intel_pstate_update_limits_for_all(void)
>  {
>         int cpu;
>
> -       for_each_possible_cpu(cpu) {
> -               struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
> -
> -               if (!policy)
> -                       continue;
> -
> -               __intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
> -
> -               cpufreq_cpu_release(policy);
> -       }
> +       for_each_possible_cpu(cpu)
> +               intel_pstate_update_max_freq(all_cpu_data[cpu]);
>
>         mutex_lock(&hybrid_capacity_lock);
>
> @@ -1840,13 +1827,8 @@
>  {
>         struct cpudata *cpudata =
>                 container_of(to_delayed_work(work), struct cpudata, hwp_notify_work);
> -       struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata->cpu);
> -
> -       if (policy) {
> -               __intel_pstate_update_max_freq(cpudata, policy);
> -
> -               cpufreq_cpu_release(policy);
>
> +       if (intel_pstate_update_max_freq(cpudata)) {
>                 /*
>                  * The driver will not be unregistered while this function is
>                  * running, so update the capacity without acquiring the driver
>
>
>
>

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[Rafael J. Wysocki]

On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since cpufreq_update_limits() obtains a cpufreq policy pointer for the
> given CPU and reference counts the corresponding policy object, it may
> as well pass the policy pointer to the cpufreq driver's ->update_limits()
> callback which allows that callback to avoid invoking cpufreq_cpu_get()
> for the same CPU.
>
> Accordingly, redefine ->update_limits() to take a policy pointer instead
> of a CPU number and update both drivers implementing it, intel_pstate
> and amd-pstate, as needed.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Hi Srinivas,

If you have any concerns regarding this patch, please let me know
(note that it is based on the [05/10]).

> ---
>  drivers/cpufreq/amd-pstate.c   |    7 ++-----
>  drivers/cpufreq/cpufreq.c      |    2 +-
>  drivers/cpufreq/intel_pstate.c |   29 ++++++++++++++++++-----------
>  include/linux/cpufreq.h        |    2 +-
>  4 files changed, 22 insertions(+), 18 deletions(-)
>
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -821,19 +821,16 @@
>         schedule_work(&sched_prefcore_work);
>  }
>
> -static void amd_pstate_update_limits(unsigned int cpu)
> +static void amd_pstate_update_limits(struct cpufreq_policy *policy)
>  {
> -       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>         struct amd_cpudata *cpudata;
>         u32 prev_high = 0, cur_high = 0;
>         bool highest_perf_changed = false;
> +       unsigned int cpu = policy->cpu;
>
>         if (!amd_pstate_prefcore)
>                 return;
>
> -       if (!policy)
> -               return;
> -
>         if (amd_get_highest_perf(cpu, &cur_high))
>                 return;
>
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2741,7 +2741,7 @@
>                 return;
>
>         if (cpufreq_driver->update_limits)
> -               cpufreq_driver->update_limits(cpu);
> +               cpufreq_driver->update_limits(policy);
>         else
>                 cpufreq_policy_refresh(policy);
>  }
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1353,14 +1353,9 @@
>                 cpufreq_update_policy(cpu);
>  }
>
> -static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> +static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
> +                                          struct cpudata *cpudata)
>  {
> -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> -
> -       policy = cpufreq_cpu_get(cpudata->cpu);
> -       if (!policy)
> -               return false;
> -
>         guard(cpufreq_policy_write)(policy);
>
>         if (hwp_active)
> @@ -1370,16 +1365,28 @@
>                         cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
>
>         refresh_frequency_limits(policy);
> +}
> +
> +static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> +{
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> +
> +       policy = cpufreq_cpu_get(cpudata->cpu);
> +       if (!policy)
> +               return false;
> +
> +       __intel_pstate_update_max_freq(policy, cpudata);
>
>         return true;
>  }
>
> -static void intel_pstate_update_limits(unsigned int cpu)
> +static void intel_pstate_update_limits(struct cpufreq_policy *policy)
>  {
> -       struct cpudata *cpudata = all_cpu_data[cpu];
> +       struct cpudata *cpudata = all_cpu_data[policy->cpu];
> +
> +       __intel_pstate_update_max_freq(policy, cpudata);
>
> -       if (intel_pstate_update_max_freq(cpudata))
> -               hybrid_update_capacity(cpudata);
> +       hybrid_update_capacity(cpudata);
>  }
>
>  static void intel_pstate_update_limits_for_all(void)
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -399,7 +399,7 @@
>         unsigned int    (*get)(unsigned int cpu);
>
>         /* Called to update policy limits on firmware notifications. */
> -       void            (*update_limits)(unsigned int cpu);
> +       void            (*update_limits)(struct cpufreq_policy *policy);
>
>         /* optional */
>         int             (*bios_limit)(int cpu, unsigned int *limit);
>
>
>
>

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[srinivas pandruvada]

On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net>
> wrote:
> > 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Since cpufreq_update_limits() obtains a cpufreq policy pointer for
> > the
> > given CPU and reference counts the corresponding policy object, it
> > may
> > as well pass the policy pointer to the cpufreq driver's -
> > >update_limits()
> > callback which allows that callback to avoid invoking
> > cpufreq_cpu_get()
> > for the same CPU.
> > 
> > Accordingly, redefine ->update_limits() to take a policy pointer
> > instead
> > of a CPU number and update both drivers implementing it,
> > intel_pstate
> > and amd-pstate, as needed.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
Hi Rafael,

> Hi Srinivas,
> 
> If you have any concerns regarding this patch, please let me know
> (note that it is based on the [05/10]).
> 
Changes looks fine, but wants to test out some update limits from
interrupt path.
Checked your branches at linux-pm, not able to locate in any branch to
apply.
Please point me to a branch.

Thanks,
Srinivas

> > ---
> >  drivers/cpufreq/amd-pstate.c   |    7 ++-----
> >  drivers/cpufreq/cpufreq.c      |    2 +-
> >  drivers/cpufreq/intel_pstate.c |   29 ++++++++++++++++++----------
> > -
> >  include/linux/cpufreq.h        |    2 +-
> >  4 files changed, 22 insertions(+), 18 deletions(-)
> > 
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -821,19 +821,16 @@
> >         schedule_work(&sched_prefcore_work);
> >  }
> > 
> > -static void amd_pstate_update_limits(unsigned int cpu)
> > +static void amd_pstate_update_limits(struct cpufreq_policy
> > *policy)
> >  {
> > -       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> > cpufreq_cpu_get(cpu);
> >         struct amd_cpudata *cpudata;
> >         u32 prev_high = 0, cur_high = 0;
> >         bool highest_perf_changed = false;
> > +       unsigned int cpu = policy->cpu;
> > 
> >         if (!amd_pstate_prefcore)
> >                 return;
> > 
> > -       if (!policy)
> > -               return;
> > -
> >         if (amd_get_highest_perf(cpu, &cur_high))
> >                 return;
> > 
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2741,7 +2741,7 @@
> >                 return;
> > 
> >         if (cpufreq_driver->update_limits)
> > -               cpufreq_driver->update_limits(cpu);
> > +               cpufreq_driver->update_limits(policy);
> >         else
> >                 cpufreq_policy_refresh(policy);
> >  }
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1353,14 +1353,9 @@
> >                 cpufreq_update_policy(cpu);
> >  }
> > 
> > -static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> > +static void __intel_pstate_update_max_freq(struct cpufreq_policy
> > *policy,
> > +                                          struct cpudata *cpudata)
> >  {
> > -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> > -
> > -       policy = cpufreq_cpu_get(cpudata->cpu);
> > -       if (!policy)
> > -               return false;
> > -
> >         guard(cpufreq_policy_write)(policy);
> > 
> >         if (hwp_active)
> > @@ -1370,16 +1365,28 @@
> >                         cpudata->pstate.max_freq : cpudata-
> > >pstate.turbo_freq;
> > 
> >         refresh_frequency_limits(policy);
> > +}
> > +
> > +static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> > +{
> > +       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> > +
> > +       policy = cpufreq_cpu_get(cpudata->cpu);
> > +       if (!policy)
> > +               return false;
> > +
> > +       __intel_pstate_update_max_freq(policy, cpudata);
> > 
> >         return true;
> >  }
> > 
> > -static void intel_pstate_update_limits(unsigned int cpu)
> > +static void intel_pstate_update_limits(struct cpufreq_policy
> > *policy)
> >  {
> > -       struct cpudata *cpudata = all_cpu_data[cpu];
> > +       struct cpudata *cpudata = all_cpu_data[policy->cpu];
> > +
> > +       __intel_pstate_update_max_freq(policy, cpudata);
> > 
> > -       if (intel_pstate_update_max_freq(cpudata))
> > -               hybrid_update_capacity(cpudata);
> > +       hybrid_update_capacity(cpudata);
> >  }
> > 
> >  static void intel_pstate_update_limits_for_all(void)
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -399,7 +399,7 @@
> >         unsigned int    (*get)(unsigned int cpu);
> > 
> >         /* Called to update policy limits on firmware
> > notifications. */
> > -       void            (*update_limits)(unsigned int cpu);
> > +       void            (*update_limits)(struct cpufreq_policy
> > *policy);
> > 
> >         /* optional */
> >         int             (*bios_limit)(int cpu, unsigned int
> > *limit);
> > 
> > 
> > 
> > 
> 

RE: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[Doug Smythies]

On 2025.04.07 15:38 srinivas pandruvada wrote:
> On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
>> On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> 
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> 
>>> Since cpufreq_update_limits() obtains a cpufreq policy pointer for
>>> the
>>> given CPU and reference counts the corresponding policy object, it
>>> may
>>> as well pass the policy pointer to the cpufreq driver's -
>>> >update_limits()
>>> callback which allows that callback to avoid invoking
>>> cpufreq_cpu_get()
>>> for the same CPU.
>>> 
>>> Accordingly, redefine ->update_limits() to take a policy pointer
>>> instead
>>> of a CPU number and update both drivers implementing it,
>>> intel_pstate
>>> and amd-pstate, as needed.
>>> 
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> 
> Hi Rafael,
>
>> Hi Srinivas,
>> 
>> If you have any concerns regarding this patch, please let me know
>> (note that it is based on the [05/10]).
>> 
> Changes looks fine, but wants to test out some update limits from
> interrupt path.
> Checked your branches at linux-pm, not able to locate in any branch to
> apply.
> Please point me to a branch.

Hi Srinivas,

You can get the series from patchworks [1].
Then just edit it, deleting patch 1 of 10, because that one was included in kernel 6.15-rc1
The rest will apply cleanly to kernel 6.15-rc1.

I just did all this in the last hour, because I wanted to check if the patchset fixed a years old
issue with HWP enabled, intel_cpufreq, schedutil, minimum frequency set above hardware
minimum was properly reflected in scaling_cur_freq  when the frequency was stale. [2]
The issue is not fixed.

[1] https://patchwork.kernel.org/project/linux-pm/patch/2315023.iZASKD2KPV@rjwysocki.net/
[2] https://lore.kernel.org/linux-pm/CAAYoRsU2=qOUhBKSRskcoRXSgBudWgDNVvKtJA+c22cPa8EZ1Q@mail.gmail.com/

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[Rafael J. Wysocki]

On Tue, Apr 8, 2025 at 12:28 AM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net>
> > wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Since cpufreq_update_limits() obtains a cpufreq policy pointer for
> > > the
> > > given CPU and reference counts the corresponding policy object, it
> > > may
> > > as well pass the policy pointer to the cpufreq driver's -
> > > >update_limits()
> > > callback which allows that callback to avoid invoking
> > > cpufreq_cpu_get()
> > > for the same CPU.
> > >
> > > Accordingly, redefine ->update_limits() to take a policy pointer
> > > instead
> > > of a CPU number and update both drivers implementing it,
> > > intel_pstate
> > > and amd-pstate, as needed.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> Hi Rafael,
>
> > Hi Srinivas,
> >
> > If you have any concerns regarding this patch, please let me know
> > (note that it is based on the [05/10]).
> >
> Changes looks fine, but wants to test out some update limits from
> interrupt path.
> Checked your branches at linux-pm, not able to locate in any branch to
> apply.
> Please point me to a branch.

I'll put it in 'testing' later today.

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[Rafael J. Wysocki]

On Tue, Apr 8, 2025 at 1:41 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Apr 8, 2025 at 12:28 AM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> >
> > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> > > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki <rjw@rjwysocki.net>
> > > wrote:
> > > >
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Since cpufreq_update_limits() obtains a cpufreq policy pointer for
> > > > the
> > > > given CPU and reference counts the corresponding policy object, it
> > > > may
> > > > as well pass the policy pointer to the cpufreq driver's -
> > > > >update_limits()
> > > > callback which allows that callback to avoid invoking
> > > > cpufreq_cpu_get()
> > > > for the same CPU.
> > > >
> > > > Accordingly, redefine ->update_limits() to take a policy pointer
> > > > instead
> > > > of a CPU number and update both drivers implementing it,
> > > > intel_pstate
> > > > and amd-pstate, as needed.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > Hi Rafael,
> >
> > > Hi Srinivas,
> > >
> > > If you have any concerns regarding this patch, please let me know
> > > (note that it is based on the [05/10]).
> > >
> > Changes looks fine, but wants to test out some update limits from
> > interrupt path.
> > Checked your branches at linux-pm, not able to locate in any branch to
> > apply.
> > Please point me to a branch.
>
> I'll put it in 'testing' later today.

Now available from

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git testing

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[srinivas pandruvada]

On Mon, 2025-04-07 at 16:49 -0700, Doug Smythies wrote:
> On 2025.04.07 15:38 srinivas pandruvada wrote:
> > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> > > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki
> > > <rjw@rjwysocki.net> wrote:
> > > > 
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > Since cpufreq_update_limits() obtains a cpufreq policy pointer
> > > > for
> > > > the
> > > > given CPU and reference counts the corresponding policy object,
> > > > it
> > > > may
> > > > as well pass the policy pointer to the cpufreq driver's -
> > > > > update_limits()
> > > > callback which allows that callback to avoid invoking
> > > > cpufreq_cpu_get()
> > > > for the same CPU.
> > > > 
> > > > Accordingly, redefine ->update_limits() to take a policy
> > > > pointer
> > > > instead
> > > > of a CPU number and update both drivers implementing it,
> > > > intel_pstate
> > > > and amd-pstate, as needed.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > Hi Rafael,
> > 
> > > Hi Srinivas,
> > > 
> > > If you have any concerns regarding this patch, please let me know
> > > (note that it is based on the [05/10]).
> > > 
> > Changes looks fine, but wants to test out some update limits from
> > interrupt path.
> > Checked your branches at linux-pm, not able to locate in any branch
> > to
> > apply.
> > Please point me to a branch.
> 
> Hi Srinivas,
> 
> You can get the series from patchworks [1].
> Then just edit it, deleting patch 1 of 10, because that one was
> included in kernel 6.15-rc1
> The rest will apply cleanly to kernel 6.15-rc1.
> 
Hi Doug,

You are correct. But I prefer a branch usually as there may be other
fixes so that I can verify once.

Thanks,
Srinivas

> I just did all this in the last hour, because I wanted to check if
> the patchset fixed a years old
> issue with HWP enabled, intel_cpufreq, schedutil, minimum frequency
> set above hardware
> minimum was properly reflected in scaling_cur_freq  when the
> frequency was stale. [2]
> The issue is not fixed.
> 
> [1]
> https://patchwork.kernel.org/project/linux-pm/patch/2315023.iZASKD2KPV@rjwysocki.net/
> [2]
> https://lore.kernel.org/linux-pm/CAAYoRsU2=qOUhBKSRskcoRXSgBudWgDNVvKtJA+c22cPa8EZ1Q@mail.gmail.com/
> 
> 
> 

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[srinivas pandruvada]

On Tue, 2025-04-08 at 15:37 +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 8, 2025 at 1:41 PM Rafael J. Wysocki <rafael@kernel.org>
> wrote:
> > 
> > On Tue, Apr 8, 2025 at 12:28 AM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > > 
> > > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> > > > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki
> > > > <rjw@rjwysocki.net>
> > > > wrote:
> > > > > 
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > Since cpufreq_update_limits() obtains a cpufreq policy
> > > > > pointer for
> > > > > the
> > > > > given CPU and reference counts the corresponding policy
> > > > > object, it
> > > > > may
> > > > > as well pass the policy pointer to the cpufreq driver's -
> > > > > > update_limits()
> > > > > callback which allows that callback to avoid invoking
> > > > > cpufreq_cpu_get()
> > > > > for the same CPU.
> > > > > 
> > > > > Accordingly, redefine ->update_limits() to take a policy
> > > > > pointer
> > > > > instead
> > > > > of a CPU number and update both drivers implementing it,
> > > > > intel_pstate
> > > > > and amd-pstate, as needed.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > Hi Rafael,
> > > 
> > > > Hi Srinivas,
> > > > 
> > > > If you have any concerns regarding this patch, please let me
> > > > know
> > > > (note that it is based on the [05/10]).
> > > > 
> > > Changes looks fine, but wants to test out some update limits from
> > > interrupt path.
> > > Checked your branches at linux-pm, not able to locate in any
> > > branch to
> > > apply.
> > > Please point me to a branch.
> > 
> > I'll put it in 'testing' later today.
> 
> Now available from
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> testing
> 
Looks good.

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[Rafael J. Wysocki]

On Tue, Apr 8, 2025 at 7:47 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2025-04-08 at 15:37 +0200, Rafael J. Wysocki wrote:
> > On Tue, Apr 8, 2025 at 1:41 PM Rafael J. Wysocki <rafael@kernel.org>
> > wrote:
> > >
> > > On Tue, Apr 8, 2025 at 12:28 AM srinivas pandruvada
> > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > >
> > > > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> > > > > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki
> > > > > <rjw@rjwysocki.net>
> > > > > wrote:
> > > > > >
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > Since cpufreq_update_limits() obtains a cpufreq policy
> > > > > > pointer for
> > > > > > the
> > > > > > given CPU and reference counts the corresponding policy
> > > > > > object, it
> > > > > > may
> > > > > > as well pass the policy pointer to the cpufreq driver's -
> > > > > > > update_limits()
> > > > > > callback which allows that callback to avoid invoking
> > > > > > cpufreq_cpu_get()
> > > > > > for the same CPU.
> > > > > >
> > > > > > Accordingly, redefine ->update_limits() to take a policy
> > > > > > pointer
> > > > > > instead
> > > > > > of a CPU number and update both drivers implementing it,
> > > > > > intel_pstate
> > > > > > and amd-pstate, as needed.
> > > > > >
> > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > Hi Rafael,
> > > >
> > > > > Hi Srinivas,
> > > > >
> > > > > If you have any concerns regarding this patch, please let me
> > > > > know
> > > > > (note that it is based on the [05/10]).
> > > > >
> > > > Changes looks fine, but wants to test out some update limits from
> > > > interrupt path.
> > > > Checked your branches at linux-pm, not able to locate in any
> > > > branch to
> > > > apply.
> > > > Please point me to a branch.
> > >
> > > I'll put it in 'testing' later today.
> >
> > Now available from
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> > testing
> >
> Looks good.
>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks!

IIUC this applies to both [5/10] and [10/10], or please let me know if
that's not the case.

Re: [PATCH v1 10/10] cpufreq: Pass policy pointer to ->update_limits()

[srinivas pandruvada]

On Tue, 2025-04-08 at 20:34 +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 8, 2025 at 7:47 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Tue, 2025-04-08 at 15:37 +0200, Rafael J. Wysocki wrote:
> > > On Tue, Apr 8, 2025 at 1:41 PM Rafael J. Wysocki
> > > <rafael@kernel.org>
> > > wrote:
> > > > 
> > > > On Tue, Apr 8, 2025 at 12:28 AM srinivas pandruvada
> > > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > > > 
> > > > > On Mon, 2025-04-07 at 20:48 +0200, Rafael J. Wysocki wrote:
> > > > > > On Fri, Mar 28, 2025 at 9:49 PM Rafael J. Wysocki
> > > > > > <rjw@rjwysocki.net>
> > > > > > wrote:
> > > > > > > 
> > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > 
> > > > > > > Since cpufreq_update_limits() obtains a cpufreq policy
> > > > > > > pointer for
> > > > > > > the
> > > > > > > given CPU and reference counts the corresponding policy
> > > > > > > object, it
> > > > > > > may
> > > > > > > as well pass the policy pointer to the cpufreq driver's -
> > > > > > > > update_limits()
> > > > > > > callback which allows that callback to avoid invoking
> > > > > > > cpufreq_cpu_get()
> > > > > > > for the same CPU.
> > > > > > > 
> > > > > > > Accordingly, redefine ->update_limits() to take a policy
> > > > > > > pointer
> > > > > > > instead
> > > > > > > of a CPU number and update both drivers implementing it,
> > > > > > > intel_pstate
> > > > > > > and amd-pstate, as needed.
> > > > > > > 
> > > > > > > Signed-off-by: Rafael J. Wysocki
> > > > > > > <rafael.j.wysocki@intel.com>
> > > > > > 
> > > > > Hi Rafael,
> > > > > 
> > > > > > Hi Srinivas,
> > > > > > 
> > > > > > If you have any concerns regarding this patch, please let
> > > > > > me
> > > > > > know
> > > > > > (note that it is based on the [05/10]).
> > > > > > 
> > > > > Changes looks fine, but wants to test out some update limits
> > > > > from
> > > > > interrupt path.
> > > > > Checked your branches at linux-pm, not able to locate in any
> > > > > branch to
> > > > > apply.
> > > > > Please point me to a branch.
> > > > 
> > > > I'll put it in 'testing' later today.
> > > 
> > > Now available from
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> > > testing
> > > 
> > Looks good.
> > 
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> Thanks!
> 
> IIUC this applies to both [5/10] and [10/10], or please let me know
> if
> that's not the case.

This is for both. I tested the whole "testing" branch.

Thanks,
Srinivas

Re: [PATCH v1 00/10] cpufreq: cpufreq_update_limits() fix and some cleanups

[Sudeep Holla]

On Fri, Mar 28, 2025 at 09:36:05PM +0100, Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> This series of patches consists of a fix for a possible (and actually happening
> in some cases) NULL pointer dereference in cpufreq_update_limits() (patch
> [01/10]) and a bunch of tangentially related fixes inspired by it, mostly about
> using __free() for cleanup and locking guards.
> 
> The first patch is definitely for 6.15, but the rest is rather 6.16 material.
> 
> Please refer to the individual patch changelogs for more information.
> 

For all the generic changes(i.e. except x86 specific changes)

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

I also gave it a quick test using scmi-cpufreq on my Juno platform.

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

Re: [PATCH v1 01/10] cpufreq: Reference count policy in cpufreq_update_limits()

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

On Fri, Mar 28, 2025 at 09:39:08PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since acpi_processor_notify() can be called before registering a cpufreq
> driver or even in cases when a cpufreq driver is not registered at all,
> cpufreq_update_limits() needs to check if a cpufreq driver is present
> and prevent it from being unregistered.
> 
> For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
> policy pointer for the given CPU and reference count the corresponding
> policy object, if present.
> 
> Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
> Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
> Reporetd-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It looks like this patch is missing in stable branches.

> ---
>  drivers/cpufreq/cpufreq.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2781,6 +2781,12 @@
>   */
>  void cpufreq_update_limits(unsigned int cpu)
>  {
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy);
> +
> +	policy = cpufreq_cpu_get(cpu);
> +	if (!policy)
> +		return;
> +
>  	if (cpufreq_driver->update_limits)
>  		cpufreq_driver->update_limits(cpu);
>  	else
> 
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 01/10] cpufreq: Reference count policy in cpufreq_update_limits()

[Rafael J. Wysocki]

On Tue, Apr 15, 2025 at 3:08 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Fri, Mar 28, 2025 at 09:39:08PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Since acpi_processor_notify() can be called before registering a cpufreq
> > driver or even in cases when a cpufreq driver is not registered at all,
> > cpufreq_update_limits() needs to check if a cpufreq driver is present
> > and prevent it from being unregistered.
> >
> > For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
> > policy pointer for the given CPU and reference count the corresponding
> > policy object, if present.
> >
> > Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
> > Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
> > Reporetd-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Cc: All applicable <stable@vger.kernel.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It looks like this patch is missing in stable branches.

It may have not been picked up by the "stable" maintainers yet.

> > ---
> >  drivers/cpufreq/cpufreq.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2781,6 +2781,12 @@
> >   */
> >  void cpufreq_update_limits(unsigned int cpu)
> >  {
> > +     struct cpufreq_policy *policy __free(put_cpufreq_policy);
> > +
> > +     policy = cpufreq_cpu_get(cpu);
> > +     if (!policy)
> > +             return;
> > +
> >       if (cpufreq_driver->update_limits)
> >               cpufreq_driver->update_limits(cpu);
> >       else
> >
> >
> >
>
> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab