drivers/cpufreq/cpufreq.c | 53 +++++++++++---------------------------- include/linux/cpufreq.h | 6 ++--- 2 files changed, 17 insertions(+), 42 deletions(-)
A recent change exposed a bug in the error path: if
freq_qos_add_request(boost_freq_req) fails, min_freq_req may remain a
valid pointer even though it was never successfully added. During policy
teardown, this leads to an unconditional call to
freq_qos_remove_request(), triggering a WARN.
The current design allocates all three freq_req objects together, making
the lifetime rules unclear and error handling fragile.
Simplify this by allocating the QoS freq_req objects at policy
allocation time. The policy itself is dynamically allocated, and two of
the three requests are always needed anyway. This ensures consistent
lifetime management and eliminates the inconsistent state in failure
paths.
Reported-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
Fixes: 6e39ba4e5a82 ("cpufreq: Add boost_freq_req QoS request")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 53 +++++++++++----------------------------
include/linux/cpufreq.h | 6 ++---
2 files changed, 17 insertions(+), 42 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c0aa970c7a67..f4a949f1e48f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -614,7 +614,7 @@ static int policy_set_boost(struct cpufreq_policy *policy, bool enable)
return ret;
}
- ret = freq_qos_update_request(policy->boost_freq_req, policy->cpuinfo.max_freq);
+ ret = freq_qos_update_request(&policy->boost_freq_req, policy->cpuinfo.max_freq);
if (ret < 0) {
policy->boost_enabled = !policy->boost_enabled;
cpufreq_driver->set_boost(policy, policy->boost_enabled);
@@ -769,7 +769,7 @@ static ssize_t store_##file_name \
if (ret) \
return ret; \
\
- ret = freq_qos_update_request(policy->object##_freq_req, val);\
+ ret = freq_qos_update_request(&policy->object##_freq_req, val); \
return ret >= 0 ? count : ret; \
}
@@ -1374,7 +1374,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
/* Cancel any pending policy->update work before freeing the policy. */
cancel_work_sync(&policy->update);
- if (policy->max_freq_req) {
+ if (freq_qos_request_active(&policy->max_freq_req)) {
/*
* Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
* notification, since CPUFREQ_CREATE_POLICY notification was
@@ -1382,12 +1382,13 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
*/
blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
CPUFREQ_REMOVE_POLICY, policy);
- freq_qos_remove_request(policy->max_freq_req);
+ freq_qos_remove_request(&policy->max_freq_req);
}
- freq_qos_remove_request(policy->min_freq_req);
- freq_qos_remove_request(policy->boost_freq_req);
- kfree(policy->min_freq_req);
+ if (freq_qos_request_active(&policy->min_freq_req))
+ freq_qos_remove_request(&policy->min_freq_req);
+ if (freq_qos_request_active(&policy->boost_freq_req))
+ freq_qos_remove_request(&policy->boost_freq_req);
cpufreq_policy_put_kobj(policy);
free_cpumask_var(policy->real_cpus);
@@ -1452,57 +1453,31 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
if (new_policy) {
- unsigned int count;
-
for_each_cpu(j, policy->related_cpus) {
per_cpu(cpufreq_cpu_data, j) = policy;
add_cpu_dev_symlink(policy, j, get_cpu_device(j));
}
- count = policy->boost_supported ? 3 : 2;
- policy->min_freq_req = kzalloc(count * sizeof(*policy->min_freq_req),
- GFP_KERNEL);
- if (!policy->min_freq_req) {
- ret = -ENOMEM;
- goto out_destroy_policy;
- }
-
if (policy->boost_supported) {
- policy->boost_freq_req = policy->min_freq_req + 2;
-
ret = freq_qos_add_request(&policy->constraints,
- policy->boost_freq_req,
+ &policy->boost_freq_req,
FREQ_QOS_MAX,
policy->cpuinfo.max_freq);
- if (ret < 0) {
- policy->boost_freq_req = NULL;
+ if (ret < 0)
goto out_destroy_policy;
- }
}
ret = freq_qos_add_request(&policy->constraints,
- policy->min_freq_req, FREQ_QOS_MIN,
+ &policy->min_freq_req, FREQ_QOS_MIN,
FREQ_QOS_MIN_DEFAULT_VALUE);
- if (ret < 0) {
- kfree(policy->min_freq_req);
- policy->min_freq_req = NULL;
+ if (ret < 0)
goto out_destroy_policy;
- }
-
- /*
- * This must be initialized right here to avoid calling
- * freq_qos_remove_request() on uninitialized request in case
- * of errors.
- */
- policy->max_freq_req = policy->min_freq_req + 1;
ret = freq_qos_add_request(&policy->constraints,
- policy->max_freq_req, FREQ_QOS_MAX,
+ &policy->max_freq_req, FREQ_QOS_MAX,
FREQ_QOS_MAX_DEFAULT_VALUE);
- if (ret < 0) {
- policy->max_freq_req = NULL;
+ if (ret < 0)
goto out_destroy_policy;
- }
blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
CPUFREQ_CREATE_POLICY, policy);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index b6f6c7d06912..9b10eb486ece 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -79,9 +79,9 @@ struct cpufreq_policy {
* called, but you're in IRQ context */
struct freq_constraints constraints;
- struct freq_qos_request *min_freq_req;
- struct freq_qos_request *max_freq_req;
- struct freq_qos_request *boost_freq_req;
+ struct freq_qos_request min_freq_req;
+ struct freq_qos_request max_freq_req;
+ struct freq_qos_request boost_freq_req;
struct cpufreq_frequency_table *freq_table;
enum cpufreq_table_sorting freq_table_sorted;
--
2.31.1.272.g89b43f80a514On 3/31/2026 1:03 PM, Viresh Kumar wrote:
> A recent change exposed a bug in the error path: if
> freq_qos_add_request(boost_freq_req) fails, min_freq_req may remain a
> valid pointer even though it was never successfully added. During policy
> teardown, this leads to an unconditional call to
> freq_qos_remove_request(), triggering a WARN.
>
> The current design allocates all three freq_req objects together, making
> the lifetime rules unclear and error handling fragile.
>
> Simplify this by allocating the QoS freq_req objects at policy
> allocation time. The policy itself is dynamically allocated, and two of
> the three requests are always needed anyway. This ensures consistent
> lifetime management and eliminates the inconsistent state in failure
> paths.
>
> Reported-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> Fixes: 6e39ba4e5a82 ("cpufreq: Add boost_freq_req QoS request")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Thanks for the elegant solution. It looks good to me.
Reviewed-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> ---
> drivers/cpufreq/cpufreq.c | 53 +++++++++++----------------------------
> include/linux/cpufreq.h | 6 ++---
> 2 files changed, 17 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index c0aa970c7a67..f4a949f1e48f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -614,7 +614,7 @@ static int policy_set_boost(struct cpufreq_policy *policy, bool enable)
> return ret;
> }
>
> - ret = freq_qos_update_request(policy->boost_freq_req, policy->cpuinfo.max_freq);
> + ret = freq_qos_update_request(&policy->boost_freq_req, policy->cpuinfo.max_freq);
> if (ret < 0) {
> policy->boost_enabled = !policy->boost_enabled;
> cpufreq_driver->set_boost(policy, policy->boost_enabled);
> @@ -769,7 +769,7 @@ static ssize_t store_##file_name \
> if (ret) \
> return ret; \
> \
> - ret = freq_qos_update_request(policy->object##_freq_req, val);\
> + ret = freq_qos_update_request(&policy->object##_freq_req, val); \
> return ret >= 0 ? count : ret; \
> }
>
> @@ -1374,7 +1374,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> /* Cancel any pending policy->update work before freeing the policy. */
> cancel_work_sync(&policy->update);
>
> - if (policy->max_freq_req) {
> + if (freq_qos_request_active(&policy->max_freq_req)) {
> /*
> * Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
> * notification, since CPUFREQ_CREATE_POLICY notification was
> @@ -1382,12 +1382,13 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> */
> blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> CPUFREQ_REMOVE_POLICY, policy);
> - freq_qos_remove_request(policy->max_freq_req);
> + freq_qos_remove_request(&policy->max_freq_req);
> }
>
> - freq_qos_remove_request(policy->min_freq_req);
> - freq_qos_remove_request(policy->boost_freq_req);
> - kfree(policy->min_freq_req);
> + if (freq_qos_request_active(&policy->min_freq_req))
> + freq_qos_remove_request(&policy->min_freq_req);
> + if (freq_qos_request_active(&policy->boost_freq_req))
> + freq_qos_remove_request(&policy->boost_freq_req);
>
> cpufreq_policy_put_kobj(policy);
> free_cpumask_var(policy->real_cpus);
> @@ -1452,57 +1453,31 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
> cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
>
> if (new_policy) {
> - unsigned int count;
> -
> for_each_cpu(j, policy->related_cpus) {
> per_cpu(cpufreq_cpu_data, j) = policy;
> add_cpu_dev_symlink(policy, j, get_cpu_device(j));
> }
>
> - count = policy->boost_supported ? 3 : 2;
> - policy->min_freq_req = kzalloc(count * sizeof(*policy->min_freq_req),
> - GFP_KERNEL);
> - if (!policy->min_freq_req) {
> - ret = -ENOMEM;
> - goto out_destroy_policy;
> - }
> -
> if (policy->boost_supported) {
> - policy->boost_freq_req = policy->min_freq_req + 2;
> -
> ret = freq_qos_add_request(&policy->constraints,
> - policy->boost_freq_req,
> + &policy->boost_freq_req,
> FREQ_QOS_MAX,
> policy->cpuinfo.max_freq);
> - if (ret < 0) {
> - policy->boost_freq_req = NULL;
> + if (ret < 0)
> goto out_destroy_policy;
> - }
> }
>
> ret = freq_qos_add_request(&policy->constraints,
> - policy->min_freq_req, FREQ_QOS_MIN,
> + &policy->min_freq_req, FREQ_QOS_MIN,
> FREQ_QOS_MIN_DEFAULT_VALUE);
> - if (ret < 0) {
> - kfree(policy->min_freq_req);
> - policy->min_freq_req = NULL;
> + if (ret < 0)
> goto out_destroy_policy;
> - }
> -
> - /*
> - * This must be initialized right here to avoid calling
> - * freq_qos_remove_request() on uninitialized request in case
> - * of errors.
> - */
> - policy->max_freq_req = policy->min_freq_req + 1;
>
> ret = freq_qos_add_request(&policy->constraints,
> - policy->max_freq_req, FREQ_QOS_MAX,
> + &policy->max_freq_req, FREQ_QOS_MAX,
> FREQ_QOS_MAX_DEFAULT_VALUE);
> - if (ret < 0) {
> - policy->max_freq_req = NULL;
> + if (ret < 0)
> goto out_destroy_policy;
> - }
>
> blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> CPUFREQ_CREATE_POLICY, policy);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index b6f6c7d06912..9b10eb486ece 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -79,9 +79,9 @@ struct cpufreq_policy {
> * called, but you're in IRQ context */
>
> struct freq_constraints constraints;
> - struct freq_qos_request *min_freq_req;
> - struct freq_qos_request *max_freq_req;
> - struct freq_qos_request *boost_freq_req;
> + struct freq_qos_request min_freq_req;
> + struct freq_qos_request max_freq_req;
> + struct freq_qos_request boost_freq_req;
>
> struct cpufreq_frequency_table *freq_table;
> enum cpufreq_table_sorting freq_table_sorted;
--
Thx and BRs,
Zhongqiu Han
On 3/31/2026 1:03 PM, Viresh Kumar wrote:
> A recent change exposed a bug in the error path: if
> freq_qos_add_request(boost_freq_req) fails, min_freq_req may remain a
> valid pointer even though it was never successfully added. During policy
> teardown, this leads to an unconditional call to
> freq_qos_remove_request(), triggering a WARN.
>
> The current design allocates all three freq_req objects together, making
> the lifetime rules unclear and error handling fragile.
>
> Simplify this by allocating the QoS freq_req objects at policy
> allocation time. The policy itself is dynamically allocated, and two of
> the three requests are always needed anyway. This ensures consistent
> lifetime management and eliminates the inconsistent state in failure
> paths.
>
> Reported-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> Fixes: 6e39ba4e5a82 ("cpufreq: Add boost_freq_req QoS request")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
This looks much neater. Thanks!
Reviewed-by: Lifeng Zheng <zhenglifeng1@huawei.com>
On 3/31/26 08:44, zhenglifeng (A) wrote:
> On 3/31/2026 1:03 PM, Viresh Kumar wrote:
>> A recent change exposed a bug in the error path: if
>> freq_qos_add_request(boost_freq_req) fails, min_freq_req may remain a
>> valid pointer even though it was never successfully added. During policy
>> teardown, this leads to an unconditional call to
>> freq_qos_remove_request(), triggering a WARN.
>>
>> The current design allocates all three freq_req objects together, making
>> the lifetime rules unclear and error handling fragile.
>>
>> Simplify this by allocating the QoS freq_req objects at policy
>> allocation time. The policy itself is dynamically allocated, and two of
>> the three requests are always needed anyway. This ensures consistent
>> lifetime management and eliminates the inconsistent state in failure
>> paths.
>>
>> Reported-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
>> Fixes: 6e39ba4e5a82 ("cpufreq: Add boost_freq_req QoS request")
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> This looks much neater. Thanks!
>
> Reviewed-by: Lifeng Zheng <zhenglifeng1@huawei.com>
Thanks for fixing the patch, removing the allocation makes
things much cleaner.
Tested-by: Pierre Gondois <pierre.gondois@arm.com>
© 2016 - 2026 Red Hat, Inc.