| Message ID | 065301260510fbca81f5481b27b0de956073068a.1469137133.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b696baeb249d..3ef9be3965ff 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -507,12 +507,20 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, { target_freq = clamp_val(target_freq, policy->min, policy->max); policy->cached_target_freq = target_freq; + + if (cpufreq_driver->target_index) { + int idx; + + idx = cpufreq_frequency_table_target(policy, target_freq, + CPUFREQ_RELATION_L); + policy->cached_resolved_idx = idx; + return policy->freq_table[idx].frequency; + } + if (cpufreq_driver->resolve_freq) return cpufreq_driver->resolve_freq(policy, target_freq); - policy->cached_resolved_idx = - cpufreq_frequency_table_target(policy, target_freq, - CPUFREQ_RELATION_L); - return policy->freq_table[policy->cached_resolved_idx].frequency; + + return target_freq; } /*********************************************************************
The handlers provided by cpufreq core are sufficient for resolving the frequency for drivers providing ->target_index(), as the core already has the frequency table and so ->resolve_freq() isn't required for such platforms. This patch disallows drivers with ->target_index() callback to use the ->resolve_freq() callback. Also, it fixes a potential kernel crash for drivers providing ->target() but no ->resolve_freq(). Fixes: e3c062360870 ("cpufreq: add cpufreq_driver_resolve_freq()") Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V2: - s/UINT_MAX/target_freq drivers/cpufreq/cpufreq.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) -- 2.7.4