[PATCH v4 00/10] cpufreq: use __free() for all cpufreq_cpu_get() references

[PATCH v4 00/10] cpufreq: use __free() for all cpufreq_cpu_get() references

[Zihuan Zhang]

This patchset converts all remaining cpufreq users to rely on the
__free(put_cpufreq_policy) annotation for policy references, instead of
calling cpufreq_cpu_put() manually.

Motivation:
- Reduce the chance of reference counting mistakes
- Make the code more consistent with the latest kernel style
- behavior remains the same, but reference counting is now safer 
  and easier to maintain.

The changes are split into 12 patches as they touch different subsystems
and are maintained by different people. There is no functional change.

V4:
 - Fix compile error in topology.c
 - drop 2 patches
 - Move code into a function in processor_thermal.c
 - Move code into a function in intel_pstate.c
 - Move policy declare in dtpm_cpu.c
 - Fix compile error in imx_thermal.c
 - Fix compile error in ti-thermal-common.c
 - Fix compile error in energy_model.c

V3:
 - drop patch 'KVM: x86: Use __free(put_cpufreq_policy) for policy reference'
 - removed 5 patches which has been applied
 - Consolidate CPUFreq policy assignments and allocations into one line,
   suggested by Ben Horgan
 - Change cpu_has_cpufreq() return type to bool, following Rafael's suggestion
 - Change the title to 'Use scope-based cleanup helper'

V2:
 - Fix compile error in powernv-cpufreq.c
 - Split patch to separate logical changes

Zihuan Zhang (10):
  arm64: topology: Use scope-based cleanup helper
  ACPI: processor: thermal: Use scope-based cleanup helper
  cpufreq: intel_pstate: Use scope-based cleanup helper
  cpufreq: powernv: Use scope-based cleanup helper
  PM / devfreq: Use scope-based cleanup helper
  drm/i915: Use scope-based cleanup helper
  powercap: dtpm_cpu: Use scope-based cleanup helper
  thermal: imx: Use scope-based cleanup helper
  thermal/drivers/ti-soc-thermal: Use scope-based cleanup helper
  PM: EM: Use scope-based cleanup helper

 arch/arm64/kernel/topology.c                  |  9 ++---
 drivers/acpi/processor_thermal.c              | 37 +++++++++++--------
 drivers/cpufreq/intel_pstate.c                | 19 ++++++----
 drivers/cpufreq/powernv-cpufreq.c             | 11 +++---
 drivers/devfreq/governor_passive.c            | 25 +++++-------
 drivers/gpu/drm/i915/gt/intel_llc.c           |  4 +-
 drivers/powercap/dtpm_cpu.c                   | 30 ++++++---------
 drivers/thermal/imx_thermal.c                 | 14 +++----
 .../ti-soc-thermal/ti-thermal-common.c        | 13 ++-----
 kernel/power/energy_model.c                   |  7 +---
 10 files changed, 73 insertions(+), 96 deletions(-)

-- 
2.25.1

[PATCH v4 01/10] arm64: topology: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 arch/arm64/kernel/topology.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 5d07ee85bdae..0eebd3492669 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -307,17 +307,16 @@ int arch_freq_get_on_cpu(int cpu)
 		 */
 		if (!housekeeping_cpu(cpu, HK_TYPE_TICK) ||
 		    time_is_before_jiffies(last_update + msecs_to_jiffies(AMU_SAMPLE_EXP_MS))) {
-			struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+			struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+				cpufreq_cpu_get(cpu);
 			int ref_cpu;
 
 			if (!policy)
 				return -EINVAL;
 
 			if (!cpumask_intersects(policy->related_cpus,
-						housekeeping_cpumask(HK_TYPE_TICK))) {
-				cpufreq_cpu_put(policy);
+						housekeeping_cpumask(HK_TYPE_TICK)))
 				return -EOPNOTSUPP;
-			}
 
 			for_each_cpu_wrap(ref_cpu, policy->cpus, cpu + 1) {
 				if (ref_cpu == start_cpu) {
@@ -329,8 +328,6 @@ int arch_freq_get_on_cpu(int cpu)
 					break;
 			}
 
-			cpufreq_cpu_put(policy);
-
 			if (ref_cpu >= nr_cpu_ids)
 				/* No alternative to pull info from */
 				return -EAGAIN;
-- 
2.25.1

[PATCH v4 02/10] ACPI: processor: thermal: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/acpi/processor_thermal.c | 37 ++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index 1219adb11ab9..5043f17d27b7 100644
--- a/drivers/acpi/processor_thermal.c
+++ b/drivers/acpi/processor_thermal.c
@@ -62,19 +62,14 @@ static int phys_package_first_cpu(int cpu)
 	return 0;
 }
 
-static int cpu_has_cpufreq(unsigned int cpu)
+static bool cpu_has_cpufreq(unsigned int cpu)
 {
-	struct cpufreq_policy *policy;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
 
 	if (!acpi_processor_cpufreq_init)
 		return 0;
 
-	policy = cpufreq_cpu_get(cpu);
-	if (policy) {
-		cpufreq_cpu_put(policy);
-		return 1;
-	}
-	return 0;
+	return !!policy;
 }
 
 static int cpufreq_get_max_state(unsigned int cpu)
@@ -93,9 +88,23 @@ static int cpufreq_get_cur_state(unsigned int cpu)
 	return reduction_step(cpu);
 }
 
+static long long cpufreq_get_max_freq(unsigned int cpu)
+{
+	long long max_freq;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+		cpufreq_cpu_get(cpu);
+
+	if (!policy)
+		return -EINVAL;
+
+	max_freq = (policy->cpuinfo.max_freq *
+		(100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
+
+	return max_freq;
+}
+
 static int cpufreq_set_cur_state(unsigned int cpu, int state)
 {
-	struct cpufreq_policy *policy;
 	struct acpi_processor *pr;
 	unsigned long max_freq;
 	int i, ret;
@@ -120,14 +129,10 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
 		if (unlikely(!freq_qos_request_active(&pr->thermal_req)))
 			continue;
 
-		policy = cpufreq_cpu_get(i);
-		if (!policy)
-			return -EINVAL;
-
-		max_freq = (policy->cpuinfo.max_freq *
-			    (100 - reduction_step(i) * cpufreq_thermal_reduction_pctg)) / 100;
+		max_freq = cpufreq_get_max_freq(cpu);
 
-		cpufreq_cpu_put(policy);
+		if (max_freq == -EINVAL)
+			return -EINVAL;
 
 		ret = freq_qos_update_request(&pr->thermal_req, max_freq);
 		if (ret < 0) {
-- 
2.25.1

[PATCH v4 03/10] cpufreq: intel_pstate: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/cpufreq/intel_pstate.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index f366d35c5840..925efb1e65be 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1209,6 +1209,17 @@ static bool hybrid_clear_max_perf_cpu(void)
 	return ret;
 }
 
+static struct freq_qos_request *intel_pstate_cpufreq_get_req(int cpu)
+{
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+		cpufreq_cpu_get(cpu);
+
+	if (!policy)
+		return NULL;
+
+	return policy->driver_data;
+}
+
 static void __intel_pstate_get_hwp_cap(struct cpudata *cpu)
 {
 	u64 cap;
@@ -1698,19 +1709,13 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b,
 static void update_qos_request(enum freq_qos_req_type type)
 {
 	struct freq_qos_request *req;
-	struct cpufreq_policy *policy;
 	int i;
 
 	for_each_possible_cpu(i) {
 		struct cpudata *cpu = all_cpu_data[i];
 		unsigned int freq, perf_pct;
 
-		policy = cpufreq_cpu_get(i);
-		if (!policy)
-			continue;
-
-		req = policy->driver_data;
-		cpufreq_cpu_put(policy);
+		req = intel_pstate_cpufreq_get_req(i);
 
 		if (!req)
 			continue;
-- 
2.25.1

[PATCH v4 04/10] cpufreq: powernv: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/cpufreq/powernv-cpufreq.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 7d9a5f656de8..811fdbf398fa 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -892,15 +892,15 @@ static int powernv_cpufreq_reboot_notifier(struct notifier_block *nb,
 				unsigned long action, void *unused)
 {
 	int cpu;
-	struct cpufreq_policy *cpu_policy;
 
 	rebooting = true;
 	for_each_online_cpu(cpu) {
-		cpu_policy = cpufreq_cpu_get(cpu);
+		struct cpufreq_policy *cpu_policy __free(put_cpufreq_policy) =
+			cpufreq_cpu_get(cpu);
+
 		if (!cpu_policy)
 			continue;
 		powernv_cpufreq_target_index(cpu_policy, get_nominal_index());
-		cpufreq_cpu_put(cpu_policy);
 	}
 
 	return NOTIFY_DONE;
@@ -913,7 +913,6 @@ static struct notifier_block powernv_cpufreq_reboot_nb = {
 static void powernv_cpufreq_work_fn(struct work_struct *work)
 {
 	struct chip *chip = container_of(work, struct chip, throttle);
-	struct cpufreq_policy *policy;
 	unsigned int cpu;
 	cpumask_t mask;
 
@@ -928,14 +927,14 @@ static void powernv_cpufreq_work_fn(struct work_struct *work)
 	chip->restore = false;
 	for_each_cpu(cpu, &mask) {
 		int index;
+		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+			cpufreq_cpu_get(cpu);
 
-		policy = cpufreq_cpu_get(cpu);
 		if (!policy)
 			continue;
 		index = cpufreq_table_find_index_c(policy, policy->cur, false);
 		powernv_cpufreq_target_index(policy, index);
 		cpumask_andnot(&mask, &mask, policy->cpus);
-		cpufreq_cpu_put(policy);
 	}
 out:
 	cpus_read_unlock();
-- 
2.25.1

[PATCH v4 05/10] PM / devfreq: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/devfreq/governor_passive.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 953cf9a1e9f7..a035cf44bdb8 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -80,24 +80,23 @@ static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
 	struct devfreq_passive_data *p_data =
 				(struct devfreq_passive_data *)devfreq->data;
 	struct devfreq_cpu_data *parent_cpu_data;
-	struct cpufreq_policy *policy;
 	unsigned long cpu, cpu_cur, cpu_min, cpu_max, cpu_percent;
 	unsigned long dev_min, dev_max;
 	unsigned long freq = 0;
 	int ret = 0;
 
 	for_each_online_cpu(cpu) {
-		policy = cpufreq_cpu_get(cpu);
+		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+			cpufreq_cpu_get(cpu);
+
 		if (!policy) {
 			ret = -EINVAL;
 			continue;
 		}
 
 		parent_cpu_data = get_parent_cpu_data(p_data, policy);
-		if (!parent_cpu_data) {
-			cpufreq_cpu_put(policy);
+		if (!parent_cpu_data)
 			continue;
-		}
 
 		/* Get target freq via required opps */
 		cpu_cur = parent_cpu_data->cur_freq * HZ_PER_KHZ;
@@ -106,7 +105,6 @@ static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
 					devfreq->opp_table, &cpu_cur);
 		if (freq) {
 			*target_freq = max(freq, *target_freq);
-			cpufreq_cpu_put(policy);
 			continue;
 		}
 
@@ -121,7 +119,6 @@ static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
 		freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
 
 		*target_freq = max(freq, *target_freq);
-		cpufreq_cpu_put(policy);
 	}
 
 	return ret;
@@ -256,7 +253,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
 	struct device *dev = devfreq->dev.parent;
 	struct opp_table *opp_table = NULL;
 	struct devfreq_cpu_data *parent_cpu_data;
-	struct cpufreq_policy *policy;
 	struct device *cpu_dev;
 	unsigned int cpu;
 	int ret;
@@ -273,23 +269,23 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
 	}
 
 	for_each_possible_cpu(cpu) {
-		policy = cpufreq_cpu_get(cpu);
+		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+			cpufreq_cpu_get(cpu);
+
 		if (!policy) {
 			ret = -EPROBE_DEFER;
 			goto err;
 		}
 
 		parent_cpu_data = get_parent_cpu_data(p_data, policy);
-		if (parent_cpu_data) {
-			cpufreq_cpu_put(policy);
+		if (parent_cpu_data)
 			continue;
-		}
 
 		parent_cpu_data = kzalloc(sizeof(*parent_cpu_data),
 						GFP_KERNEL);
 		if (!parent_cpu_data) {
 			ret = -ENOMEM;
-			goto err_put_policy;
+			goto err;
 		}
 
 		cpu_dev = get_cpu_device(cpu);
@@ -314,7 +310,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
 		parent_cpu_data->max_freq = policy->cpuinfo.max_freq;
 
 		list_add_tail(&parent_cpu_data->node, &p_data->cpu_data_list);
-		cpufreq_cpu_put(policy);
 	}
 
 	mutex_lock(&devfreq->lock);
@@ -327,8 +322,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
 
 err_free_cpu_data:
 	kfree(parent_cpu_data);
-err_put_policy:
-	cpufreq_cpu_put(policy);
 err:
 
 	return ret;
-- 
2.25.1

[PATCH v4 06/10] drm/i915: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/gpu/drm/i915/gt/intel_llc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c b/drivers/gpu/drm/i915/gt/intel_llc.c
index 1d19c073ba2e..f15e4c0fa54b 100644
--- a/drivers/gpu/drm/i915/gt/intel_llc.c
+++ b/drivers/gpu/drm/i915/gt/intel_llc.c
@@ -29,13 +29,11 @@ static struct intel_gt *llc_to_gt(struct intel_llc *llc)
 
 static unsigned int cpu_max_MHz(void)
 {
-	struct cpufreq_policy *policy;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
 	unsigned int max_khz;
 
-	policy = cpufreq_cpu_get(0);
 	if (policy) {
 		max_khz = policy->cpuinfo.max_freq;
-		cpufreq_cpu_put(policy);
 	} else {
 		/*
 		 * Default to measured freq if none found, PCU will ensure we
-- 
2.25.1

[PATCH v4 07/10] powercap: dtpm_cpu: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/powercap/dtpm_cpu.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index 99390ec1481f..f76594185fa2 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -144,19 +144,17 @@ static int update_pd_power_uw(struct dtpm *dtpm)
 static void pd_release(struct dtpm *dtpm)
 {
 	struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
-	struct cpufreq_policy *policy;
 
 	if (freq_qos_request_active(&dtpm_cpu->qos_req))
 		freq_qos_remove_request(&dtpm_cpu->qos_req);
 
-	policy = cpufreq_cpu_get(dtpm_cpu->cpu);
-	if (policy) {
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+		cpufreq_cpu_get(dtpm_cpu->cpu);
+
+	if (policy)
 		for_each_cpu(dtpm_cpu->cpu, policy->related_cpus)
 			per_cpu(dtpm_per_cpu, dtpm_cpu->cpu) = NULL;
 
-		cpufreq_cpu_put(policy);
-	}
-
 	kfree(dtpm_cpu);
 }
 
@@ -192,7 +190,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
 {
 	struct dtpm_cpu *dtpm_cpu;
-	struct cpufreq_policy *policy;
 	struct em_perf_state *table;
 	struct em_perf_domain *pd;
 	char name[CPUFREQ_NAME_LEN];
@@ -202,21 +199,19 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
 	if (dtpm_cpu)
 		return 0;
 
-	policy = cpufreq_cpu_get(cpu);
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+		cpufreq_cpu_get(cpu);
+
 	if (!policy)
 		return 0;
 
 	pd = em_cpu_get(cpu);
-	if (!pd || em_is_artificial(pd)) {
-		ret = -EINVAL;
-		goto release_policy;
-	}
+	if (!pd || em_is_artificial(pd))
+		return -EINVAL;
 
 	dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
-	if (!dtpm_cpu) {
-		ret = -ENOMEM;
-		goto release_policy;
-	}
+	if (!dtpm_cpu)
+		return -ENOMEM;
 
 	dtpm_init(&dtpm_cpu->dtpm, &dtpm_ops);
 	dtpm_cpu->cpu = cpu;
@@ -239,7 +234,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
 	if (ret < 0)
 		goto out_dtpm_unregister;
 
-	cpufreq_cpu_put(policy);
 	return 0;
 
 out_dtpm_unregister:
@@ -251,8 +245,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
 		per_cpu(dtpm_per_cpu, cpu) = NULL;
 	kfree(dtpm_cpu);
 
-release_policy:
-	cpufreq_cpu_put(policy);
 	return ret;
 }
 
-- 
2.25.1

[PATCH v4 08/10] thermal: imx: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/thermal/imx_thermal.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 38c993d1bcb3..cd1d9d419275 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -201,7 +201,6 @@ static struct thermal_soc_data thermal_imx7d_data = {
 
 struct imx_thermal_data {
 	struct device *dev;
-	struct cpufreq_policy *policy;
 	struct thermal_zone_device *tz;
 	struct thermal_cooling_device *cdev;
 	struct regmap *tempmon;
@@ -541,22 +540,20 @@ MODULE_DEVICE_TABLE(of, of_imx_thermal_match);
 static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
 {
 	struct device_node *np;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
 	int ret = 0;
 
-	data->policy = cpufreq_cpu_get(0);
-	if (!data->policy) {
+	if (!policy) {
 		pr_debug("%s: CPUFreq policy not found\n", __func__);
 		return -EPROBE_DEFER;
 	}
 
-	np = of_get_cpu_node(data->policy->cpu, NULL);
+	np = of_get_cpu_node(policy->cpu, NULL);
 
 	if (!np || !of_property_present(np, "#cooling-cells")) {
-		data->cdev = cpufreq_cooling_register(data->policy);
-		if (IS_ERR(data->cdev)) {
+		data->cdev = cpufreq_cooling_register(policy);
+		if (IS_ERR(data->cdev))
 			ret = PTR_ERR(data->cdev);
-			cpufreq_cpu_put(data->policy);
-		}
 	}
 
 	of_node_put(np);
@@ -567,7 +564,6 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
 static void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data *data)
 {
 	cpufreq_cooling_unregister(data->cdev);
-	cpufreq_cpu_put(data->policy);
 }
 
 #else
-- 
2.25.1

[PATCH v4 09/10] thermal/drivers/ti-soc-thermal: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 0cf0826b805a..37d06468913a 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -27,7 +27,6 @@
 
 /* common data structures */
 struct ti_thermal_data {
-	struct cpufreq_policy *policy;
 	struct thermal_zone_device *ti_thermal;
 	struct thermal_zone_device *pcb_tz;
 	struct thermal_cooling_device *cool_dev;
@@ -218,6 +217,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
 {
 	struct ti_thermal_data *data;
 	struct device_node *np = bgp->dev->of_node;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
 
 	/*
 	 * We are assuming here that if one deploys the zone
@@ -234,19 +234,17 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
 	if (!data)
 		return -EINVAL;
 
-	data->policy = cpufreq_cpu_get(0);
-	if (!data->policy) {
+	if (!policy) {
 		pr_debug("%s: CPUFreq policy not found\n", __func__);
 		return -EPROBE_DEFER;
 	}
 
 	/* Register cooling device */
-	data->cool_dev = cpufreq_cooling_register(data->policy);
+	data->cool_dev = cpufreq_cooling_register(policy);
 	if (IS_ERR(data->cool_dev)) {
 		int ret = PTR_ERR(data->cool_dev);
 		dev_err(bgp->dev, "Failed to register cpu cooling device %d\n",
 			ret);
-		cpufreq_cpu_put(data->policy);
 
 		return ret;
 	}
@@ -261,11 +259,8 @@ int ti_thermal_unregister_cpu_cooling(struct ti_bandgap *bgp, int id)
 
 	data = ti_bandgap_get_sensor_data(bgp, id);
 
-	if (!IS_ERR_OR_NULL(data)) {
+	if (!IS_ERR_OR_NULL(data))
 		cpufreq_cooling_unregister(data->cool_dev);
-		if (data->policy)
-			cpufreq_cpu_put(data->policy);
-	}
 
 	return 0;
 }
-- 
2.25.1

[PATCH v4 10/10] PM: EM: Use scope-based cleanup helper

[Zihuan Zhang]

Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
annotation for policy references. This reduces the risk of reference
counting mistakes and aligns the code with the latest kernel style.

No functional change intended.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 kernel/power/energy_model.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index ea7995a25780..99401678e809 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -451,7 +451,7 @@ static void
 em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
 {
 	struct em_perf_domain *pd = dev->em_pd;
-	struct cpufreq_policy *policy;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
 	int found = 0;
 	int i, cpu;
 
@@ -479,8 +479,6 @@ em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
 			found++;
 	}
 
-	cpufreq_cpu_put(policy);
-
 	if (!found)
 		return;
 
@@ -787,7 +785,7 @@ static void em_check_capacity_update(void)
 
 	/* Check if CPUs capacity has changed than update EM */
 	for_each_possible_cpu(cpu) {
-		struct cpufreq_policy *policy;
+		struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
 		struct em_perf_domain *pd;
 		struct device *dev;
 
@@ -801,7 +799,6 @@ static void em_check_capacity_update(void)
 					      msecs_to_jiffies(1000));
 			break;
 		}
-		cpufreq_cpu_put(policy);
 
 		dev = get_cpu_device(cpu);
 		pd = em_pd_get(dev);
-- 
2.25.1

Re: [PATCH v4 10/10] PM: EM: Use scope-based cleanup helper

[Krzysztof Kozlowski]

On 03/09/2025 15:17, Zihuan Zhang wrote:
> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>  kernel/power/energy_model.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index ea7995a25780..99401678e809 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -451,7 +451,7 @@ static void
>  em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
>  {
>  	struct em_perf_domain *pd = dev->em_pd;
> -	struct cpufreq_policy *policy;
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;

This is not really correct coding style. Please read how to use
cleanup.h expressed in that header. You should have here proper
constructor or this should be moved. Or this should not be __free()...


>  	int found = 0;
>  	int i, cpu;
>  
> @@ -479,8 +479,6 @@ em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
>  			found++;
>  	}
>  
> -	cpufreq_cpu_put(policy);
> -
>  	if (!found)
>  		return;
>  
> @@ -787,7 +785,7 @@ static void em_check_capacity_update(void)
>  
>  	/* Check if CPUs capacity has changed than update EM */
>  	for_each_possible_cpu(cpu) {
> -		struct cpufreq_policy *policy;
> +		struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;

Same problem here.

I don't think you really paid attention to my feedback last time.

Cleanup.h requires knowing what you do, not just blindly adding __free()
here and there.

Best regards,
Krzysztof

Re: [PATCH v4 02/10] ACPI: processor: thermal: Use scope-based cleanup helper

[Rafael J. Wysocki]

On Wed, Sep 3, 2025 at 3:18 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
>
> No functional change intended.
>
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>  drivers/acpi/processor_thermal.c | 37 ++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> index 1219adb11ab9..5043f17d27b7 100644
> --- a/drivers/acpi/processor_thermal.c
> +++ b/drivers/acpi/processor_thermal.c
> @@ -62,19 +62,14 @@ static int phys_package_first_cpu(int cpu)
>         return 0;
>  }
>
> -static int cpu_has_cpufreq(unsigned int cpu)
> +static bool cpu_has_cpufreq(unsigned int cpu)
>  {
> -       struct cpufreq_policy *policy;
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>
>         if (!acpi_processor_cpufreq_init)
>                 return 0;
>
> -       policy = cpufreq_cpu_get(cpu);
> -       if (policy) {
> -               cpufreq_cpu_put(policy);
> -               return 1;
> -       }
> -       return 0;
> +       return !!policy;
>  }
>
>  static int cpufreq_get_max_state(unsigned int cpu)
> @@ -93,9 +88,23 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>         return reduction_step(cpu);
>  }
>
> +static long long cpufreq_get_max_freq(unsigned int cpu)
> +{
> +       long long max_freq;
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +               cpufreq_cpu_get(cpu);
> +
> +       if (!policy)
> +               return -EINVAL;
> +
> +       max_freq = (policy->cpuinfo.max_freq *
> +               (100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
> +
> +       return max_freq;
> +}
> +
>  static int cpufreq_set_cur_state(unsigned int cpu, int state)
>  {
> -       struct cpufreq_policy *policy;
>         struct acpi_processor *pr;
>         unsigned long max_freq;
>         int i, ret;
> @@ -120,14 +129,10 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
>                 if (unlikely(!freq_qos_request_active(&pr->thermal_req)))
>                         continue;
>
> -               policy = cpufreq_cpu_get(i);
> -               if (!policy)
> -                       return -EINVAL;
> -
> -               max_freq = (policy->cpuinfo.max_freq *
> -                           (100 - reduction_step(i) * cpufreq_thermal_reduction_pctg)) / 100;
> +               max_freq = cpufreq_get_max_freq(cpu);
>
> -               cpufreq_cpu_put(policy);
> +               if (max_freq == -EINVAL)
> +                       return -EINVAL;

Please also move the code below to the new function so it does not
need to return a value.

>
>                 ret = freq_qos_update_request(&pr->thermal_req, max_freq);
>                 if (ret < 0) {
> --

Re: [PATCH v4 10/10] PM: EM: Use scope-based cleanup helper

[Rafael J. Wysocki]

On Wed, Sep 3, 2025 at 3:22 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 03/09/2025 15:17, Zihuan Zhang wrote:
> > Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> > annotation for policy references. This reduces the risk of reference
> > counting mistakes and aligns the code with the latest kernel style.
> >
> > No functional change intended.
> >
> > Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> > ---
> >  kernel/power/energy_model.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> > index ea7995a25780..99401678e809 100644
> > --- a/kernel/power/energy_model.c
> > +++ b/kernel/power/energy_model.c
> > @@ -451,7 +451,7 @@ static void
> >  em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
> >  {
> >       struct em_perf_domain *pd = dev->em_pd;
> > -     struct cpufreq_policy *policy;
> > +     struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
>
> This is not really correct coding style. Please read how to use
> cleanup.h expressed in that header. You should have here proper
> constructor or this should be moved. Or this should not be __free()...

I gather that this is what you mean (quoted verbatim from cleanup.h)

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.

and thanks for pointing this out!

Re: [PATCH v4 10/10] PM: EM: Use scope-based cleanup helper

[Krzysztof Kozlowski]

On 03/09/2025 15:41, Rafael J. Wysocki wrote:
>>>  em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
>>>  {
>>>       struct em_perf_domain *pd = dev->em_pd;
>>> -     struct cpufreq_policy *policy;
>>> +     struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
>>
>> This is not really correct coding style. Please read how to use
>> cleanup.h expressed in that header. You should have here proper
>> constructor or this should be moved. Or this should not be __free()...
> 
> I gather that this is what you mean (quoted verbatim from cleanup.h)
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> 
> and thanks for pointing this out!


... and the only exception would be if there is no single constructor,
but multiple (in if() block). That's not the case here, I think.

Best regards,
Krzysztof

Re: [PATCH v4 07/10] powercap: dtpm_cpu: Use scope-based cleanup helper

[Rafael J. Wysocki]

On Wed, Sep 3, 2025 at 3:18 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
>
> No functional change intended.
>
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>  drivers/powercap/dtpm_cpu.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 99390ec1481f..f76594185fa2 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -144,19 +144,17 @@ static int update_pd_power_uw(struct dtpm *dtpm)
>  static void pd_release(struct dtpm *dtpm)
>  {
>         struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
> -       struct cpufreq_policy *policy;
>
>         if (freq_qos_request_active(&dtpm_cpu->qos_req))
>                 freq_qos_remove_request(&dtpm_cpu->qos_req);
>
> -       policy = cpufreq_cpu_get(dtpm_cpu->cpu);
> -       if (policy) {
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +               cpufreq_cpu_get(dtpm_cpu->cpu);
> +
> +       if (policy)
>                 for_each_cpu(dtpm_cpu->cpu, policy->related_cpus)
>                         per_cpu(dtpm_per_cpu, dtpm_cpu->cpu) = NULL;
>
> -               cpufreq_cpu_put(policy);
> -       }
> -
>         kfree(dtpm_cpu);
>  }
>
> @@ -192,7 +190,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>  static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>  {
>         struct dtpm_cpu *dtpm_cpu;
> -       struct cpufreq_policy *policy;
>         struct em_perf_state *table;
>         struct em_perf_domain *pd;
>         char name[CPUFREQ_NAME_LEN];
> @@ -202,21 +199,19 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>         if (dtpm_cpu)
>                 return 0;
>
> -       policy = cpufreq_cpu_get(cpu);
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +               cpufreq_cpu_get(cpu);
> +
>         if (!policy)
>                 return 0;
>
>         pd = em_cpu_get(cpu);
> -       if (!pd || em_is_artificial(pd)) {
> -               ret = -EINVAL;
> -               goto release_policy;
> -       }
> +       if (!pd || em_is_artificial(pd))
> +               return -EINVAL;
>
>         dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
> -       if (!dtpm_cpu) {
> -               ret = -ENOMEM;
> -               goto release_policy;
> -       }
> +       if (!dtpm_cpu)
> +               return -ENOMEM;
>
>         dtpm_init(&dtpm_cpu->dtpm, &dtpm_ops);
>         dtpm_cpu->cpu = cpu;
> @@ -239,7 +234,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>         if (ret < 0)
>                 goto out_dtpm_unregister;

So this change kind of goes against another recommendation given in cleanup.h:

 * Lastly, given that the benefit of cleanup helpers is removal of
 * "goto", and that the "goto" statement can jump between scopes, the
 * expectation is that usage of "goto" and cleanup helpers is never
 * mixed in the same function. I.e. for a given routine, convert all
 * resources that need a "goto" cleanup to scope-based cleanup, or
 * convert none of them.

>
> -       cpufreq_cpu_put(policy);
>         return 0;
>
>  out_dtpm_unregister:
> @@ -251,8 +245,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>                 per_cpu(dtpm_per_cpu, cpu) = NULL;
>         kfree(dtpm_cpu);
>
> -release_policy:
> -       cpufreq_cpu_put(policy);
>         return ret;
>  }
>
> --

Re: [PATCH v4 10/10] PM: EM: Use scope-based cleanup helper

[Zihuan Zhang]

在 2025/9/3 21:43, Krzysztof Kozlowski 写道:
> On 03/09/2025 15:41, Rafael J. Wysocki wrote:
>>>>   em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
>>>>   {
>>>>        struct em_perf_domain *pd = dev->em_pd;
>>>> -     struct cpufreq_policy *policy;
>>>> +     struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
>>> This is not really correct coding style. Please read how to use
>>> cleanup.h expressed in that header. You should have here proper
>>> constructor or this should be moved. Or this should not be __free()...
>> I gather that this is what you mean (quoted verbatim from cleanup.h)
>>
>>   * Given that the "__free(...) = NULL" pattern for variables defined at
>>   * the top of the function poses this potential interdependency problem
>>   * the recommendation is to always define and assign variables in one
>>   * statement and not group variable definitions at the top of the
>>   * function when __free() is used.
>>
>> and thanks for pointing this out!
>
> ... and the only exception would be if there is no single constructor,
> but multiple (in if() block). That's not the case here, I think.
>
> Best regards,
> Krzysztof


Sorry, I didn’t fully understand this earlier. In v3 I split the
definition and assignment mainly because the CPU value was obtained
later, so I thought I couldn’t initialize it in one go at the top of
the function. Honestly, it was also for “prettier” style.

After looking at the code Rafael just committed, I realized I can
simply define and assign the variable later in one line, without
needing to separate them. I’ll fix this in the next version.

Thanks for pointing it out!

Re: [PATCH v4 07/10] powercap: dtpm_cpu: Use scope-based cleanup helper

[Zihuan Zhang]

在 2025/9/3 21:45, Rafael J. Wysocki 写道:
> On Wed, Sep 3, 2025 at 3:18 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
>> annotation for policy references. This reduces the risk of reference
>> counting mistakes and aligns the code with the latest kernel style.
>>
>> No functional change intended.
>>
>> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
>> ---
>>   drivers/powercap/dtpm_cpu.c | 30 +++++++++++-------------------
>>   1 file changed, 11 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
>> index 99390ec1481f..f76594185fa2 100644
>> --- a/drivers/powercap/dtpm_cpu.c
>> +++ b/drivers/powercap/dtpm_cpu.c
>> @@ -144,19 +144,17 @@ static int update_pd_power_uw(struct dtpm *dtpm)
>>   static void pd_release(struct dtpm *dtpm)
>>   {
>>          struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
>> -       struct cpufreq_policy *policy;
>>
>>          if (freq_qos_request_active(&dtpm_cpu->qos_req))
>>                  freq_qos_remove_request(&dtpm_cpu->qos_req);
>>
>> -       policy = cpufreq_cpu_get(dtpm_cpu->cpu);
>> -       if (policy) {
>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
>> +               cpufreq_cpu_get(dtpm_cpu->cpu);
>> +
>> +       if (policy)
>>                  for_each_cpu(dtpm_cpu->cpu, policy->related_cpus)
>>                          per_cpu(dtpm_per_cpu, dtpm_cpu->cpu) = NULL;
>>
>> -               cpufreq_cpu_put(policy);
>> -       }
>> -
>>          kfree(dtpm_cpu);
>>   }
>>
>> @@ -192,7 +190,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>>   static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>>   {
>>          struct dtpm_cpu *dtpm_cpu;
>> -       struct cpufreq_policy *policy;
>>          struct em_perf_state *table;
>>          struct em_perf_domain *pd;
>>          char name[CPUFREQ_NAME_LEN];
>> @@ -202,21 +199,19 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>>          if (dtpm_cpu)
>>                  return 0;
>>
>> -       policy = cpufreq_cpu_get(cpu);
>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
>> +               cpufreq_cpu_get(cpu);
>> +
>>          if (!policy)
>>                  return 0;
>>
>>          pd = em_cpu_get(cpu);
>> -       if (!pd || em_is_artificial(pd)) {
>> -               ret = -EINVAL;
>> -               goto release_policy;
>> -       }
>> +       if (!pd || em_is_artificial(pd))
>> +               return -EINVAL;
>>
>>          dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
>> -       if (!dtpm_cpu) {
>> -               ret = -ENOMEM;
>> -               goto release_policy;
>> -       }
>> +       if (!dtpm_cpu)
>> +               return -ENOMEM;
>>
>>          dtpm_init(&dtpm_cpu->dtpm, &dtpm_ops);
>>          dtpm_cpu->cpu = cpu;
>> @@ -239,7 +234,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>>          if (ret < 0)
>>                  goto out_dtpm_unregister;
> So this change kind of goes against another recommendation given in cleanup.h:
>
>   * Lastly, given that the benefit of cleanup helpers is removal of
>   * "goto", and that the "goto" statement can jump between scopes, the
>   * expectation is that usage of "goto" and cleanup helpers is never
>   * mixed in the same function. I.e. for a given routine, convert all
>   * resources that need a "goto" cleanup to scope-based cleanup, or
>   * convert none of them.


Should I replace all the memory allocation cleanups here with `__free`?
That would allow us to drop all the `goto`s, but since this function has
quite a few of them, I’m concerned it might introduce new issues. What’s
your recommendation?

Thanks!

>> -       cpufreq_cpu_put(policy);
>>          return 0;
>>
>>   out_dtpm_unregister:
>> @@ -251,8 +245,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>>                  per_cpu(dtpm_per_cpu, cpu) = NULL;
>>          kfree(dtpm_cpu);
>>
>> -release_policy:
>> -       cpufreq_cpu_put(policy);
>>          return ret;
>>   }
>>
>> --

Re: [PATCH v4 07/10] powercap: dtpm_cpu: Use scope-based cleanup helper

[Krzysztof Kozlowski]

On 04/09/2025 12:37, Zihuan Zhang wrote:
>>   * Lastly, given that the benefit of cleanup helpers is removal of
>>   * "goto", and that the "goto" statement can jump between scopes, the
>>   * expectation is that usage of "goto" and cleanup helpers is never
>>   * mixed in the same function. I.e. for a given routine, convert all
>>   * resources that need a "goto" cleanup to scope-based cleanup, or
>>   * convert none of them.
> 
> 
> Should I replace all the memory allocation cleanups here with `__free`?
> That would allow us to drop all the `goto`s, but since this function has
> quite a few of them, I’m concerned it might introduce new issues. What’s
> your recommendation?

If you keep asking this, I have doubts you really know how to use
cleanup.h. Don't blindly convert code to cleanup.h. It's very odd syntax
and it is not even welcomed everywhere.

Best regards,
Krzysztof

Re: [PATCH v4 07/10] powercap: dtpm_cpu: Use scope-based cleanup helper

[Rafael J. Wysocki]

On Thu, Sep 4, 2025 at 12:38 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
>
> 在 2025/9/3 21:45, Rafael J. Wysocki 写道:
> > On Wed, Sep 3, 2025 at 3:18 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
> >> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> >> annotation for policy references. This reduces the risk of reference
> >> counting mistakes and aligns the code with the latest kernel style.
> >>
> >> No functional change intended.
> >>
> >> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> >> ---
> >>   drivers/powercap/dtpm_cpu.c | 30 +++++++++++-------------------
> >>   1 file changed, 11 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> >> index 99390ec1481f..f76594185fa2 100644
> >> --- a/drivers/powercap/dtpm_cpu.c
> >> +++ b/drivers/powercap/dtpm_cpu.c
> >> @@ -144,19 +144,17 @@ static int update_pd_power_uw(struct dtpm *dtpm)
> >>   static void pd_release(struct dtpm *dtpm)
> >>   {
> >>          struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
> >> -       struct cpufreq_policy *policy;
> >>
> >>          if (freq_qos_request_active(&dtpm_cpu->qos_req))
> >>                  freq_qos_remove_request(&dtpm_cpu->qos_req);
> >>
> >> -       policy = cpufreq_cpu_get(dtpm_cpu->cpu);
> >> -       if (policy) {
> >> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> >> +               cpufreq_cpu_get(dtpm_cpu->cpu);
> >> +
> >> +       if (policy)
> >>                  for_each_cpu(dtpm_cpu->cpu, policy->related_cpus)
> >>                          per_cpu(dtpm_per_cpu, dtpm_cpu->cpu) = NULL;
> >>
> >> -               cpufreq_cpu_put(policy);
> >> -       }
> >> -
> >>          kfree(dtpm_cpu);
> >>   }
> >>
> >> @@ -192,7 +190,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> >>   static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
> >>   {
> >>          struct dtpm_cpu *dtpm_cpu;
> >> -       struct cpufreq_policy *policy;
> >>          struct em_perf_state *table;
> >>          struct em_perf_domain *pd;
> >>          char name[CPUFREQ_NAME_LEN];
> >> @@ -202,21 +199,19 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
> >>          if (dtpm_cpu)
> >>                  return 0;
> >>
> >> -       policy = cpufreq_cpu_get(cpu);
> >> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> >> +               cpufreq_cpu_get(cpu);
> >> +
> >>          if (!policy)
> >>                  return 0;
> >>
> >>          pd = em_cpu_get(cpu);
> >> -       if (!pd || em_is_artificial(pd)) {
> >> -               ret = -EINVAL;
> >> -               goto release_policy;
> >> -       }
> >> +       if (!pd || em_is_artificial(pd))
> >> +               return -EINVAL;
> >>
> >>          dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
> >> -       if (!dtpm_cpu) {
> >> -               ret = -ENOMEM;
> >> -               goto release_policy;
> >> -       }
> >> +       if (!dtpm_cpu)
> >> +               return -ENOMEM;
> >>
> >>          dtpm_init(&dtpm_cpu->dtpm, &dtpm_ops);
> >>          dtpm_cpu->cpu = cpu;
> >> @@ -239,7 +234,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
> >>          if (ret < 0)
> >>                  goto out_dtpm_unregister;
> > So this change kind of goes against another recommendation given in cleanup.h:
> >
> >   * Lastly, given that the benefit of cleanup helpers is removal of
> >   * "goto", and that the "goto" statement can jump between scopes, the
> >   * expectation is that usage of "goto" and cleanup helpers is never
> >   * mixed in the same function. I.e. for a given routine, convert all
> >   * resources that need a "goto" cleanup to scope-based cleanup, or
> >   * convert none of them.
>
>
> Should I replace all the memory allocation cleanups here with `__free`?
> That would allow us to drop all the `goto`s, but since this function has
> quite a few of them, I’m concerned it might introduce new issues. What’s
> your recommendation?

Frankly, don't use __free() in this code at all, at least for the time being.

There is a problem with dropping the reference to policy at the end of
__dtpm_cpu_setup() because that policy may be subsequently indirectly
used in set_pd_power_limit() which calls
freq_qos_update_request(&dtpm_cpu->qos_req, freq) and
dtpm_cpu->qos_req->qos is policy->constraints, so using it will cause
policy->constraints to be dereferenced in freq_qos_apply() which will
crash and burn if the policy goes away in the meantime.  So AFAICS
__dtpm_cpu_setup() shouldn't call cpufreq_cpu_put() at all and the
policy should be released in pd_release() without acquiring a new
reference to it.

You may as well try to fix this if you have free cycles.

Re: [PATCH v4 09/10] thermal/drivers/ti-soc-thermal: Use scope-based cleanup helper

[Andreas Kemnade]

Am Wed,  3 Sep 2025 21:17:32 +0800
schrieb Zihuan Zhang <zhangzihuan@kylinos.cn>:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 0cf0826b805a..37d06468913a 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -27,7 +27,6 @@
>  
>  /* common data structures */
>  struct ti_thermal_data {
> -	struct cpufreq_policy *policy;
>  	struct thermal_zone_device *ti_thermal;
>  	struct thermal_zone_device *pcb_tz;
>  	struct thermal_cooling_device *cool_dev;
> @@ -218,6 +217,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
>  {
>  	struct ti_thermal_data *data;
>  	struct device_node *np = bgp->dev->of_node;
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
>  
this looks as it changes the lifecycle from the device lifetime to just
this function...

>  	/*
>  	 * We are assuming here that if one deploys the zone
> @@ -234,19 +234,17 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
>  	if (!data)
>  		return -EINVAL;
>  
> -	data->policy = cpufreq_cpu_get(0);
> -	if (!data->policy) {
> +	if (!policy) {
>  		pr_debug("%s: CPUFreq policy not found\n", __func__);
>  		return -EPROBE_DEFER;
>  	}
>  
>  	/* Register cooling device */
> -	data->cool_dev = cpufreq_cooling_register(data->policy);
> +	data->cool_dev = cpufreq_cooling_register(policy);

and it is passed on to something living beyond this function. I see no
_get(policy) in cpufreq_cooling_register().
Am I missing something?

Regards,
Andreas

Re: [PATCH v4 09/10] thermal/drivers/ti-soc-thermal: Use scope-based cleanup helper

[Krzysztof Kozlowski]

On 05/09/2025 08:57, Andreas Kemnade wrote:
> Am Wed,  3 Sep 2025 21:17:32 +0800
> schrieb Zihuan Zhang <zhangzihuan@kylinos.cn>:
> 
>> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
>> annotation for policy references. This reduces the risk of reference
>> counting mistakes and aligns the code with the latest kernel style.
>>
>> No functional change intended.
>>
>> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
>> ---
>>  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> index 0cf0826b805a..37d06468913a 100644
>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> @@ -27,7 +27,6 @@
>>  
>>  /* common data structures */
>>  struct ti_thermal_data {
>> -	struct cpufreq_policy *policy;
>>  	struct thermal_zone_device *ti_thermal;
>>  	struct thermal_zone_device *pcb_tz;
>>  	struct thermal_cooling_device *cool_dev;
>> @@ -218,6 +217,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
>>  {
>>  	struct ti_thermal_data *data;
>>  	struct device_node *np = bgp->dev->of_node;
>> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
>>  
> this looks as it changes the lifecycle from the device lifetime to just
> this function...
> 
>>  	/*
>>  	 * We are assuming here that if one deploys the zone
>> @@ -234,19 +234,17 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
>>  	if (!data)
>>  		return -EINVAL;
>>  
>> -	data->policy = cpufreq_cpu_get(0);
>> -	if (!data->policy) {
>> +	if (!policy) {
>>  		pr_debug("%s: CPUFreq policy not found\n", __func__);
>>  		return -EPROBE_DEFER;
>>  	}
>>  
>>  	/* Register cooling device */
>> -	data->cool_dev = cpufreq_cooling_register(data->policy);
>> +	data->cool_dev = cpufreq_cooling_register(policy);
> 
> and it is passed on to something living beyond this function. I see no
> _get(policy) in cpufreq_cooling_register().
> Am I missing something?
Yeah, " No functional change intended." is clearly incorrect.

I already commented on this series that it is very bad and author should
does not understand how cleanup.h works, and here is another example.

IMO, entire patchset should be dropped.

Best regards,
Krzysztof

Re: [PATCH v4 07/10] powercap: dtpm_cpu: Use scope-based cleanup helper

[Zihuan Zhang]

在 2025/9/4 21:17, Rafael J. Wysocki 写道:
> On Thu, Sep 4, 2025 at 12:38 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>>
>> 在 2025/9/3 21:45, Rafael J. Wysocki 写道:
>>> On Wed, Sep 3, 2025 at 3:18 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>>>> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
>>>> annotation for policy references. This reduces the risk of reference
>>>> counting mistakes and aligns the code with the latest kernel style.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
>>>> ---
>>>>    drivers/powercap/dtpm_cpu.c | 30 +++++++++++-------------------
>>>>    1 file changed, 11 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
>>>> index 99390ec1481f..f76594185fa2 100644
>>>> --- a/drivers/powercap/dtpm_cpu.c
>>>> +++ b/drivers/powercap/dtpm_cpu.c
>>>> @@ -144,19 +144,17 @@ static int update_pd_power_uw(struct dtpm *dtpm)
>>>>    static void pd_release(struct dtpm *dtpm)
>>>>    {
>>>>           struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
>>>> -       struct cpufreq_policy *policy;
>>>>
>>>>           if (freq_qos_request_active(&dtpm_cpu->qos_req))
>>>>                   freq_qos_remove_request(&dtpm_cpu->qos_req);
>>>>
>>>> -       policy = cpufreq_cpu_get(dtpm_cpu->cpu);
>>>> -       if (policy) {
>>>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
>>>> +               cpufreq_cpu_get(dtpm_cpu->cpu);
>>>> +
>>>> +       if (policy)
>>>>                   for_each_cpu(dtpm_cpu->cpu, policy->related_cpus)
>>>>                           per_cpu(dtpm_per_cpu, dtpm_cpu->cpu) = NULL;
>>>>
>>>> -               cpufreq_cpu_put(policy);
>>>> -       }
>>>> -
>>>>           kfree(dtpm_cpu);
>>>>    }
>>>>
>>>> @@ -192,7 +190,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>>>>    static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>>>>    {
>>>>           struct dtpm_cpu *dtpm_cpu;
>>>> -       struct cpufreq_policy *policy;
>>>>           struct em_perf_state *table;
>>>>           struct em_perf_domain *pd;
>>>>           char name[CPUFREQ_NAME_LEN];
>>>> @@ -202,21 +199,19 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>>>>           if (dtpm_cpu)
>>>>                   return 0;
>>>>
>>>> -       policy = cpufreq_cpu_get(cpu);
>>>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
>>>> +               cpufreq_cpu_get(cpu);
>>>> +
>>>>           if (!policy)
>>>>                   return 0;
>>>>
>>>>           pd = em_cpu_get(cpu);
>>>> -       if (!pd || em_is_artificial(pd)) {
>>>> -               ret = -EINVAL;
>>>> -               goto release_policy;
>>>> -       }
>>>> +       if (!pd || em_is_artificial(pd))
>>>> +               return -EINVAL;
>>>>
>>>>           dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
>>>> -       if (!dtpm_cpu) {
>>>> -               ret = -ENOMEM;
>>>> -               goto release_policy;
>>>> -       }
>>>> +       if (!dtpm_cpu)
>>>> +               return -ENOMEM;
>>>>
>>>>           dtpm_init(&dtpm_cpu->dtpm, &dtpm_ops);
>>>>           dtpm_cpu->cpu = cpu;
>>>> @@ -239,7 +234,6 @@ static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>>>>           if (ret < 0)
>>>>                   goto out_dtpm_unregister;
>>> So this change kind of goes against another recommendation given in cleanup.h:
>>>
>>>    * Lastly, given that the benefit of cleanup helpers is removal of
>>>    * "goto", and that the "goto" statement can jump between scopes, the
>>>    * expectation is that usage of "goto" and cleanup helpers is never
>>>    * mixed in the same function. I.e. for a given routine, convert all
>>>    * resources that need a "goto" cleanup to scope-based cleanup, or
>>>    * convert none of them.
>>
>> Should I replace all the memory allocation cleanups here with `__free`?
>> That would allow us to drop all the `goto`s, but since this function has
>> quite a few of them, I’m concerned it might introduce new issues. What’s
>> your recommendation?
> Frankly, don't use __free() in this code at all, at least for the time being.
>
> There is a problem with dropping the reference to policy at the end of
> __dtpm_cpu_setup() because that policy may be subsequently indirectly
> used in set_pd_power_limit() which calls
> freq_qos_update_request(&dtpm_cpu->qos_req, freq) and
> dtpm_cpu->qos_req->qos is policy->constraints, so using it will cause
> policy->constraints to be dereferenced in freq_qos_apply() which will
> crash and burn if the policy goes away in the meantime.  So AFAICS
> __dtpm_cpu_setup() shouldn't call cpufreq_cpu_put() at all and the
> policy should be released in pd_release() without acquiring a new
> reference to it.
>

Thanks a lot for the detailed explanation!

Re: [PATCH v4 09/10] thermal/drivers/ti-soc-thermal: Use scope-based cleanup helper

[Zihuan Zhang]

在 2025/9/5 14:57, Andreas Kemnade 写道:
> Am Wed,  3 Sep 2025 21:17:32 +0800
> schrieb Zihuan Zhang <zhangzihuan@kylinos.cn>:
>
>> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
>> annotation for policy references. This reduces the risk of reference
>> counting mistakes and aligns the code with the latest kernel style.
>>
>> No functional change intended.
>>
>> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
>> ---
>>   drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> index 0cf0826b805a..37d06468913a 100644
>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> @@ -27,7 +27,6 @@
>>   
>>   /* common data structures */
>>   struct ti_thermal_data {
>> -	struct cpufreq_policy *policy;
>>   	struct thermal_zone_device *ti_thermal;
>>   	struct thermal_zone_device *pcb_tz;
>>   	struct thermal_cooling_device *cool_dev;
>> @@ -218,6 +217,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
>>   {
>>   	struct ti_thermal_data *data;
>>   	struct device_node *np = bgp->dev->of_node;
>> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
>>   
> this looks as it changes the lifecycle from the device lifetime to just
> this function...


I thought policy was only used in this function, so I moved it here.

Thanks for clarifying the lifecycle issue.

>>   	/*
>>   	 * We are assuming here that if one deploys the zone
>> @@ -234,19 +234,17 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id)
>>   	if (!data)
>>   		return -EINVAL;
>>   
>> -	data->policy = cpufreq_cpu_get(0);
>> -	if (!data->policy) {
>> +	if (!policy) {
>>   		pr_debug("%s: CPUFreq policy not found\n", __func__);
>>   		return -EPROBE_DEFER;
>>   	}
>>   
>>   	/* Register cooling device */
>> -	data->cool_dev = cpufreq_cooling_register(data->policy);
>> +	data->cool_dev = cpufreq_cooling_register(policy);
> and it is passed on to something living beyond this function. I see no
> _get(policy) in cpufreq_cooling_register().
> Am I missing something?

This indeed causes a problem.

Sure,  I will drop the patchset.

Thanks!

> Regards,
> Andreas

Re: [PATCH v4 01/10] arm64: topology: Use scope-based cleanup helper

[Jonathan Cameron]

On Wed,  3 Sep 2025 21:17:24 +0800
Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
Diff is being slightly annoying in not quite showing enough context
to see that it is fine to release this at the end of scope.
What you have done looks correct to me but there is a further
potential cleanup

	for_each_cpu_wrap(ref_cpu, policy->cpus, cpu + 1) {
		if (ref_cpu == start_cpu) {
			/* Prevent verifying same CPU twice */
			ref_cpu = nr_cpu_ids;
			break;
//here, might as well return -EAGAIN as only thing in this block
//and now no advantage in sharing the exit path with the one that
//says we never matched the next condition.
		}
		if (!idle_cpu(ref_cpu))
			break;
		}

Either way I'm fine with this.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  arch/arm64/kernel/topology.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 5d07ee85bdae..0eebd3492669 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -307,17 +307,16 @@ int arch_freq_get_on_cpu(int cpu)
>  		 */
>  		if (!housekeeping_cpu(cpu, HK_TYPE_TICK) ||
>  		    time_is_before_jiffies(last_update + msecs_to_jiffies(AMU_SAMPLE_EXP_MS))) {
> -			struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +			struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +				cpufreq_cpu_get(cpu);
>  			int ref_cpu;
>  
>  			if (!policy)
>  				return -EINVAL;
>  
>  			if (!cpumask_intersects(policy->related_cpus,
> -						housekeeping_cpumask(HK_TYPE_TICK))) {
> -				cpufreq_cpu_put(policy);
> +						housekeeping_cpumask(HK_TYPE_TICK)))
>  				return -EOPNOTSUPP;
> -			}
>  
>  			for_each_cpu_wrap(ref_cpu, policy->cpus, cpu + 1) {
>  				if (ref_cpu == start_cpu) {
> @@ -329,8 +328,6 @@ int arch_freq_get_on_cpu(int cpu)
>  					break;
>  			}
>  
> -			cpufreq_cpu_put(policy);
> -
>  			if (ref_cpu >= nr_cpu_ids)
>  				/* No alternative to pull info from */
>  				return -EAGAIN;

Re: [PATCH v4 02/10] ACPI: processor: thermal: Use scope-based cleanup helper

[Jonathan Cameron]

On Wed, 3 Sep 2025 15:23:31 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Wed, Sep 3, 2025 at 3:18 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
> >
> > Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> > annotation for policy references. This reduces the risk of reference
> > counting mistakes and aligns the code with the latest kernel style.
> >
> > No functional change intended.
> >
> > Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> > ---
> >  drivers/acpi/processor_thermal.c | 37 ++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> > index 1219adb11ab9..5043f17d27b7 100644
> > --- a/drivers/acpi/processor_thermal.c
> > +++ b/drivers/acpi/processor_thermal.c
> > @@ -62,19 +62,14 @@ static int phys_package_first_cpu(int cpu)
> >         return 0;
> >  }
> >
> > -static int cpu_has_cpufreq(unsigned int cpu)
> > +static bool cpu_has_cpufreq(unsigned int cpu)
> >  {
> > -       struct cpufreq_policy *policy;
> > +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);

I'd put the order back as it was.  See docs in cleanup.h, it is fine to
declare local variables inline if they are being use with __free()

That way if the simple check on acpi_process_cpu_freq_init fails no
get needs to occur.

So something like

static bool cpu_has_cpufreq(unsigned int cpu)
{
	if (!acpi_processor_cpufreq_init)
                 return 0;

	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);

	return policy != NULL; //Personally I find !! on a pointer a bit weird :)
}

> >
> >         if (!acpi_processor_cpufreq_init)
> >                 return 0;
> >
> > -       policy = cpufreq_cpu_get(cpu);
> > -       if (policy) {
> > -               cpufreq_cpu_put(policy);
> > -               return 1;
> > -       }
> > -       return 0;
> > +       return !!policy;
> >  }
> >
> >  static int cpufreq_get_max_state(unsigned int cpu)
> > @@ -93,9 +88,23 @@ static int cpufreq_get_cur_state(unsigned int cpu)
> >         return reduction_step(cpu);
> >  }
> >
> > +static long long cpufreq_get_max_freq(unsigned int cpu)
> > +{
> > +       long long max_freq;
> > +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> > +               cpufreq_cpu_get(cpu);

Format consistently.  If you are going to wrap to 80 chars here
then do it for the cpu_has_cpufreq() line that is identical to this.

> > +
> > +       if (!policy)
> > +               return -EINVAL;
> > +
> > +       max_freq = (policy->cpuinfo.max_freq *
> > +               (100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
> > +
> > +       return max_freq;
> > +}
> > +
> >  static int cpufreq_set_cur_state(unsigned int cpu, int state)
> >  {
> > -       struct cpufreq_policy *policy;
> >         struct acpi_processor *pr;
> >         unsigned long max_freq;
> >         int i, ret;
> > @@ -120,14 +129,10 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
> >                 if (unlikely(!freq_qos_request_active(&pr->thermal_req)))
> >                         continue;
> >
> > -               policy = cpufreq_cpu_get(i);
> > -               if (!policy)
> > -                       return -EINVAL;
> > -
> > -               max_freq = (policy->cpuinfo.max_freq *
> > -                           (100 - reduction_step(i) * cpufreq_thermal_reduction_pctg)) / 100;
> > +               max_freq = cpufreq_get_max_freq(cpu);
> >
> > -               cpufreq_cpu_put(policy);
> > +               if (max_freq == -EINVAL)
> > +                       return -EINVAL;  
> 
> Please also move the code below to the new function so it does not
> need to return a value.
> 
> >
> >                 ret = freq_qos_update_request(&pr->thermal_req, max_freq);
> >                 if (ret < 0) {
> > --  
> 

Re: [PATCH v4 03/10] cpufreq: intel_pstate: Use scope-based cleanup helper

[Jonathan Cameron]

On Wed,  3 Sep 2025 21:17:26 +0800
Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>

One trivial comment inline.  This LGTM
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  drivers/cpufreq/intel_pstate.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index f366d35c5840..925efb1e65be 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1209,6 +1209,17 @@ static bool hybrid_clear_max_perf_cpu(void)
>  	return ret;
>  }
>  
> +static struct freq_qos_request *intel_pstate_cpufreq_get_req(int cpu)
> +{
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +		cpufreq_cpu_get(cpu);
> +
> +	if (!policy)
> +		return NULL;
> +
> +	return policy->driver_data;
> +}
> +
>  static void __intel_pstate_get_hwp_cap(struct cpudata *cpu)
>  {
>  	u64 cap;
> @@ -1698,19 +1709,13 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b,
>  static void update_qos_request(enum freq_qos_req_type type)
>  {
>  	struct freq_qos_request *req;
> -	struct cpufreq_policy *policy;
>  	int i;
>  
>  	for_each_possible_cpu(i) {
>  		struct cpudata *cpu = all_cpu_data[i];
>  		unsigned int freq, perf_pct;
>  
> -		policy = cpufreq_cpu_get(i);
> -		if (!policy)
> -			continue;
> -
> -		req = policy->driver_data;
> -		cpufreq_cpu_put(policy);
> +		req = intel_pstate_cpufreq_get_req(i);
>  

I'd drop this blank line as it'll keep the setting or req and checking it
closely coupled.  Previously the put inbetween stopped that but now
we can improve the formatting!

>  		if (!req)
>  			continue;

Re: [PATCH v4 03/10] cpufreq: intel_pstate: Use scope-based cleanup helper

[Rafael J. Wysocki]

On Wed, Sep 3, 2025 at 3:18 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
>
> No functional change intended.
>
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>  drivers/cpufreq/intel_pstate.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index f366d35c5840..925efb1e65be 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1209,6 +1209,17 @@ static bool hybrid_clear_max_perf_cpu(void)
>         return ret;
>  }
>
> +static struct freq_qos_request *intel_pstate_cpufreq_get_req(int cpu)
> +{
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +               cpufreq_cpu_get(cpu);
> +
> +       if (!policy)
> +               return NULL;
> +
> +       return policy->driver_data;
> +}
> +
>  static void __intel_pstate_get_hwp_cap(struct cpudata *cpu)
>  {
>         u64 cap;
> @@ -1698,19 +1709,13 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b,
>  static void update_qos_request(enum freq_qos_req_type type)
>  {
>         struct freq_qos_request *req;
> -       struct cpufreq_policy *policy;
>         int i;
>
>         for_each_possible_cpu(i) {
>                 struct cpudata *cpu = all_cpu_data[i];
>                 unsigned int freq, perf_pct;
>
> -               policy = cpufreq_cpu_get(i);
> -               if (!policy)
> -                       continue;
> -
> -               req = policy->driver_data;
> -               cpufreq_cpu_put(policy);
> +               req = intel_pstate_cpufreq_get_req(i);
>
>                 if (!req)
>                         continue;
> --

This change would have been fine if the modified code had been
correct.  Unfortunately, this is not the case because below req is
passed to freq_qos_update_request() which accesses the policy object
indirectly through it, so the policy object should be
reference-counted until freq_qos_update_request() completes.

I will send a fix for this shortly (it is better to fix it before
rearranging the code to allow the fix to be backported more easily)
along with my version of the change in question.

Thanks!

Re: [PATCH v4 04/10] cpufreq: powernv: Use scope-based cleanup helper

[Jonathan Cameron]

On Wed,  3 Sep 2025 21:17:27 +0800
Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
This one is a little more weak wrt to justification.  Kernel style
isn't to use __free() where there isn't a significant benefit.

Up to maintainer of the driver on this one so I'll not offer
a tag.

> ---
>  drivers/cpufreq/powernv-cpufreq.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 7d9a5f656de8..811fdbf398fa 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -892,15 +892,15 @@ static int powernv_cpufreq_reboot_notifier(struct notifier_block *nb,
>  				unsigned long action, void *unused)
>  {
>  	int cpu;
> -	struct cpufreq_policy *cpu_policy;
>  
>  	rebooting = true;
>  	for_each_online_cpu(cpu) {
> -		cpu_policy = cpufreq_cpu_get(cpu);
> +		struct cpufreq_policy *cpu_policy __free(put_cpufreq_policy) =
> +			cpufreq_cpu_get(cpu);
> +
>  		if (!cpu_policy)
>  			continue;
>  		powernv_cpufreq_target_index(cpu_policy, get_nominal_index());
> -		cpufreq_cpu_put(cpu_policy);
>  	}
>  
>  	return NOTIFY_DONE;
> @@ -913,7 +913,6 @@ static struct notifier_block powernv_cpufreq_reboot_nb = {
>  static void powernv_cpufreq_work_fn(struct work_struct *work)
>  {
>  	struct chip *chip = container_of(work, struct chip, throttle);
> -	struct cpufreq_policy *policy;
>  	unsigned int cpu;
>  	cpumask_t mask;
>  
> @@ -928,14 +927,14 @@ static void powernv_cpufreq_work_fn(struct work_struct *work)
>  	chip->restore = false;
>  	for_each_cpu(cpu, &mask) {
>  		int index;
> +		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +			cpufreq_cpu_get(cpu);
>  
> -		policy = cpufreq_cpu_get(cpu);
>  		if (!policy)
>  			continue;
>  		index = cpufreq_table_find_index_c(policy, policy->cur, false);
>  		powernv_cpufreq_target_index(policy, index);
>  		cpumask_andnot(&mask, &mask, policy->cpus);
> -		cpufreq_cpu_put(policy);
>  	}
>  out:
>  	cpus_read_unlock();

Re: [PATCH v4 05/10] PM / devfreq: Use scope-based cleanup helper

[Jonathan Cameron]

On Wed,  3 Sep 2025 21:17:28 +0800
Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>

This falls into the mess of mixing gotos with cleanup.h usage.

The guidance in cleanup.h IIRC say don't do this.  It isn't (I think) buggy here
but it does make things harder to reason about and generally removes
the point of doing __free.  So I think if you are going to do this one
you need to do it fully which is a little more complex.
Need to deal with parent_cpu_data which isn't that hard.

If you mix the two, Linus may get grumpy!

> ---
>  drivers/devfreq/governor_passive.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 953cf9a1e9f7..a035cf44bdb8 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c

> @@ -256,7 +253,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  	struct device *dev = devfreq->dev.parent;
>  	struct opp_table *opp_table = NULL;
>  	struct devfreq_cpu_data *parent_cpu_data;
> -	struct cpufreq_policy *policy;
>  	struct device *cpu_dev;
>  	unsigned int cpu;
>  	int ret;
> @@ -273,23 +269,23 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  	}
>  
>  	for_each_possible_cpu(cpu) {
> -		policy = cpufreq_cpu_get(cpu);
> +		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +			cpufreq_cpu_get(cpu);
> +
>  		if (!policy) {
>  			ret = -EPROBE_DEFER;
>  			goto err;
Return directly here (and after changes below, in all error paths.
>  		}
>  
>  		parent_cpu_data = get_parent_cpu_data(p_data, policy);
> -		if (parent_cpu_data) {
> -			cpufreq_cpu_put(policy);
> +		if (parent_cpu_data)
>  			continue;
This is the first use of parent_cpu_data. If it's set at this point
we don't use it at all.  So step 1. Rename this to split this
use from the one that follows.


> -		}
>  
>  		parent_cpu_data = kzalloc(sizeof(*parent_cpu_data),
>  						GFP_KERNEL);
This one needs to be
		struct devfreq_cpu_data *parent_cpu_data __free(kfree) =
			kzalloc(sizeof(*parent_cpu_data), GFP_KERNEL);

		
>  		if (!parent_cpu_data) {
>  			ret = -ENOMEM;
> -			goto err_put_policy;
> +			goto err;
>  		}
>  
>  		cpu_dev = get_cpu_device(cpu);
> @@ -314,7 +310,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  		parent_cpu_data->max_freq = policy->cpuinfo.max_freq;
>  
>  		list_add_tail(&parent_cpu_data->node, &p_data->cpu_data_list);

then here we need to ensure we don't free parent_cpu_data. Hence

		list_add_tail(&(no_free_ptr(parent_cpu_data)->node,
			      &p_data->cpu_data_list);

That that point we have passed ownership of the data to the list.

> -		cpufreq_cpu_put(policy);
>  	}
>  
>  	mutex_lock(&devfreq->lock);
> @@ -327,8 +322,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  
>  err_free_cpu_data:
>  	kfree(parent_cpu_data);
And all this error block goes away.

> -err_put_policy:
> -	cpufreq_cpu_put(policy);
>  err:
>  
>  	return ret;

Re: [PATCH v4 06/10] drm/i915: Use scope-based cleanup helper

[Jonathan Cameron]

On Wed,  3 Sep 2025 21:17:29 +0800
Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
FWIW this one is fine if low impact.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_llc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c b/drivers/gpu/drm/i915/gt/intel_llc.c
> index 1d19c073ba2e..f15e4c0fa54b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_llc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_llc.c
> @@ -29,13 +29,11 @@ static struct intel_gt *llc_to_gt(struct intel_llc *llc)
>  
>  static unsigned int cpu_max_MHz(void)
>  {
> -	struct cpufreq_policy *policy;
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
>  	unsigned int max_khz;
>  
> -	policy = cpufreq_cpu_get(0);
>  	if (policy) {
>  		max_khz = policy->cpuinfo.max_freq;
> -		cpufreq_cpu_put(policy);
>  	} else {
>  		/*
>  		 * Default to measured freq if none found, PCU will ensure we

Re: [PATCH v4 08/10] thermal: imx: Use scope-based cleanup helper

[Jonathan Cameron]

On Wed,  3 Sep 2025 21:17:31 +0800
Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>

This radically changes the lifetime of the reference to policy.
If that is valid, then I'd expect a lot more description of why!

> ---
>  drivers/thermal/imx_thermal.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 38c993d1bcb3..cd1d9d419275 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -201,7 +201,6 @@ static struct thermal_soc_data thermal_imx7d_data = {
>  
>  struct imx_thermal_data {
>  	struct device *dev;
> -	struct cpufreq_policy *policy;
>  	struct thermal_zone_device *tz;
>  	struct thermal_cooling_device *cdev;
>  	struct regmap *tempmon;
> @@ -541,22 +540,20 @@ MODULE_DEVICE_TABLE(of, of_imx_thermal_match);
>  static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
>  {
>  	struct device_node *np;
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(0);
>  	int ret = 0;
>  
> -	data->policy = cpufreq_cpu_get(0);
> -	if (!data->policy) {
> +	if (!policy) {
>  		pr_debug("%s: CPUFreq policy not found\n", __func__);
>  		return -EPROBE_DEFER;
>  	}
>  
> -	np = of_get_cpu_node(data->policy->cpu, NULL);
> +	np = of_get_cpu_node(policy->cpu, NULL);
>  
>  	if (!np || !of_property_present(np, "#cooling-cells")) {
> -		data->cdev = cpufreq_cooling_register(data->policy);
> -		if (IS_ERR(data->cdev)) {
> +		data->cdev = cpufreq_cooling_register(policy);
> +		if (IS_ERR(data->cdev))
>  			ret = PTR_ERR(data->cdev);
> -			cpufreq_cpu_put(data->policy);
> -		}
>  	}
>  
>  	of_node_put(np);
> @@ -567,7 +564,6 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
>  static void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data *data)
>  {
>  	cpufreq_cooling_unregister(data->cdev);
> -	cpufreq_cpu_put(data->policy);
>  }
>  
>  #else

Re: [PATCH v4 08/10] thermal: imx: Use scope-based cleanup helper

[Zihuan Zhang]

在 2025/9/5 18:05, Jonathan Cameron 写道:
> This radically changes the lifetime of the reference to policy.
> If that is valid, then I'd expect a lot more description of why!


Thanks for your comment.

Initially I thought policy was only used within this function, so I 
moved it here.
After your feedback, I realize that this change might affect the 
reference lifetime in ways I didn’t fully consider. I need to review it 
further to ensure it’s safe.