1. Patchew
  2. linux
  3. [PATCH v3 00/12] cpufreq: use __free() for all cpufreq_cpu_get() references
  4. View patch

[PATCH v3 03/12] cpufreq: intel_pstate: Use scope-based cleanup helper

Zihuan Zhang posted 12 patches 1 month ago
There is a newer version of this series
[PATCH v3 03/12] cpufreq: intel_pstate: 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>
---
 drivers/cpufreq/intel_pstate.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index f366d35c5840..4abc1ef2d2b0 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1502,9 +1502,8 @@ static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
 
 static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
 {
-	struct cpufreq_policy *policy __free(put_cpufreq_policy);
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
 
-	policy = cpufreq_cpu_get(cpudata->cpu);
 	if (!policy)
 		return false;
 
@@ -1698,19 +1697,18 @@ 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;
+		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+			cpufreq_cpu_get(i);
 
-		policy = cpufreq_cpu_get(i);
 		if (!policy)
 			continue;
 
 		req = policy->driver_data;
-		cpufreq_cpu_put(policy);
 
 		if (!req)
 			continue;
-- 
2.25.1
Re: [PATCH v3 03/12] cpufreq: intel_pstate: Use scope-based cleanup helper
Posted by Rafael J. Wysocki 1 month ago 
On Mon, Sep 1, 2025 at 10:58 AM 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 | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index f366d35c5840..4abc1ef2d2b0 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1502,9 +1502,8 @@ static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
>
>  static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
>  {
> -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
>
> -       policy = cpufreq_cpu_get(cpudata->cpu);
>         if (!policy)
>                 return false;

The structure of the code is intentional here and there's no reason to
change it.
Re: [PATCH v3 03/12] cpufreq: intel_pstate: Use scope-based cleanup helper
Posted by Zihuan Zhang 1 month ago 
在 2025/9/1 23:17, Rafael J. Wysocki 写道:
> On Mon, Sep 1, 2025 at 10:58 AM 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 | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index f366d35c5840..4abc1ef2d2b0 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -1502,9 +1502,8 @@ static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
>>
>>   static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
>>   {
>> -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
>>
>> -       policy = cpufreq_cpu_get(cpudata->cpu);
>>          if (!policy)
>>                  return false;
> The structure of the code is intentional here and there's no reason to
> change it.


Got it. Thanks for clarifying.

So for this case the current structure is intentional -

should I also avoid similar changes in other drivers?
Re: [PATCH v3 03/12] cpufreq: intel_pstate: Use scope-based cleanup helper
Posted by Rafael J. Wysocki 1 month ago 
On Tue, Sep 2, 2025 at 12:33 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
>
> 在 2025/9/1 23:17, Rafael J. Wysocki 写道:
> > On Mon, Sep 1, 2025 at 10:58 AM 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 | 8 +++-----
> >>   1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >> index f366d35c5840..4abc1ef2d2b0 100644
> >> --- a/drivers/cpufreq/intel_pstate.c
> >> +++ b/drivers/cpufreq/intel_pstate.c
> >> @@ -1502,9 +1502,8 @@ static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
> >>
> >>   static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> >>   {
> >> -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> >> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
> >>
> >> -       policy = cpufreq_cpu_get(cpudata->cpu);
> >>          if (!policy)
> >>                  return false;
> > The structure of the code is intentional here and there's no reason to
> > change it.
>
>
> Got it. Thanks for clarifying.
>
> So for this case the current structure is intentional -

Note that I'm talking about this particular change only.  The other
change in the $subject patch is fine.

> should I also avoid similar changes in other drivers?

That depends on who maintains them, which is why I wanted you to split
the patch into smaller changes in the first place.

My personal view is that code formatting changes, which effectively is
what this particular one is, are pointless unless they make the code
much easier to follow.
Re: [PATCH v3 03/12] cpufreq: intel_pstate: Use scope-based cleanup helper
Posted by Zihuan Zhang 1 month ago 
在 2025/9/2 19:47, Rafael J. Wysocki 写道:
> On Tue, Sep 2, 2025 at 12:33 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>>
>> 在 2025/9/1 23:17, Rafael J. Wysocki 写道:
>>> On Mon, Sep 1, 2025 at 10:58 AM 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 | 8 +++-----
>>>>    1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>>> index f366d35c5840..4abc1ef2d2b0 100644
>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>> @@ -1502,9 +1502,8 @@ static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
>>>>
>>>>    static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
>>>>    {
>>>> -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
>>>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
>>>>
>>>> -       policy = cpufreq_cpu_get(cpudata->cpu);
>>>>           if (!policy)
>>>>                   return false;
>>> The structure of the code is intentional here and there's no reason to
>>> change it.
>>
>> Got it. Thanks for clarifying.
>>
>> So for this case the current structure is intentional -
> Note that I'm talking about this particular change only.  The other
> change in the $subject patch is fine.
>
>> should I also avoid similar changes in other drivers?
> That depends on who maintains them, which is why I wanted you to split
> the patch into smaller changes in the first place.
>
> My personal view is that code formatting changes, which effectively is
> what this particular one is, are pointless unless they make the code
> much easier to follow.


UnderStood, Thanks!
Re: [PATCH v3 03/12] cpufreq: intel_pstate: Use scope-based cleanup helper
Posted by Rafael J. Wysocki 1 month ago 
On Wed, Sep 3, 2025 at 2:51 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
>
> 在 2025/9/2 19:47, Rafael J. Wysocki 写道:
> > On Tue, Sep 2, 2025 at 12:33 PM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
> >>
> >> 在 2025/9/1 23:17, Rafael J. Wysocki 写道:
> >>> On Mon, Sep 1, 2025 at 10:58 AM 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 | 8 +++-----
> >>>>    1 file changed, 3 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >>>> index f366d35c5840..4abc1ef2d2b0 100644
> >>>> --- a/drivers/cpufreq/intel_pstate.c
> >>>> +++ b/drivers/cpufreq/intel_pstate.c
> >>>> @@ -1502,9 +1502,8 @@ static void __intel_pstate_update_max_freq(struct cpufreq_policy *policy,
> >>>>
> >>>>    static bool intel_pstate_update_max_freq(struct cpudata *cpudata)
> >>>>    {
> >>>> -       struct cpufreq_policy *policy __free(put_cpufreq_policy);
> >>>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
> >>>>
> >>>> -       policy = cpufreq_cpu_get(cpudata->cpu);
> >>>>           if (!policy)
> >>>>                   return false;
> >>> The structure of the code is intentional here and there's no reason to
> >>> change it.
> >>
> >> Got it. Thanks for clarifying.
> >>
> >> So for this case the current structure is intentional -
> > Note that I'm talking about this particular change only.  The other
> > change in the $subject patch is fine.
> >
> >> should I also avoid similar changes in other drivers?
> > That depends on who maintains them, which is why I wanted you to split
> > the patch into smaller changes in the first place.
> >
> > My personal view is that code formatting changes, which effectively is
> > what this particular one is, are pointless unless they make the code
> > much easier to follow.
>
>
> UnderStood, Thanks!

Although I think that it would be cleaner to move the code executed in
each step of the for_each_possible_cpu() loop to a separate function.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.