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]

Date:   Mon, 18 May 2020 12:41:19 +0200
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>
Cc:     Viresh Kumar <viresh.kumar@...aro.org>,
        Serge Semin <fancer.lancer@...il.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.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: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@...aro.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!