Re: [PATCH] cpufreq: Documentation: fix freq_step description

[Pengjie Zhang]

On 5/30/2026 10:36 PM, Zhongqiu Han wrote:
> On 5/29/2026 7:11 PM, Pengjie Zhang wrote:
> > The conservative governor documentation incorrectly states that setting
> > freq_step to 0 will use the default 5% frequency step. In reality, since
> > the governor's initial implementation
> > commit b9170836d1aa ("[CPUFREQ] Conservative cpufreq governer"),
> > freq_step=0 has always caused the governor to skip frequency updates
> > entirely.
>
> Hi Pengjie,
>
> Thanks for the patch.
>
> The documentation fix looks correct: in the current code,
> cs_dbs_update() has an early goto out when freq_step == 0, which skips
> the call to get_freq_step() and all subsequent frequency change logic.
>
> However, the commit message's historical claim appears to be inaccurate.
> In the original implementation (b9170836d1aa), freq_step=0 had
> asymmetric behavior: frequency decreases were skipped (early return),
> but frequency increases still used the hardcoded 5% fallback (freq_step
> = 5 after the unlikely(freq_step == 0) check).
>
> If so, would it make sense to remove/update the historical claim to
> avoid the incorrect historical claim?
Thanks for the careful review.

Agreed. The correct commit for the symmetric freq_step=0 behavior
should be 8e677ce83bf4 ("[CPUFREQ] conservative: fixup governor to
function more like ondemand logic"), not b9170836d1aa.

I'll fix the commit message in v2.

On a related note, I have a quick question regarding code readability in
this area. Currently, the code uses the name "freq_step" for two different
concepts:

1. `cs_tuners->freq_step`: The tunable exposed via sysfs/documentation,
   which represents a percentage.
2. `freq_step = get_freq_step(...)`: The local variable representing the
   actual calculated frequency step (in kHz). The `if 
(unlikely(freq_step == 0))`
   check also applies to this absolute value.

Since mixing a percentage and an absolute kHz value under the same name
might be slightly confusing for readers, would it make sense to rename the
local variable (e.g., to `freq_step_khz`) to clearly distinguish the two?

Cheers,
    Pengjie

> > Correct the documentation to reflect the actual behavior: freq_step=0
> > disables frequency changes by the governor entirely.
> >
> > Fixes: 2a0e49279850 ("cpufreq: User/admin documentation update and 
> > consolidation")
> > Signed-off-by: Pengjie Zhang <zhangpengjie2@xxxxxxxxxx>
> > ---
> >   Documentation/admin-guide/pm/cpufreq.rst | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/pm/cpufreq.rst 
> > b/Documentation/admin-guide/pm/cpufreq.rst
> > index dbe6d23a5d67..98c724d49047 100644
> > --- a/Documentation/admin-guide/pm/cpufreq.rst
> > +++ b/Documentation/admin-guide/pm/cpufreq.rst
> > @@ -586,8 +586,8 @@ This governor exposes the following tunables:
> >       100 (5 by default).
> >         This is how much the frequency is allowed to change in one 
> > go.  Setting
> > -    it to 0 will cause the default frequency step (5 percent) to be 
> > used
> > -    and setting it to 100 effectively causes the governor to 
> > periodically
> > +    it to 0 disables frequency changes by the governor entirely and 
> > setting
> > +    it to 100 effectively causes the governor to periodically
> >       switch the frequency between the ``scaling_min_freq`` and
> >       ``scaling_max_freq`` policy limits.