[PATCH v1 0/3] cpufreq: intel_pstate: A fix, a __free()-based simplification, and a follow-up adjustment

[PATCH v1 0/3] cpufreq: intel_pstate: A fix, a __free()-based simplification, and a follow-up adjustment

[Rafael J. Wysocki]

Hi All,

This series is following up a discussion on a patch submitted by Zihuan Zhang,
and this message in particular:

https://lore.kernel.org/linux-pm/CAJZ5v0gN1T5woSF0tO=TbAh+2-sWzxFjWyDbB7wG2TFCOU01iQ@mail.gmail.com/

The first patch fixes the bug described in the message linked above, the
second one makes a __free()-based simplification based on it, and the
third one makes one more adjustment on top of the two previous patches.

Please see the changelogs for details.

Thanks!

[PATCH v1 1/3] cpufreq: intel_pstate: Fix object lifecycle issue in update_qos_request()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The cpufreq_cpu_put() call in update_qos_request() takes place too early
because the latter subsequently calls freq_qos_update_request() that
indirectly accesses the policy object in question through the QoS request
object passed to it.

Fortunately, update_qos_request() is called under intel_pstate_driver_lock,
so this issue does not matter for changing the intel_pstate operation
mode, but it theoretically can cause a crash to occur on CPU device hot
removal (which currently can only happen in virt, but it is formally
supported nevertheless).

Address this issue by modifying update_qos_request() to drop the
reference to the policy later.

Fixes: da5c504c7aae ("cpufreq: intel_pstate: Implement QoS supported freq constraints")
Cc: 5.4+ <stable@vger.kernel.org> # 5.4+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1708,10 +1708,10 @@ static void update_qos_request(enum freq
 			continue;
 
 		req = policy->driver_data;
-		cpufreq_cpu_put(policy);
-
-		if (!req)
+		if (!req) {
+			cpufreq_cpu_put(policy);
 			continue;
+		}
 
 		if (hwp_active)
 			intel_pstate_get_hwp_cap(cpu);
@@ -1727,6 +1727,8 @@ static void update_qos_request(enum freq
 
 		if (freq_qos_update_request(req, freq) < 0)
 			pr_warn("Failed to update freq constraint: CPU%d\n", i);
+
+		cpufreq_cpu_put(policy);
 	}
 }
 

Re: [PATCH v1 1/3] cpufreq: intel_pstate: Fix object lifecycle issue in update_qos_request()

[Zihuan Zhang]

在 2025/9/5 21:52, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The cpufreq_cpu_put() call in update_qos_request() takes place too early
> because the latter subsequently calls freq_qos_update_request() that
> indirectly accesses the policy object in question through the QoS request
> object passed to it.
>
> Fortunately, update_qos_request() is called under intel_pstate_driver_lock,
> so this issue does not matter for changing the intel_pstate operation
> mode, but it theoretically can cause a crash to occur on CPU device hot
> removal (which currently can only happen in virt, but it is formally
> supported nevertheless).
>
> Address this issue by modifying update_qos_request() to drop the
> reference to the policy later.
>
> Fixes: da5c504c7aae ("cpufreq: intel_pstate: Implement QoS supported freq constraints")
> Cc: 5.4+ <stable@vger.kernel.org> # 5.4+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/cpufreq/intel_pstate.c |    8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1708,10 +1708,10 @@ static void update_qos_request(enum freq
>   			continue;
>   
>   		req = policy->driver_data;
> -		cpufreq_cpu_put(policy);
> -
> -		if (!req)
> +		if (!req) {
> +			cpufreq_cpu_put(policy);
>   			continue;
> +		}
>   
>   		if (hwp_active)
>   			intel_pstate_get_hwp_cap(cpu);
> @@ -1727,6 +1727,8 @@ static void update_qos_request(enum freq
>   
>   		if (freq_qos_update_request(req, freq) < 0)
>   			pr_warn("Failed to update freq constraint: CPU%d\n", i);
> +
> +		cpufreq_cpu_put(policy);
>   	}
>   }
>   
Reviewed-by: Zihuan Zhang <zhangzihuan@kylinos.cn>

[PATCH v1 2/3] cpufreq: intel_pstate: Rearrange freq QoS updates using __free()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move the code from the for_each_possible_cpu() loop in update_qos_request()
to a separate function and use __free() for cpufreq policy reference
counting in it to avoid having to call cpufreq_cpu_put() repeatedly (or
using goto).

While at it, rename update_qos_request() to update_qos_requests()
because it updates multiple requests in one go.

No intentional functional impact.

Link: https://lore.kernel.org/linux-pm/CAJZ5v0gN1T5woSF0tO=TbAh+2-sWzxFjWyDbB7wG2TFCOU01iQ@mail.gmail.com/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   65 ++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1693,43 +1693,42 @@ unlock_driver:
 	return count;
 }
 
-static void update_qos_request(enum freq_qos_req_type type)
+static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
 {
+	struct cpudata *cpu = all_cpu_data[cpunum];
 	struct freq_qos_request *req;
-	struct cpufreq_policy *policy;
-	int i;
+	unsigned int freq, perf_pct;
 
-	for_each_possible_cpu(i) {
-		struct cpudata *cpu = all_cpu_data[i];
-		unsigned int freq, perf_pct;
-
-		policy = cpufreq_cpu_get(i);
-		if (!policy)
-			continue;
-
-		req = policy->driver_data;
-		if (!req) {
-			cpufreq_cpu_put(policy);
-			continue;
-		}
-
-		if (hwp_active)
-			intel_pstate_get_hwp_cap(cpu);
-
-		if (type == FREQ_QOS_MIN) {
-			perf_pct = global.min_perf_pct;
-		} else {
-			req++;
-			perf_pct = global.max_perf_pct;
-		}
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
+	if (!policy)
+		return;
+
+	req = policy->driver_data;
+	if (!req)
+		return;
+
+	if (hwp_active)
+		intel_pstate_get_hwp_cap(cpu);
+
+	if (type == FREQ_QOS_MIN) {
+		perf_pct = global.min_perf_pct;
+	} else {
+		req++;
+		perf_pct = global.max_perf_pct;
+	}
 
-		freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
+	freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
 
-		if (freq_qos_update_request(req, freq) < 0)
-			pr_warn("Failed to update freq constraint: CPU%d\n", i);
+	if (freq_qos_update_request(req, freq) < 0)
+		pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
+}
 
-		cpufreq_cpu_put(policy);
-	}
+static void update_qos_requests(enum freq_qos_req_type type)
+{
+	int i;
+
+	for_each_possible_cpu(i)
+		update_cpu_qos_request(i, type);
 }
 
 static ssize_t store_max_perf_pct(struct kobject *a, struct kobj_attribute *b,
@@ -1758,7 +1757,7 @@ static ssize_t store_max_perf_pct(struct
 	if (intel_pstate_driver == &intel_pstate)
 		intel_pstate_update_policies();
 	else
-		update_qos_request(FREQ_QOS_MAX);
+		update_qos_requests(FREQ_QOS_MAX);
 
 	mutex_unlock(&intel_pstate_driver_lock);
 
@@ -1792,7 +1791,7 @@ static ssize_t store_min_perf_pct(struct
 	if (intel_pstate_driver == &intel_pstate)
 		intel_pstate_update_policies();
 	else
-		update_qos_request(FREQ_QOS_MIN);
+		update_qos_requests(FREQ_QOS_MIN);
 
 	mutex_unlock(&intel_pstate_driver_lock);
 

Re: [PATCH v1 2/3] cpufreq: intel_pstate: Rearrange freq QoS updates using __free()

[Zihuan Zhang]

在 2025/9/5 21:52, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Move the code from the for_each_possible_cpu() loop in update_qos_request()
> to a separate function and use __free() for cpufreq policy reference
> counting in it to avoid having to call cpufreq_cpu_put() repeatedly (or
> using goto).
>
> While at it, rename update_qos_request() to update_qos_requests()
> because it updates multiple requests in one go.
>
> No intentional functional impact.
>
> Link: https://lore.kernel.org/linux-pm/CAJZ5v0gN1T5woSF0tO=TbAh+2-sWzxFjWyDbB7wG2TFCOU01iQ@mail.gmail.com/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/cpufreq/intel_pstate.c |   65 ++++++++++++++++++++---------------------
>   1 file changed, 32 insertions(+), 33 deletions(-)
>
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1693,43 +1693,42 @@ unlock_driver:
>   	return count;
>   }
>   
> -static void update_qos_request(enum freq_qos_req_type type)
> +static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
>   {
> +	struct cpudata *cpu = all_cpu_data[cpunum];


Maybe the parameter could be named int cpu instead of cpunum, and the 
struct cpudata * variable could be renamed to cpudata — this might read 
a bit cleaner and help reduce potential confusion.

I also noticed that in this driver some places use one naming style and 
others use another, so it might be worth unifying the style here.

Other than that, looks good to me.

>   	struct freq_qos_request *req;
> -	struct cpufreq_policy *policy;
> -	int i;
> +	unsigned int freq, perf_pct;
>   
> -	for_each_possible_cpu(i) {
> -		struct cpudata *cpu = all_cpu_data[i];
> -		unsigned int freq, perf_pct;
> -
> -		policy = cpufreq_cpu_get(i);
> -		if (!policy)
> -			continue;
> -
> -		req = policy->driver_data;
> -		if (!req) {
> -			cpufreq_cpu_put(policy);
> -			continue;
> -		}
> -
> -		if (hwp_active)
> -			intel_pstate_get_hwp_cap(cpu);
> -
> -		if (type == FREQ_QOS_MIN) {
> -			perf_pct = global.min_perf_pct;
> -		} else {
> -			req++;
> -			perf_pct = global.max_perf_pct;
> -		}
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
> +	if (!policy)
> +		return;
> +
> +	req = policy->driver_data;
> +	if (!req)
> +		return;
> +
> +	if (hwp_active)
> +		intel_pstate_get_hwp_cap(cpu);
> +
> +	if (type == FREQ_QOS_MIN) {
> +		perf_pct = global.min_perf_pct;
> +	} else {
> +		req++;
> +		perf_pct = global.max_perf_pct;
> +	}
>   
> -		freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
> +	freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
>   
> -		if (freq_qos_update_request(req, freq) < 0)
> -			pr_warn("Failed to update freq constraint: CPU%d\n", i);
> +	if (freq_qos_update_request(req, freq) < 0)
> +		pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
> +}
>   
> -		cpufreq_cpu_put(policy);
> -	}
> +static void update_qos_requests(enum freq_qos_req_type type)
> +{
> +	int i;
> +
> +	for_each_possible_cpu(i)
> +		update_cpu_qos_request(i, type);
>   }
>   
>   static ssize_t store_max_perf_pct(struct kobject *a, struct kobj_attribute *b,
> @@ -1758,7 +1757,7 @@ static ssize_t store_max_perf_pct(struct
>   	if (intel_pstate_driver == &intel_pstate)
>   		intel_pstate_update_policies();
>   	else
> -		update_qos_request(FREQ_QOS_MAX);
> +		update_qos_requests(FREQ_QOS_MAX);
>   
>   	mutex_unlock(&intel_pstate_driver_lock);
>   
> @@ -1792,7 +1791,7 @@ static ssize_t store_min_perf_pct(struct
>   	if (intel_pstate_driver == &intel_pstate)
>   		intel_pstate_update_policies();
>   	else
> -		update_qos_request(FREQ_QOS_MIN);
> +		update_qos_requests(FREQ_QOS_MIN);
>   
>   	mutex_unlock(&intel_pstate_driver_lock);
>   
>
>
>
Reviewed-by: Zihuan Zhang <zhangzihuan@kylinos.cn>

Re: [PATCH v1 2/3] cpufreq: intel_pstate: Rearrange freq QoS updates using __free()

[Rafael J. Wysocki]

On Mon, Sep 8, 2025 at 2:53 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
>
> 在 2025/9/5 21:52, Rafael J. Wysocki 写道:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Move the code from the for_each_possible_cpu() loop in update_qos_request()
> > to a separate function and use __free() for cpufreq policy reference
> > counting in it to avoid having to call cpufreq_cpu_put() repeatedly (or
> > using goto).
> >
> > While at it, rename update_qos_request() to update_qos_requests()
> > because it updates multiple requests in one go.
> >
> > No intentional functional impact.
> >
> > Link: https://lore.kernel.org/linux-pm/CAJZ5v0gN1T5woSF0tO=TbAh+2-sWzxFjWyDbB7wG2TFCOU01iQ@mail.gmail.com/
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/cpufreq/intel_pstate.c |   65 ++++++++++++++++++++---------------------
> >   1 file changed, 32 insertions(+), 33 deletions(-)
> >
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1693,43 +1693,42 @@ unlock_driver:
> >       return count;
> >   }
> >
> > -static void update_qos_request(enum freq_qos_req_type type)
> > +static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
> >   {
> > +     struct cpudata *cpu = all_cpu_data[cpunum];
>
>
> Maybe the parameter could be named int cpu instead of cpunum, and the
> struct cpudata * variable could be renamed to cpudata — this might read
> a bit cleaner and help reduce potential confusion.

Yeah, you are right that it could be done this way.

I kind of wanted the new code to be possibly similar to the new one,
but since this is a new function anyway, I guess this doesn't matter.
I'll change the naming when applying the patch.

> I also noticed that in this driver some places use one naming style and
> others use another, so it might be worth unifying the style here.

Well, I'm not sure about this.

I guess it may be confusing sometimes, but then I'm not sure if that
justifies the code churn that would result from changing it.

> Other than that, looks good to me.
>
> >       struct freq_qos_request *req;
> > -     struct cpufreq_policy *policy;
> > -     int i;
> > +     unsigned int freq, perf_pct;
> >
> > -     for_each_possible_cpu(i) {
> > -             struct cpudata *cpu = all_cpu_data[i];
> > -             unsigned int freq, perf_pct;
> > -
> > -             policy = cpufreq_cpu_get(i);
> > -             if (!policy)
> > -                     continue;
> > -
> > -             req = policy->driver_data;
> > -             if (!req) {
> > -                     cpufreq_cpu_put(policy);
> > -                     continue;
> > -             }
> > -
> > -             if (hwp_active)
> > -                     intel_pstate_get_hwp_cap(cpu);
> > -
> > -             if (type == FREQ_QOS_MIN) {
> > -                     perf_pct = global.min_perf_pct;
> > -             } else {
> > -                     req++;
> > -                     perf_pct = global.max_perf_pct;
> > -             }
> > +     struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
> > +     if (!policy)
> > +             return;
> > +
> > +     req = policy->driver_data;
> > +     if (!req)
> > +             return;
> > +
> > +     if (hwp_active)
> > +             intel_pstate_get_hwp_cap(cpu);
> > +
> > +     if (type == FREQ_QOS_MIN) {
> > +             perf_pct = global.min_perf_pct;
> > +     } else {
> > +             req++;
> > +             perf_pct = global.max_perf_pct;
> > +     }
> >
> > -             freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
> > +     freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
> >
> > -             if (freq_qos_update_request(req, freq) < 0)
> > -                     pr_warn("Failed to update freq constraint: CPU%d\n", i);
> > +     if (freq_qos_update_request(req, freq) < 0)
> > +             pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
> > +}
> >
> > -             cpufreq_cpu_put(policy);
> > -     }
> > +static void update_qos_requests(enum freq_qos_req_type type)
> > +{
> > +     int i;
> > +
> > +     for_each_possible_cpu(i)
> > +             update_cpu_qos_request(i, type);
> >   }
> >
> >   static ssize_t store_max_perf_pct(struct kobject *a, struct kobj_attribute *b,
> > @@ -1758,7 +1757,7 @@ static ssize_t store_max_perf_pct(struct
> >       if (intel_pstate_driver == &intel_pstate)
> >               intel_pstate_update_policies();
> >       else
> > -             update_qos_request(FREQ_QOS_MAX);
> > +             update_qos_requests(FREQ_QOS_MAX);
> >
> >       mutex_unlock(&intel_pstate_driver_lock);
> >
> > @@ -1792,7 +1791,7 @@ static ssize_t store_min_perf_pct(struct
> >       if (intel_pstate_driver == &intel_pstate)
> >               intel_pstate_update_policies();
> >       else
> > -             update_qos_request(FREQ_QOS_MIN);
> > +             update_qos_requests(FREQ_QOS_MIN);
> >
> >       mutex_unlock(&intel_pstate_driver_lock);
> >
> >
> >
> >
> Reviewed-by: Zihuan Zhang <zhangzihuan@kylinos.cn>

Thanks!

Re: [PATCH v1 2/3] cpufreq: intel_pstate: Rearrange freq QoS updates using __free()

[Zihuan Zhang]

在 2025/9/9 02:10, Rafael J. Wysocki 写道:
>> I also noticed that in this driver some places use one naming style and
>> others use another, so it might be worth unifying the style here.
> Well, I'm not sure about this.
>
> I guess it may be confusing sometimes, but then I'm not sure if that
> justifies the code churn that would result from changing it.

Agreed — there are quite a lot of places that would need to be changed, 
and perhaps it’s better to keep the current style as is.

[PATCH v1 3/3] cpufreq: intel_pstate: Adjust frequency percentage computations

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Adjust frequency percentage computations in update_cpu_qos_request() to
avoid going above the exact numerical percentage in the FREQ_QOS_MAX
case.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1696,8 +1696,8 @@ unlock_driver:
 static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
 {
 	struct cpudata *cpu = all_cpu_data[cpunum];
+	unsigned int freq = cpu->pstate.turbo_freq;
 	struct freq_qos_request *req;
-	unsigned int freq, perf_pct;
 
 	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
 	if (!policy)
@@ -1711,14 +1711,12 @@ static void update_cpu_qos_request(int c
 		intel_pstate_get_hwp_cap(cpu);
 
 	if (type == FREQ_QOS_MIN) {
-		perf_pct = global.min_perf_pct;
+		freq = DIV_ROUND_UP(freq * global.min_perf_pct, 100);
 	} else {
 		req++;
-		perf_pct = global.max_perf_pct;
+		freq = (freq * global.max_perf_pct) / 100;
 	}
 
-	freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
-
 	if (freq_qos_update_request(req, freq) < 0)
 		pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
 }

Re: [PATCH v1 3/3] cpufreq: intel_pstate: Adjust frequency percentage

[Zihuan Zhang]

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1696,8 +1696,8 @@ unlock_driver:
>  static void update_cpu_qos_request(int cpunum, enum freq_qos_req_type type)
>  {
>  	struct cpudata *cpu = all_cpu_data[cpunum];
> +	unsigned int freq = cpu->pstate.turbo_freq;
>  	struct freq_qos_request *req;
> -	unsigned int freq, perf_pct;
>  
>  	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpunum);
>  	if (!policy)
> @@ -1711,14 +1711,12 @@ static void update_cpu_qos_request(int c
>  		intel_pstate_get_hwp_cap(cpu);
>  
>  	if (type == FREQ_QOS_MIN) {
> -		perf_pct = global.min_perf_pct;
> +		freq = DIV_ROUND_UP(freq * global.min_perf_pct, 100);
>  	} else {
>  		req++;
> -		perf_pct = global.max_perf_pct;
> +		freq = (freq * global.max_perf_pct) / 100;
>  	}
>  
> -	freq = DIV_ROUND_UP(cpu->pstate.turbo_freq * perf_pct, 100);
> -
>  	if (freq_qos_update_request(req, freq) < 0)
>  		pr_warn("Failed to update freq constraint: CPU%d\n", cpunum);
>  }

Acked-by: Zihuan Zhang <zhangzihuan@kylinos.cn>