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?

-Sergey

-- viresh