Re: [PATCH] cpufreq: conservative: Drop cached requested_freq

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

On 3/10/2026 2:34 PM, Viresh Kumar wrote:
> A recently reported issue highlighted that the cached requested_freq
> is not guaranteed to stay in sync with policy->cur. If the platform
> changes the actual CPU frequency after the governor sets one (e.g.
> due to platform-specific frequency scaling) and a re-sync occurs
> later, policy->cur may diverge from requested_freq.
> 
> This can lead to incorrect behavior in the conservative governor.
> For example, the governor may assume the CPU is already running at
> the maximum frequency and skip further increases even though there
> is still headroom.
> 
> Avoid this by dropping the cached requested_freq and using
> policy->cur directly.

Hi Viresh,

Thanks for the patch. The fix looks correct to me for the reported
issue. I do have one question though - should we also consider the
interaction with commit abb6627910a1 ("cpufreq: conservative: Fix
next frequency selection")?

The cached requested_freq can diverge from policy->cur when the
platform may change the actual CPU frequency outside of cpufreq
interfaces, causing cs_dbs_update() to incorrectly assume the CPU is
already at policy->max and skip frequency increases. Using policy->cur
directly avoids this.

This direction was already argued for in commit d352cf47d93e
("cpufreq: conservative: Do not use transition notifications"), which
noted that "policy->cur can be used instead of [requested_freq] and
then the governor will not have to worry about updating the tracked
value when the current frequency changes independently."

However, that change was subsequently reverted by commit abb6627910a1
("cpufreq: conservative: Fix next frequency selection"), which noted
that using policy->cur directly broke the algorithm when freq_step is
small relative to the distances between available frequencies. In that
case, the governor may not be able to stay within a narrow range
between two consecutive available frequencies and instead jumps through
steps faster than intended.

May I know am I missing something here, or does this patch potentially
reintroduce the same issue? Thank you



> 
> Reported-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> Link: https://lore.kernel.org/all/20260210115458.3493646-1-zhenglifeng1@huawei.com/
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Lifeng Zheng, can you please give this a try and provide your Tested-by as well
> ?
> 
>   drivers/cpufreq/cpufreq_conservative.c | 16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index e0e847764511..c69577e4f941 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -14,7 +14,6 @@
>   struct cs_policy_dbs_info {
>   	struct policy_dbs_info policy_dbs;
>   	unsigned int down_skip;
> -	unsigned int requested_freq;
>   };
>   
>   static inline struct cs_policy_dbs_info *to_dbs_info(struct policy_dbs_info *policy_dbs)
> @@ -59,10 +58,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   {
>   	struct policy_dbs_info *policy_dbs = policy->governor_data;
>   	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
> -	unsigned int requested_freq = dbs_info->requested_freq;
>   	struct dbs_data *dbs_data = policy_dbs->dbs_data;
>   	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
>   	unsigned int load = dbs_update(policy);
> +	unsigned int requested_freq = policy->cur;
>   	unsigned int freq_step;
>   
>   	/*
> @@ -72,16 +71,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   	if (cs_tuners->freq_step == 0)
>   		goto out;
>   
> -	/*
> -	 * If requested_freq is out of range, it is likely that the limits
> -	 * changed in the meantime, so fall back to current frequency in that
> -	 * case.
> -	 */
> -	if (requested_freq > policy->max || requested_freq < policy->min) {
> -		requested_freq = policy->cur;
> -		dbs_info->requested_freq = requested_freq;
> -	}
> -
>   	freq_step = get_freq_step(cs_tuners, policy);
>   
>   	/*
> @@ -113,7 +102,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   
>   		__cpufreq_driver_target(policy, requested_freq,
>   					CPUFREQ_RELATION_HE);
> -		dbs_info->requested_freq = requested_freq;
>   		goto out;
>   	}
>   
> @@ -137,7 +125,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   
>   		__cpufreq_driver_target(policy, requested_freq,
>   					CPUFREQ_RELATION_LE);
> -		dbs_info->requested_freq = requested_freq;
>   	}
>   
>    out:
> @@ -310,7 +297,6 @@ static void cs_start(struct cpufreq_policy *policy)
>   	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
>   
>   	dbs_info->down_skip = 0;
> -	dbs_info->requested_freq = policy->cur;
>   }
>   
>   static struct dbs_governor cs_governor = {


-- 
Thx and BRs,
Zhongqiu Han