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

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

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!

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

在 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!