linux-kernel - Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <10461949.HoJUxHt8jL@kreacher>
Date:   Mon, 18 May 2020 12:22:31 +0200
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Serge Semin <fancer.lancer@...il.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Arnd Bergmann <arnd@...db.de>,
        Rob Herring <robh+dt@...nel.org>, linux-mips@...r.kernel.org,
        devicetree@...r.kernel.org, stable@...r.kernel.org,
        Frederic Weisbecker <frederic@...nel.org>,
        Ingo Molnar <mingo@...nel.org>, Yue Hu <huyue2@...ong.com>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

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)