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

[Zhongqiu Han]

On 3/12/2026 6:27 PM, Viresh Kumar wrote:
> On 10-03-26, 21:17, Zhongqiu Han wrote:
> > 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")?
>
> Thanks for pointing this, I missed this change.
>
> > 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.
>
> Its the opposite I think. The governor stays within a range and never
> goes to a higher or lower frequency. This is how I think this happens:
> - Lets say frequencies are from 1GHz to 2GHz with a gap of 200 MHz.
> - Lets say the frequency is 1 GHz now and the step size is 100 MHz.
> - conservative governor will try to change the freq to 1.1 GHz and end
>    up selecting 1 GHz only (due to CPUFREQ_RELATION_H, highest freq
>    below 1.1 GHz).
> - With my patch, we will keep resetting to 1 GHz (cur freq) and never
>    change freq.
> - With a recorded requested_freq, we will move to 1.1 GHz (actual 1
>    GHz), but a subsequent call will go for 1.2 GHz.
>
> Here is another idea, once everyone agrees I can send this formally:

Hi Viresh,

Thanks a lot for the detailed explanation — that makes sense. I agree
that my earlier concern was not entirely accurate, and your explanation
clarifies the behavior well.

Overall, this patch looks like a good balance between avoiding stalling
with small freq_step on discrete frequency tables and fixing
requested_freq drift from policy->cur due to out-of-band frequency
changes.

LGTM from my side.


> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index e0e847764511..df01d33993d8 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -313,6 +313,17 @@ static void cs_start(struct cpufreq_policy *policy)
>          dbs_info->requested_freq = policy->cur;
>   }
>
> +static void cs_limits(struct cpufreq_policy *policy)
> +{
> +       struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
> +
> +       /*
> +        * The limits have changed, so may have the current frequency. Reset
> +        * requested_freq to avoid any unintended outcomes due to the mismatch.
> +        */
> +       dbs_info->requested_freq = policy->cur;
> +}
> +
>   static struct dbs_governor cs_governor = {
>          .gov = CPUFREQ_DBS_GOVERNOR_INITIALIZER("conservative"),
>          .kobj_type = { .default_groups = cs_groups },
> @@ -322,6 +333,7 @@ static struct dbs_governor cs_governor = {
>          .init = cs_init,
>          .exit = cs_exit,
>          .start = cs_start,
> +       .limits = cs_limits,
>   };
>
>   #define CPU_FREQ_GOV_CONSERVATIVE      (cs_governor.gov)
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 36eb7aee4bcd..acf101878733 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -563,6 +563,7 @@ EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_stop);
>
>   void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
>   {
> +       struct dbs_governor *gov = dbs_governor_of(policy);
>          struct policy_dbs_info *policy_dbs;
>
>          /* Protect gov->gdbs_data against cpufreq_dbs_governor_exit() */
> @@ -574,6 +575,8 @@ void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
>          mutex_lock(&policy_dbs->update_mutex);
>          cpufreq_policy_apply_limits(policy);
>          gov_update_sample_delay(policy_dbs, 0);
> +       if (gov->limits)
> +               gov->limits(policy);
>          mutex_unlock(&policy_dbs->update_mutex);
>
>   out:
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index 168c23fd7fca..1462d59277bd 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -138,6 +138,7 @@ struct dbs_governor {
>          int (*init)(struct dbs_data *dbs_data);
>          void (*exit)(struct dbs_data *dbs_data);
>          void (*start)(struct cpufreq_policy *policy);
> +       void (*limits)(struct cpufreq_policy *policy);
>   };
>
>   static inline struct dbs_governor *dbs_governor_of(struct cpufreq_policy *policy)


--
Thx and BRs,
Zhongqiu Han