[PATCH v5 2/6] ACPI: processor: thermal: Use scope-based cleanup helper

Zihuan Zhang posted 6 patches 4 days, 16 hours ago
[PATCH v5 2/6] ACPI: processor: thermal: Use scope-based cleanup helper
Posted by Zihuan Zhang 4 days, 16 hours 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/acpi/processor_thermal.c | 52 +++++++++++++++++---------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index 1219adb11ab9..460713d1414a 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;
-
 	if (!acpi_processor_cpufreq_init)
 		return 0;
 
-	policy = cpufreq_cpu_get(cpu);
-	if (policy) {
-		cpufreq_cpu_put(policy);
-		return 1;
-	}
-	return 0;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
+
+	return policy != NULL;
 }
 
 static int cpufreq_get_max_state(unsigned int cpu)
@@ -93,12 +88,31 @@ static int cpufreq_get_cur_state(unsigned int cpu)
 	return reduction_step(cpu);
 }
 
+static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
+{
+	unsigned long max_freq;
+	int ret;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
+
+	if (!policy)
+		return false;
+
+	max_freq = (policy->cpuinfo.max_freq *
+		(100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
+
+	ret = freq_qos_update_request(&pr->thermal_req, max_freq);
+	if (ret < 0) {
+		pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
+	  pr->id, ret);
+	}
+
+	return true;
+}
+
 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;
+	int i;
 
 	if (!cpu_has_cpufreq(cpu))
 		return 0;
@@ -120,20 +134,8 @@ 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)
+		if (!cpufreq_update_thermal_limit(i, pr))
 			return -EINVAL;
-
-		max_freq = (policy->cpuinfo.max_freq *
-			    (100 - reduction_step(i) * cpufreq_thermal_reduction_pctg)) / 100;
-
-		cpufreq_cpu_put(policy);
-
-		ret = freq_qos_update_request(&pr->thermal_req, max_freq);
-		if (ret < 0) {
-			pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
-				pr->id, ret);
-		}
 	}
 	return 0;
 }
-- 
2.25.1
Re: [PATCH v5 2/6] ACPI: processor: thermal: Use scope-based cleanup helper
Posted by Rafael J. Wysocki 1 day, 11 hours ago
On Fri, Sep 5, 2025 at 3:24 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 | 52 +++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> index 1219adb11ab9..460713d1414a 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;
> -
>         if (!acpi_processor_cpufreq_init)
>                 return 0;
>
> -       policy = cpufreq_cpu_get(cpu);
> -       if (policy) {
> -               cpufreq_cpu_put(policy);
> -               return 1;
> -       }
> -       return 0;
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
> +
> +       return policy != NULL;
>  }
>
>  static int cpufreq_get_max_state(unsigned int cpu)
> @@ -93,12 +88,31 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>         return reduction_step(cpu);
>  }
>
> +static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
> +{
> +       unsigned long max_freq;
> +       int ret;
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
> +
> +       if (!policy)
> +               return false;
> +
> +       max_freq = (policy->cpuinfo.max_freq *
> +               (100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
> +
> +       ret = freq_qos_update_request(&pr->thermal_req, max_freq);
> +       if (ret < 0) {
> +               pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
> +         pr->id, ret);
> +       }
> +
> +       return true;
> +}
> +
>  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;
> +       int i;
>
>         if (!cpu_has_cpufreq(cpu))
>                 return 0;
> @@ -120,20 +134,8 @@ 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)
> +               if (!cpufreq_update_thermal_limit(i, pr))
>                         return -EINVAL;
> -
> -               max_freq = (policy->cpuinfo.max_freq *
> -                           (100 - reduction_step(i) * cpufreq_thermal_reduction_pctg)) / 100;
> -
> -               cpufreq_cpu_put(policy);
> -
> -               ret = freq_qos_update_request(&pr->thermal_req, max_freq);
> -               if (ret < 0) {
> -                       pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
> -                               pr->id, ret);
> -               }
>         }
>         return 0;
>  }
> --

Applied as 6.18 material under a new subject, with some minor edits in
the changelog, and with some minor whitespace breakage fixed.

Thanks!
Re: [PATCH v5 2/6] ACPI: processor: thermal: Use scope-based cleanup helper
Posted by Rafael J. Wysocki 4 days, 9 hours ago
On Fri, Sep 5, 2025 at 3:24 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 | 52 +++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> index 1219adb11ab9..460713d1414a 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;
> -
>         if (!acpi_processor_cpufreq_init)
>                 return 0;
>
> -       policy = cpufreq_cpu_get(cpu);
> -       if (policy) {
> -               cpufreq_cpu_put(policy);
> -               return 1;
> -       }
> -       return 0;
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
> +
> +       return policy != NULL;
>  }
>
>  static int cpufreq_get_max_state(unsigned int cpu)

The changes above are fine and can be sent as a separate patch.

> @@ -93,12 +88,31 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>         return reduction_step(cpu);
>  }
>
> +static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
> +{
> +       unsigned long max_freq;
> +       int ret;
> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
> +
> +       if (!policy)
> +               return false;
> +
> +       max_freq = (policy->cpuinfo.max_freq *
> +               (100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
> +
> +       ret = freq_qos_update_request(&pr->thermal_req, max_freq);
> +       if (ret < 0) {
> +               pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
> +         pr->id, ret);
> +       }

But this silently fixes a bug in the original code which needs to be
documented with a Fixes: tag (and it would be better to fix the bug
separately before the using the __free()-based cleanup TBH) and
introduces some whitespace breakage.

> +
> +       return true;
> +}
> +
>  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;
> +       int i;
>
>         if (!cpu_has_cpufreq(cpu))
>                 return 0;
> @@ -120,20 +134,8 @@ 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)
> +               if (!cpufreq_update_thermal_limit(i, pr))
>                         return -EINVAL;
> -
> -               max_freq = (policy->cpuinfo.max_freq *
> -                           (100 - reduction_step(i) * cpufreq_thermal_reduction_pctg)) / 100;
> -
> -               cpufreq_cpu_put(policy);
> -
> -               ret = freq_qos_update_request(&pr->thermal_req, max_freq);
> -               if (ret < 0) {
> -                       pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
> -                               pr->id, ret);
> -               }
>         }
>         return 0;
>  }
> --
Re: [PATCH v5 2/6] ACPI: processor: thermal: Use scope-based cleanup helper
Posted by Zihuan Zhang 1 day, 20 hours ago
在 2025/9/6 04:17, Rafael J. Wysocki 写道:
> On Fri, Sep 5, 2025 at 3:24 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 | 52 +++++++++++++++++---------------
>>   1 file changed, 27 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
>> index 1219adb11ab9..460713d1414a 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;
>> -
>>          if (!acpi_processor_cpufreq_init)
>>                  return 0;
>>
>> -       policy = cpufreq_cpu_get(cpu);
>> -       if (policy) {
>> -               cpufreq_cpu_put(policy);
>> -               return 1;
>> -       }
>> -       return 0;
>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>> +
>> +       return policy != NULL;
>>   }
>>
>>   static int cpufreq_get_max_state(unsigned int cpu)
> The changes above are fine and can be sent as a separate patch.
>
>> @@ -93,12 +88,31 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>>          return reduction_step(cpu);
>>   }
>>
>> +static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
>> +{
>> +       unsigned long max_freq;
>> +       int ret;
>> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>> +
>> +       if (!policy)
>> +               return false;
>> +
>> +       max_freq = (policy->cpuinfo.max_freq *
>> +               (100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
>> +
>> +       ret = freq_qos_update_request(&pr->thermal_req, max_freq);
>> +       if (ret < 0) {
>> +               pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
>> +         pr->id, ret);
>> +       }
> But this silently fixes a bug in the original code which needs to be
> documented with a Fixes: tag (and it would be better to fix the bug
> separately before the using the __free()-based cleanup TBH) and
> introduces some whitespace breakage.

Thanks!

  I’ll follow your advice and handle the Fixes tag and whitespace issues.


>> +
>> +       return true;
>> +}
>> +
>>   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;
>> +       int i;
>>
>>          if (!cpu_has_cpufreq(cpu))
>>                  return 0;
>> @@ -120,20 +134,8 @@ 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)
>> +               if (!cpufreq_update_thermal_limit(i, pr))
>>                          return -EINVAL;
>> -
>> -               max_freq = (policy->cpuinfo.max_freq *
>> -                           (100 - reduction_step(i) * cpufreq_thermal_reduction_pctg)) / 100;
>> -
>> -               cpufreq_cpu_put(policy);
>> -
>> -               ret = freq_qos_update_request(&pr->thermal_req, max_freq);
>> -               if (ret < 0) {
>> -                       pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
>> -                               pr->id, ret);
>> -               }
>>          }
>>          return 0;
>>   }
>> --
Re: [PATCH v5 2/6] ACPI: processor: thermal: Use scope-based cleanup helper
Posted by Rafael J. Wysocki 1 day, 12 hours ago
On Mon, Sep 8, 2025 at 11:16 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
>
> 在 2025/9/6 04:17, Rafael J. Wysocki 写道:
> > On Fri, Sep 5, 2025 at 3:24 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 | 52 +++++++++++++++++---------------
> >>   1 file changed, 27 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> >> index 1219adb11ab9..460713d1414a 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;
> >> -
> >>          if (!acpi_processor_cpufreq_init)
> >>                  return 0;
> >>
> >> -       policy = cpufreq_cpu_get(cpu);
> >> -       if (policy) {
> >> -               cpufreq_cpu_put(policy);
> >> -               return 1;
> >> -       }
> >> -       return 0;
> >> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
> >> +
> >> +       return policy != NULL;
> >>   }
> >>
> >>   static int cpufreq_get_max_state(unsigned int cpu)
> > The changes above are fine and can be sent as a separate patch.
> >
> >> @@ -93,12 +88,31 @@ static int cpufreq_get_cur_state(unsigned int cpu)
> >>          return reduction_step(cpu);
> >>   }
> >>
> >> +static bool cpufreq_update_thermal_limit(unsigned int cpu, struct acpi_processor *pr)
> >> +{
> >> +       unsigned long max_freq;
> >> +       int ret;
> >> +       struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
> >> +
> >> +       if (!policy)
> >> +               return false;
> >> +
> >> +       max_freq = (policy->cpuinfo.max_freq *
> >> +               (100 - reduction_step(cpu) * cpufreq_thermal_reduction_pctg)) / 100;
> >> +
> >> +       ret = freq_qos_update_request(&pr->thermal_req, max_freq);
> >> +       if (ret < 0) {
> >> +               pr_warn("Failed to update thermal freq constraint: CPU%d (%d)\n",
> >> +         pr->id, ret);
> >> +       }
> > But this silently fixes a bug in the original code which needs to be
> > documented with a Fixes: tag (and it would be better to fix the bug
> > separately before the using the __free()-based cleanup TBH) and
> > introduces some whitespace breakage.
>
> Thanks!
>
>   I’ll follow your advice and handle the Fixes tag and whitespace issues.

Actually, no need to resend.

The current code is correct as it registers and unregisters the freq
QoS request from cpufreq policy notifiers, so the policy is guaranteed
to be there when cpufreq_set_cur_state() runs.