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
