On Mon, May 18, 2020 at 12:56 PM Serge Semin Sergey.Semin@baikalelectronics.ru wrote:
On Mon, May 18, 2020 at 12:51:15PM +0200, Rafael J. Wysocki wrote:
On Mon, May 18, 2020 at 12:46 PM Serge Semin Sergey.Semin@baikalelectronics.ru wrote:
On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
On 18-05-20, 12:22, Rafael J. Wysocki wrote: > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > That said if you really only want it to return 0 on success, you may as well > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > the last break in the loop. > > > > That can be done as well, but will be a bit less efficient as the loop > > will execute once for each policy, and so the statement will run > > multiple times. Though it isn't going to add any significant latency > > in the code. > > Right. > > However, the logic in this entire function looks somewhat less than > straightforward to me, because it looks like it should return an > error on the first policy without a frequency table (having a frequency > table depends on the driver and that is the same for all policies, so it > is pointless to iterate any further in that case). > > Also, the error should not be -EINVAL, because that means "invalid > argument" which would be the state value. > > So I would do something like this: > > --- > drivers/cpufreq/cpufreq.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > static int cpufreq_boost_set_sw(int state) > { > struct cpufreq_policy *policy; > - int ret = -EINVAL; > > for_each_active_policy(policy) { > + int ret; > + > if (!policy->freq_table) > - continue; > + return -ENXIO; > > ret = cpufreq_frequency_table_cpuinfo(policy, > policy->freq_table); > if (ret) { > pr_err("%s: Policy frequency update failed\n", > __func__); > - break; > + return ret; > } > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > if (ret < 0) > - break; > + return ret; > } > > - return ret; > + return 0; > } > > int cpufreq_boost_trigger_state(int state)
Acked-by: Viresh Kumar viresh.kumar@linaro.org
Ok. Thanks for the comments. Shall I resend the patch with update Rafael suggests or you'll merge the Rafael's fix in yourself?
I'll apply the fix directly, thanks!
Great. Is it going to be available in the repo: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/ ?
Yes, it is. Please see the bleeding-edge branch in there, thanks!
No credits with at least Reported-by tag? That's sad.(
OK, done now, but you are not the only reported of it, so I've added the other reporter too.
Thanks!