Messages in this thread |  | | From | "Rafael J. Wysocki" <> | Date | Thu, 27 Jun 2019 11:52:04 +0200 | Subject | Re: [PATCH V2 2/5] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target() |
| |
On Thu, Jun 27, 2019 at 7:00 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 20-06-19, 08:35, Viresh Kumar wrote: > > > CPUFREQ_CONST_LOOPS was introduced in a very old commit from pre-2.6 > > > kernel release commit 6a4a93f9c0d5 ("[CPUFREQ] Fix 'out of sync' > > > issue"). > > > > > > Probably the initial idea was to just avoid these checks for set_policy > > > type drivers and then things got changed over the years. And it is very > > > unclear why these checks are there at all. > > > > > > Replace the CPUFREQ_CONST_LOOPS check with has_target(), which makes > > > more sense now. > > > > > > cpufreq_notify_transition() is only called for has_target() type driver > > > and not for set_policy type, and the check is simply redundant. Remove > > > it as well. > > > > > > Also remove () around freq comparison statement as they aren't required > > > and checkpatch also warns for them. > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > --- > > > drivers/cpufreq/cpufreq.c | 13 +++++-------- > > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > index 54befd775bd6..41ac701e324f 100644 > > > --- a/drivers/cpufreq/cpufreq.c > > > +++ b/drivers/cpufreq/cpufreq.c > > > @@ -359,12 +359,10 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy, > > > * which is not equal to what the cpufreq core thinks is > > > * "old frequency". > > > */ > > > - if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) { > > > - if (policy->cur && (policy->cur != freqs->old)) { > > > - pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n", > > > - freqs->old, policy->cur); > > > - freqs->old = policy->cur; > > > - } > > > + if (policy->cur && policy->cur != freqs->old) { > > > + pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n", > > > + freqs->old, policy->cur); > > > + freqs->old = policy->cur; > > > } > > > > > > srcu_notifier_call_chain(&cpufreq_transition_notifier_list, > > > @@ -1618,8 +1616,7 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy) > > > if (policy->fast_switch_enabled) > > > return ret_freq; > > > > > > - if (ret_freq && policy->cur && > > > - !(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) { > > > + if (has_target() && ret_freq && policy->cur) { > > > /* verify no discrepancy between actual and > > > saved value exists */ > > > if (unlikely(ret_freq != policy->cur)) { > > @Rafael: Here are your comments from the IRC exchange we had > yesterday: > > > <rafael>: > > > > so the problem is that, because of the CPUFREQ_CONST_LOOPS check in > > __cpufreq_get(), it almost never does the cpufreq_out_of_sync() thing > > now. Because many drivers set CPUFREQ_CONST_LOOPS most of the time, > > some of them even unconditionally. This patch changes the code that > > runs very rarely into code that runs relatively often. > > Right, we will do the frequency verification on has_target() platforms > with CPUFREQ_CONST_LOOPS set after this patch. But why is it the wrong > thing to do ?
Well, my point was exactly what I said.
The patch pretended to be a cleanup and changed the code in a meaningful way (at least for some drivers).
> What we do here is that we verify that the cached value of current > frequency is same as the real frequency the hardware is running at. It > makes sense to not do this check for setpolicy type drivers as the > cpufreq core isn't always aware of what the driver will end up doing > with the frequency and so no verification. > > But for has_target() type drivers, cpufreq core caches the value with > it and it should check it to make sure everything is fine. I don't see > a correlation with CPUFREQ_CONST_LOOPS flag here, that's it. Either we > do this verification or we don't, but there is no reason (as per my > understanding) of skipping it using this flag. > > So if you look at the commit I pointed in the history git [1], it does > two things: > - It adds the verification code (which is quite similar today as > well). > - And it sets the CPUFREQ_CONST_LOOPS flag only for setpolicy drivers, > rightly so. > > The problem happened when we started to use CPUFREQ_CONST_LOOPS for > constant loops-per-jiffy thing as well and many has_target() drivers > started using the same flag and unknowingly skipped the verification > of frequency. > > So, I think the current code is doing the wrong thing by skipping the > verification using CPUFREQ_CONST_LOOPS flag.
All right then, thanks for explaining it here.
The patch is a bug fix, not a cleanup, and it fixes the changes that caused CPUFREQ_CONST_LOOPS to be used for a different purpose without adjusting the original code accordingly.
I can agree with this rationale, but please fix the changelog.
|  |