linux-kernel - Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table

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:	Tue, 11 Aug 2015 05:53:42 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Stephen Boyd <sboyd@...eaurora.org>
Cc:	Rafael Wysocki <rjw@...ysocki.net>, nm@...com,
	linaro-kernel@...ts.linaro.org, linux-pm@...r.kernel.org,
	khilman@...aro.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Len Brown <len.brown@...el.com>,
	open list <linux-kernel@...r.kernel.org>,
	Pavel Machek <pavel@....cz>
Subject: Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

On 10-08-15, 12:23, Stephen Boyd wrote:
> On 08/10, Viresh Kumar wrote:
> > 'dev_opp' will always be NULL in _of_init_opp_table_v2() after creating
> > OPPs for a device. There is no point comparing it against NULL there.
> > 
> > Restructure code a bit to make it more efficient.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> 
> Curious if these are a response to the static checker mails? If
> so it would be good to add a reported-by tag.

No it wasn't, I had these waiting in my queue even before Rafael
applied the earlier ones. I was waiting for them to get in before
sending more stuff :)

> > ---
> >  drivers/base/power/opp.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index 1daaa1a418a2..c9747fb192b1 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -1295,20 +1295,19 @@ static int _of_init_opp_table_v2(struct device *dev,
> >  	if (WARN_ON(!count))
> >  		goto out;
> >  
> > -	if (!ret) {
> > -		if (!dev_opp) {
> > -			dev_opp = _find_device_opp(dev);
> > -			if (WARN_ON(!dev_opp))
> > -				goto out;
> > -		}
> > -
> > -		dev_opp->np = opp_np;
> > -		dev_opp->shared_opp = of_property_read_bool(opp_np,
> > -							    "opp-shared");
> > -	} else {
> > +	if (ret) {
> >  		of_free_opp_table(dev);
> > +		goto out;
> >  	}
> >  
> > +	dev_opp = _find_device_opp(dev);
> > +	if (WARN_ON(!dev_opp))
> > +		goto out;
> 
> Doesn't ret = 0 in this case?

Because ret is already 0, juse see the above if (ret) check.

> Why not drop the goto and just
> return some error code. Same for the goto out up above.

Actually yes, because we don't do anything special in goto now. But it
required more (unrelated) code changes, plus I didn't wanted to break
the 'return from single place' rule for this function, in case we
really need to free some resource or undo some work from the goto
place.

But if you suggest/insist, then I will do it in a separate patch.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/