Re: [PATCH v7 2/2] cpufreq: Add boost_freq_req QoS request

[Viresh Kumar]

On 25-03-26, 17:52, Pierre Gondois wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> @@ -1445,18 +1456,42 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>  	cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
>  
>  	if (new_policy) {
> +		unsigned int req_nr;

Maybe rename to `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));
>  		}
>  
> -		policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req),
> +		req_nr = policy->boost_supported ? 3 : 2;
> +		policy->min_freq_req = kzalloc(req_nr * sizeof(*policy->min_freq_req),
>  					       GFP_KERNEL);
>  		if (!policy->min_freq_req) {
>  			ret = -ENOMEM;
>  			goto out_destroy_policy;
>  		}
>  
> +		if (policy->boost_supported) {

I would rather add this last: min -> max -> boost.

And change the code in free as:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 58e3839a2140..7f5d18da78c6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1358,17 +1358,18 @@ 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 (policy->boost_freq_req) {
                /*
-                * Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
+                * Remove boost_freq_req after sending CPUFREQ_REMOVE_POLICY
                 * notification, since CPUFREQ_CREATE_POLICY notification was
-                * sent after adding max_freq_req earlier.
+                * sent after adding boost_freq_req earlier.
                 */
                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->boost_freq_req);
        }

+       freq_qos_remove_request(policy->max_freq_req);
        freq_qos_remove_request(policy->min_freq_req);
        kfree(policy->min_freq_req);


> +			policy->boost_freq_req = policy->min_freq_req + 2;
> +
> +			/*
> +			 * If boost is supported,
> +			 * init the constraint with cpuinfo.max_freq.
> +			 */

Don't need a comment for obvious code ?

> +			ret = freq_qos_add_request(&policy->constraints,
> +						   policy->boost_freq_req,
> +						   FREQ_QOS_MAX,
> +						   policy->cpuinfo.max_freq);
> +			if (ret < 0) {
> +				/*
> +				 * So we don't call freq_qos_remove_request() for an
> +				 * uninitialized request.
> +				 */

Actually we are calling freq_qos_remove_request() for NULL values. This comment
and other exiting ones like this can be removed I guess. They aren't adding much
value.

-- 
viresh