[PATCH v3 12/12] PM: EM: Use scope-based cleanup helper

Zihuan Zhang posted 12 patches 1 month ago
There is a newer version of this series
[PATCH v3 12/12] PM: EM: Use scope-based cleanup helper
Posted by Zihuan Zhang 1 month ago
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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index ea7995a25780..852d48039ce2 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);
 	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,21 +785,20 @@ 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) =
+			cpufreq_cpu_get(cpu);
 		struct em_perf_domain *pd;
 		struct device *dev;
 
 		if (cpumask_test_cpu(cpu, cpu_done_mask))
 			continue;
 
-		policy = cpufreq_cpu_get(cpu);
 		if (!policy) {
 			pr_debug("Accessing cpu%d policy failed\n", cpu);
 			schedule_delayed_work(&em_update_work,
 					      msecs_to_jiffies(1000));
 			break;
 		}
-		cpufreq_cpu_put(policy);
 
 		dev = get_cpu_device(cpu);
 		pd = em_pd_get(dev);
-- 
2.25.1
Re: [PATCH v3 12/12] PM: EM: Use scope-based cleanup helper
Posted by Krzysztof Kozlowski 1 month ago
On 01/09/2025 10:57, 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 | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index ea7995a25780..852d48039ce2 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);

You are not improving the source code here. This is not how to use
__free() and you clearly do not understand the source code.

What's more, you did not use standard tools which would tell you this is
buggy and wrong.

Don't introduce cleanup.h if you do not understand how it works.
Best regards,
Krzysztof
Re: [PATCH v3 12/12] PM: EM: Use scope-based cleanup helper
Posted by Zihuan Zhang 1 month ago
> You are not improving the source code here. This is not how to use
>  __free() and you clearly do not understand the source code.

Sorry for the problem, policy should be assigned after cpumask_test_cpu().

I actually realized earlier that __free() only frees at the end of the variable’s lifetime. 
I had suggested using a braced macro in cpufreq.h to allow immediate release after use, 
but I understand the maintainer’s advice to “keep it simple” and will follow that.

> What's more, you did not use standard tools which would tell you this is
> buggy and wrong.

Could you please let me know which standard tools you recommend for detecting such issues? 

I’d like to use them to avoid similar mistakes in the future.

> Don't introduce cleanup.h if you do not understand how it works.

Should I drop this patch?
Re: [PATCH v3 12/12] PM: EM: Use scope-based cleanup helper
Posted by Krzysztof Kozlowski 1 month ago
On 03/09/2025 04:12, Zihuan Zhang wrote:
>> You are not improving the source code here. This is not how to use
>>  __free() and you clearly do not understand the source code.
> 
> Sorry for the problem, policy should be assigned after cpumask_test_cpu().
> 
> I actually realized earlier that __free() only frees at the end of the variable’s lifetime. 
> I had suggested using a braced macro in cpufreq.h to allow immediate release after use, 
> but I understand the maintainer’s advice to “keep it simple” and will follow that.
> 
>> What's more, you did not use standard tools which would tell you this is
>> buggy and wrong.
> 
> Could you please let me know which standard tools you recommend for detecting such issues? 
> 
> I’d like to use them to avoid similar mistakes in the future.
All standard tools used for kernel development, sparse, smatch, clang,
coccinelle, see my talk from OSSE25.

Best regards,
Krzysztof