[v4] cpufreq: use __free() for all cpufreq_cpu_get() references

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

Posted by Zihuan Zhang 4 weeks, 1 day 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>
---
 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

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

Posted by Andreas Kemnade 4 weeks ago

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

Posted by Zihuan Zhang 3 weeks, 6 days ago

在 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 09/10] thermal/drivers/ti-soc-thermal: Use scope-based cleanup helper

Posted by Krzysztof Kozlowski 4 weeks ago

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