On Tuesday, June 07, 2016 09:58:07 AM Viresh Kumar wrote:
On 06-06-16, 23:56, Rafael J. Wysocki wrote:
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.
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!