Re: [PATCH] cpufreq: Allocate QoS freq_req objects with policy

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

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>

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