Re: [PATCH v2 1/1] cpufreq: fix the target freq not in the range of policy->min & max

[Rafael J. Wysocki]

On Tue, Jun 29, 2021 at 8:18 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 27-06-21, 00:23, TungChen Shih wrote:
> >     The function cpufreq_driver_resolve_freq() should return the lowest
>
> Don't add extra spaces at the beginning of paragraphs here.
>
> > supported freq greater than or equal to the given target_freq, subject
> > to policy (min/max) and driver limitations. However, the index returned
> > by cpufreq_frequency_table_target() won't subject to policy min/max in
> > some cases.
> >
> >     In cpufreq_frequency_table_target(), this function will try to find
> > an index for @target_freq in freq_table, and the frequency of selected
> > index should be in the range [policy->min, policy->max], which means:
> >
> >     policy->min <= policy->freq_table[idx].frequency <= policy->max
> >
> >     Though "clamp_val(target_freq, policy->min, policy->max);" would
> > have been called to check this condition, when policy->max or min is
> > not exactly one of the frequency in the frequency table,
> > policy->freq_table[idx].frequency may still go out of the range
> >
> >     For example, if our sorted freq_table is [3000, 2000, 1000], and
> > suppose we have:
> >
> >     @target_freq = 2500
> >     @policy->min = 2000
> >     @policy->max = 2200
> >     @relation = CPUFREQ_RELATION_L
> >
> > 1. After clamp_val(target_freq, policy->min, policy->max); @target_freq
> > becomes 2200
> > 2. Since we use CPUFREQ_REALTION_L, final selected freq will be 3000 which
> > beyonds policy->max
>
> Right so the problem does exist,

That IMO is a matter for discussion and the patch author seems to have
decided to ignore my previous comments.

> and not only with
> cpufreq_driver_resolve_freq(), but __cpufreq_driver_target() as well.

That all depends on what the policy min and max limits are expected to
mean and so far the interpretation has been that they are applied to
the target frequency coming from the governor.

Drivers have never been expected to ensure that the final effective
frequency will always be between the policy min and max and, indeed,
they may not even be able to ensure that.

Now, because RELATION_L is defined as "the closest frequency equal to
or above the target", running at a frequency below the target is
questionable even if the max limit gets in the way.  IOW, RELATION_L
takes precedence over the policy max limit.

Accordingly, I'm not going to apply this patch or anything similar to
it until I'm given a really convincing argument otherwise.

> I have a sent a patchset to update both of these to start sharing some
> code and we need to fix this for both now.
>
> > Signed-off-by: TungChen Shih <tung-chen.shih@xxxxxxxxxxxx>
> > ---
> >  drivers/cpufreq/cpufreq.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 802abc925b2a..8e3a17781618 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -544,8 +544,23 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> >       if (cpufreq_driver->target_index) {
> >               unsigned int idx;
> >
> > +             /*  to find the frequency >= target_freq */
> >               idx = cpufreq_frequency_table_target(policy, target_freq,
> >                                                    CPUFREQ_RELATION_L);
> > +
> > +             /* frequency should subject to policy (min/max) */
> > +             if (policy->freq_table[idx].frequency > policy->max) {
> > +                     if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
> > +                             idx--;
> > +                     else
> > +                             idx++;
> > +             } else if (policy->freq_table[idx].frequency < policy->min) {
> > +                     if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
> > +                             idx++;
> > +                     else
> > +                             idx--;
> > +             }
>
> This doesn't look clean to be honest.
>
> Rafael, does it make sense to update cpufreq_frequency_table_target()
> (and its internal routines) to take policy bounds in consideration, or
> something else ?
>
> --
> viresh