Re: [PATCH v2 1/4] cpufreq: Extract cpufreq_policy_init_qos() function

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

On 5/11/2026 9:55 PM, Pierre Gondois wrote:
> Extract the QoS related logic from cpufreq_policy_online()
> to make the function shorter/simpler.
> 
> The logic is placed in cpufreq_policy_init_qos() and is
> now executed right after the following calls:
> - cpufreq_driver->init()
> - cpufreq_table_validate_and_sort()
> 
> This helps preparing following patches that will,
> in cpufreq_policy_init_qos():
> - treat the policy->min/max values set by drivers as QoS requests.
> - set a default policy->min/max value to all policies.
> 
> No functional change.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>


Looks good to me apart from a minor nit inline.

Reviewed-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>


> ---
>   drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++++----------------
>   1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 44eb1b7e7fc1b..034603c2af325 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1397,6 +1397,32 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>   	kfree(policy);
>   }
>   
> +static int cpufreq_policy_init_qos(struct cpufreq_policy *policy)
> +{
> +	int ret;
> +
> +	if (policy->boost_supported) {
> +		ret = freq_qos_add_request(&policy->constraints,
> +						&policy->boost_freq_req,
> +						FREQ_QOS_MAX,
> +						policy->cpuinfo.max_freq);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = freq_qos_add_request(&policy->constraints, &policy->min_freq_req,
> +				   FREQ_QOS_MIN, FREQ_QOS_MIN_DEFAULT_VALUE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = freq_qos_add_request(&policy->constraints, &policy->max_freq_req,
> +				   FREQ_QOS_MAX, FREQ_QOS_MAX_DEFAULT_VALUE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret;



Just minor nit: cpufreq_policy_init_qos() could perhaps return 0 on
success.

In the original inline code we only checked 'ret < 0', so positive
return values were not intended to propagate as part of the API
contract. Returning 'ret' here may expose a positive success code (e.g.
1) and make the helper easier to misuse (e.g. 'if (ret)' checks).
Returning 0 would keep the semantics unambiguous.

Alternatively, if this behavior is intentional, it might be helpful to
document it with a kdoc comment (e.g. that the function may return 0 or
1 on success and callers should check 'ret < 0' for errors).



> +}
> +
>   static int cpufreq_policy_online(struct cpufreq_policy *policy,
>   				 unsigned int cpu, bool new_policy)
>   {
> @@ -1442,6 +1468,12 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>   		if (ret)
>   			goto out_offline_policy;
>   
> +		if (new_policy) {
> +			ret = cpufreq_policy_init_qos(policy);
> +			if (ret < 0)
> +				goto out_offline_policy;
> +		}
> +
>   		/* related_cpus should at least include policy->cpus. */
>   		cpumask_copy(policy->related_cpus, policy->cpus);
>   	}
> @@ -1458,27 +1490,6 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>   			add_cpu_dev_symlink(policy, j, get_cpu_device(j));
>   		}
>   
> -		if (policy->boost_supported) {
> -			ret = freq_qos_add_request(&policy->constraints,
> -						   &policy->boost_freq_req,
> -						   FREQ_QOS_MAX,
> -						   policy->cpuinfo.max_freq);
> -			if (ret < 0)
> -				goto out_destroy_policy;
> -		}
> -
> -		ret = freq_qos_add_request(&policy->constraints,
> -					   &policy->min_freq_req, FREQ_QOS_MIN,
> -					   FREQ_QOS_MIN_DEFAULT_VALUE);
> -		if (ret < 0)
> -			goto out_destroy_policy;
> -
> -		ret = freq_qos_add_request(&policy->constraints,
> -					   &policy->max_freq_req, FREQ_QOS_MAX,
> -					   FREQ_QOS_MAX_DEFAULT_VALUE);
> -		if (ret < 0)
> -			goto out_destroy_policy;
> -
>   		blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>   				CPUFREQ_CREATE_POLICY, policy);
>   	}


-- 
Thx and BRs,
Zhongqiu Han