On Fri, Jul 22, 2016 at 01:53:13AM +0200, Rafael J. Wysocki wrote:
On Fri, Jul 22, 2016 at 1:45 AM, Steve Muckle steve.muckle@linaro.org wrote:
On Fri, Jul 22, 2016 at 01:32:00AM +0200, Rafael J. Wysocki wrote:
On Fri, Jul 22, 2016 at 1:22 AM, Steve Muckle steve.muckle@linaro.org wrote:
On Fri, Jul 22, 2016 at 01:22:22AM +0200, Rafael J. Wysocki wrote:
OK, applied.
FWIW I do have a concern on this patch, I think it adds unnecessary overhead.
It isn't unnecessary. It prevents an otherwise possible kernel crash from happening.
The logic may not be unecessary, but the overhead is. The crash could be prevented in a way that doesn't require repeatedly checking a pointer that doesn't change.
Well, you had the ->resolve_freq check in your patch, didn't you?
Viresh simply added a ->target_index check to it.
Now, you can argue that this is one check too many, but as long as drivers are allowed to implement ->target without implementing ->resolve_freq, the *number* of checks in this routine cannot be reduced.
There are three possible cases and two checks are required to determine which case really takes place.
My thinking was that one of these two would be preferable:
- Forcing ->target() drivers to install a ->resolve_freq callback, enforcing this at cpufreq driver init time. My understanding is ->target() drivers are deprecated anyway and theren't aren't many of them, though I don't know offhand exactly how many or how hard it would be to do for each one.
- Forcing callers (schedutil in this case) to check that either ->target() or ->resolve_freq() is implemented. It means catching and scrutinizing future callers of resolve_freq.
But even if one of these is better than it could always be done on top of this patch I suppose. I'm also not familiar with the platforms that use ->target() style drivers. So strictly speaking for my purposes it won't matter since the number of tests is the same for them.