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.ku...@linaro.org> -- viresh