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@xxxxxxxxxxxxxxxxx> 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.
>
> > 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)
> > +{
> > + struct bd71837 *bd71837;
> > + struct bd71837_board *board_info;
> > + int gpio_intr = 0;
> > +
> > + const char *errstr = "No IRQ configured";
> > + int ret = -EINVAL;
> > +
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > +
> > + if (bd71837 == NULL)
> > + return -ENOMEM;
> > +
> > + board_info = dev_get_platdata(&i2c->dev);
> > +
> > + if (!board_info)
> > + gpio_intr = i2c->irq;
> > + else
> > + gpio_intr = board_info->gpio_intr;
> > +
> > + if (!gpio_intr)
> > + goto err_out;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = gpio_intr;
> > +
> > + errstr = "regmap initialization failed";
> > +
> > + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> > + ret = PTR_ERR(bd71837->regmap);
> > + if (IS_ERR(bd71837->regmap))
> > + goto err_out;
> > +
> > + errstr = "Read BD71837_REG_DEVICE failed";
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to add irq_chip";
> > + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> > + bd71837->chip_irq, IRQF_ONESHOT, 0,
> > + &bd71837_irq_chip, &bd71837->irq_data);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to configure button short press timeout";
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG0,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + /* According to BD71847 datasheet the HW default for long press
> > + * detection is 10ms. So lets change it to 10 sec so we can actually
> > + * get the short push and allow gracefull shut down
> > + */
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG1,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_LONG_PRESS_10S);
> > +
> > + errstr = "Failed to configure button long press timeout";
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> > + BD71837_INT_PWRBTN_S);
> > +
> > + errstr = "Failed to get the IRQ";
> > + ret = btns[0].irq;
> > + if (btns[0].irq < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to create subdevices";
> > + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > + bd71837_mfd_cells,
> > + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> > + regmap_irq_get_domain(bd71837->irq_data));
> > + if (ret) {
> > +err_out:
> > + if (errstr)
> > + dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
> > + }
> > +
> > + return ret;
> > +}
> >
> > 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 =)
> It doesn't flow
> anywhere nearly as nicely has sorting out all of the error handling
> *after* it has been detected. You're sacrificing readability to save
> a single line and do not save any *actual* lines of code, only a brace.
I was expecting something like this comment =) But the truth is that one
gets used to reading this quickly. Well, this still sounds like I should
not try convincing you - so you can stay heretic ;)
>
> Landing a goto in the middle of a statement is messy and unsightly.
This is another thing one gets used to. I've actually seen plenty of
code using
if (0) {
error_label:
....
}
for error handling. But again - you can keep your view and I'll adopt to
it here =)
> What happens when you have some resources to free? The last few lines
> will become very messy, very quickly.
One can just build the usual clean-up sequence inside the last if (ret)
using different goto lables. Eg:
if (ret) {
err_unwind_X:
undo_x();
err_unwind_Y:
undo_y();
err_out:
dev_err(...);
}
>
> Nit: "something == NULL" is better written as "!something".
Oh, I personally liked the !foo more as Enric - but I will write the
NULL in case it won't make line too long. This is not a big deal to me.
> Nit: Please use proper multi-line comments as per the Coding Style.
Will do.
Thanks for quick reply! I will send new version today or tomorrow.
Best Regards
Matti Vaittinen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
