Re: [PATCH] cpufreq: Use a smaller freq for the policy->max when verify

From: Dhruva Gole <d-gole@ti.com>
To: Xuewen Yan <xuewen.yan@unisoc.com>
Cc: <rafael@kernel.org>, <viresh.kumar@linaro.org>,
	<ke.wang@unisoc.com>, <linux-pm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <xuewen.yan94@gmail.com>,
	<di.shen@unisoc.com>
Subject: Re: [PATCH] cpufreq: Use a smaller freq for the policy->max when verify
Date: Wed, 20 Mar 2024 10:53:56 +0530	[thread overview]
Message-ID: <20240320052356.6ogxfa6teocp7br6@dhruva> (raw)
In-Reply-To: <20240319080153.3263-1-xuewen.yan@unisoc.com>

Hi,

On Mar 19, 2024 at 16:01:53 +0800, Xuewen Yan wrote:
> When driver use the cpufreq_frequency_table_verify() as the
> cpufreq_driver->verify's callback. It may cause the policy->max
> bigger than the freq_qos's max freq.
> 
> Just as follow:
> 
> unisoc:/sys/devices/system/cpu/cpufreq/policy0 # cat scaling_available_frequencies
> 614400 768000 988000 1228800 1469000 1586000 1690000 1833000 2002000 2093000
> 
> unisoc:/sys/devices/system/cpu/cpufreq/policy0 # echo 1900000 > scaling_max_freq
> unisoc:/sys/devices/system/cpu/cpufreq/policy0 # echo 1900000 > scaling_min_freq
> unisoc:/sys/devices/system/cpu/cpufreq/policy0 # cat scaling_max_freq
> 2002000
> unisoc:/sys/devices/system/cpu/cpufreq/policy0 # cat scaling_min_freq
> 2002000
> 
> When user set the qos_min and qos_max as the same value, and the value
> is not in the freq-table, the above scenario will occur.
> 
> This is because in cpufreq_frequency_table_verify() func, when it can not
> find the freq in table, it will change the policy->max to be a bigger freq,
> as above, because there is no 1.9G in the freq-table, the policy->max would
> be set to 2.002G. As a result, the cpufreq_policy->max is bigger than the
> user's qos_max. This is unreasonable.

That's a good catch! Never thought of this.

> 
> So use a smaller freq when can not find the freq in fre-table, to prevent
> the policy->max exceed the qos's max freq.
> 
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
> ---
>  drivers/cpufreq/freq_table.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index c4d4643b6ca6..1d98b8cf1688 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -70,7 +70,7 @@ int cpufreq_frequency_table_verify(struct cpufreq_policy_data *policy,
>  				   struct cpufreq_frequency_table *table)
>  {
>  	struct cpufreq_frequency_table *pos;
> -	unsigned int freq, next_larger = ~0;
> +	unsigned int freq, prev_smaller = 0;
>  	bool found = false;
>  
>  	pr_debug("request for verification of policy (%u - %u kHz) for cpu %u\n",
> @@ -86,12 +86,12 @@ int cpufreq_frequency_table_verify(struct cpufreq_policy_data *policy,
>  			break;
>  		}
>  
> -		if ((next_larger > freq) && (freq > policy->max))
> -			next_larger = freq;
> +		if ((prev_smaller < freq) && (freq <= policy->max))
> +			prev_smaller = freq;
>  	}
>  
>  	if (!found) {
> -		policy->max = next_larger;
> +		policy->max = prev_smaller;
>  		cpufreq_verify_within_cpu_limits(policy);

LGTM!
Reviewed-by: Dhruva Gole <d-gole@ti.com>

-- 
Best regards,
Dhruva