On 08-06-16, 02:38, Rafael J. Wysocki wrote:
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.
By within ranges I meant, policy->min <= freq <= policy->max, and that's how all our checks are. So even if the table will have a single valid frequency, we will return that only.
However, the CPU has to run at *some* frequency, even if there's none in the min/max range.
I completely agree. But the error will be fired only if there is no frequency within ranges we can switch to. And that's a bug somewhere else then.
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,
Yeah, its already clamped by the freq-change helpers in cpufreq core, but others may not be doing it properly.
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.
I thought about return Index 0 on such errors, will that be fine ? Anyway the new patches have added a WARN() for such cases.
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.
Yeah.