linux-kernel - Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

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] [day] [month] [year] [list]

Message-ID: <20180704105002.GD496@dell>
Date:   Wed, 4 Jul 2018 11:53:06 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     Enric Balletbo Serra <eballetbo@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Sebastian Reichel <sre@...nel.org>, chenjh@...k-chips.com,
        Andrey Smirnov <andrew.smirnov@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        Heiko Stübner <heiko@...ech.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        sboyd@...nel.org, linux-clk@...r.kernel.org,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-input@...r.kernel.org, mikko.mutanen@...rohmeurope.com,
        heikki.haikola@...rohmeurope.com
Subject: Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

On Wed, 04 Jul 2018, Matti Vaittinen wrote:

> On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> > On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> > 
> > > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > > Missatge de Matti Vaittinen <matti.vaittinen@...rohmeurope.com> del
> > > > > dia dv., 29 de juny 2018 a les 11:47:
> > > > > 
> > > > > Now that you use devm calls and you don't need to unwind things I
> > > > > think is better to use plain returns. So,
> > > > > 
> > > > > return -ENOMEM;
> > > > 
> > > > I have never really understood why use of gotos in error handling is
> > > > discouraged.
> > 
> > They're not.
> > 
> > > > Personally I would always choose single point of exit from
> > > > a function when it is simple enough to achieve (like in this case). I've
> > > > written and fixed way too many functions which leak resources or
> > > > accidentally keep a lock when exiting from error branches. But I know
> > > > many colleagues like you who prefer not to have gotos but  in place returns
> > > > instead. So I guess I'll leave the final call on this to the one who is
> > > > maintainer for this code. And it is true there is no things to unwind
> > > > now - which does not mean that next updater won't add such. But as I
> > > > said, I know plenty of people share your view - and even though I rather
> > > > maintain code with only one exit the final call is on subsystem maintainer
> > > > here.
> > 
> > Please use gotos in the error path.
> > 
> > IMO, it's the nicest way to unwind (as you call it).
> 
> I'll keep the gotos but clean other stuff for patch v9 then.

Sounds good.

> > > Actually, If it was completely my call the probe would look something
> > > like this:
> > > 
> > > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > > +                           const struct i2c_device_id *id)
> > > +{

[...]

> > > +}
> > > 
> > > What do you think of this? To my eye it is nice. It keeps single point of
> > > exit and introduces only simple if-statements without the need of curly
> > > brackets. And finally the error prints string works as a comment too.
> > > I've seen bunch of constructs like this on the networking side but I
> > > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > > is just to illustrate the idea, I did not even try compiling it yet.
> > 
> > That is horrible.  I nearly vomited on my keyboard. 
> 
> Note to self: Never to buy second hand keyboard from Lee =)

That is sound advice.

Not sure I would buy 2nd-hand keyboard from anyone - ewe! :\

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog