On Wed, 04 Jul 2018, Matti Vaittinen wrote:

On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:

quoted

On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:

quoted

Missatge de Matti Vaittinen [off-list ref] 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.

quoted

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).

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.  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.

Landing a goto in the middle of a statement is messy and unsightly.

What happens when you have some resources to free?  The last few lines
will become very messy, very quickly.

Nit: "something == NULL" is better written as "!something".
Nit: Please use proper multi-line comments as per the Coding Style.

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