Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

On Tuesday, June 07, 2016 09:58:07 AM Viresh Kumar wrote:

On 06-06-16, 23:56, Rafael J. Wysocki wrote:

quoted

Since you are adding new code, you can write it so it doesn't do
unnecessary checks from the start.

Hmm, I will do all that in this series only now.

quoted

While at it, the "if ((freq < policy->min) || (freq > policy->max))"
checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look
good to me, because they very well may cause those function to return
-EINVAL even when there's a valid table and that may cause
acpi_cpufreq_fast_switch() to do bad things.

Hmm. So, the checks are for sure required here, otherwise we may end up
returning a frequency which we aren't allowed to. Also note that 'freq' here
isn't the target-freq, but the entry in the freq-table.

This routine should be returning a valid freq within the ranges specified by
policy->min/max.

Which in principle may not be possible if the range doesn't include any
frequency in the table, eg. min == max and between the table entries.

However, the CPU has to run at *some* frequency, even if there's none in the
min/max range.

And if we are sure that there is at least one valid frequency between min
and max, please note that target_freq has already been clamped between them,
so clamping again is rather unuseful.  And of course it is racy in general,
which makes it even more unuseful.

Also note that these routines shall *never* return -EINVAL, otherwise it is
mostly a bug we are hitting.

So make them explicitly return a valid frequency every time.

We have enough checks in place to make sure that there is at least one valid
entry in the freq-table which is >= policy->min and <= policy->max.

That assuming that the driver will always do the right thing in its ->verify
callback.

I will take care of rest of the comments though. Thanks.

Thanks!