gpiolib: Refactor gpio_export

The gpio_export function uses nested if statements and the status variable to handle the failure cases. This makes the function logic difficult to follow. Refactor the code to abort immediately on failure using goto. This makes the code slightly longer, but significantly reduces the nesting and number of split lines and makes the code easier to read.

Signed-off-by Ryan Mallon

+++ b/drivers/gpio/gpiolib.c

drivers/gpio/gpiolib.c +0 -1

@@ -702,68 +702,74 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 {
        unsigned long           flags;
        struct gpio_desc        *desc;
-       int                     status = -EINVAL;

drivers/gpio/gpiolib.c +3 -21

+
+       if (!desc->chip->direction_input || !desc->chip->direction_output)
+               direction_may_change = false;
        spin_unlock_irqrestore(&gpio_lock, flags);
 
        if (desc->chip->names && desc->chip->names[gpio - desc->chip->base])
                ioname = desc->chip->names[gpio - desc->chip->base];
 
-       if (status == 0) {
-               struct device   *dev;
-
-               dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
-                               desc, ioname ? ioname : "gpio%u", gpio);
-               if (!IS_ERR(dev)) {
-                       status = sysfs_create_group(&dev->kobj,
-                                               &gpio_attr_group);
-
-                       if (!status && direction_may_change)
-                               status = device_create_file(dev,
-                                               &dev_attr_direction);
-
-                       if (!status && gpio_to_irq(gpio) >= 0
-                                       && (direction_may_change
-                                               || !test_bit(FLAG_IS_OUT,
-                                                       &desc->flags)))
-                               status = device_create_file(dev,
-                                               &dev_attr_edge);
-
-                       if (status != 0)

Quoted 8 lines · Ryan Mallon

The gpio_export function uses nested if statements and the status variable to handle the failure cases. This makes the function logic difficult to follow. Refactor the code to abort immediately on failure using goto. This makes the code slightly longer, but significantly reduces the nesting and number of split lines and makes the code easier to read.

Signed-off-by Ryan Mallon

Very good initiative!

Quoted 6 lines

+++ b/drivers/gpio/gpiolib.c

drivers/gpio/gpiolib.c +0 -1

@@ -702,68 +702,74 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 {
        unsigned long           flags;
        struct gpio_desc        *desc;
-       int                     status = -EINVAL;

drivers/gpio/gpiolib.c int gpio_export(unsigned gpio, bool direction_may_change) · line 713

Quoted 10 lines

drivers/gpio/gpiolib.c +4 -1

+       int                     status;
        const char              *ioname = NULL;
+       struct device           *dev;
 
        /* can't export until sysfs is available ... */
        if (!gpio_class.p) {
                pr_debug("%s: called too early!\n", __func__);
-               return -ENOENT;
+               status = -ENOENT;
+               goto fail;

Why bother with all the goto:s here since there are no resources to clean up? Just pr_debug() and return -ENOENT; is good enough.

I don't quite see the point.

Arguably this should be pr_err() or something BTW, just debug() may hide serious bugs.

Quoted 33 lines · Linus Walleij

Quoted 8 lines · Ryan Mallon

The gpio_export function uses nested if statements and the status variable to handle the failure cases. This makes the function logic difficult to follow. Refactor the code to abort immediately on failure using goto. This makes the code slightly longer, but significantly reduces the nesting and number of split lines and makes the code easier to read.

Signed-off-by Ryan Mallon

Very good initiative!

Quoted 6 lines

+++ b/drivers/gpio/gpiolib.c

drivers/gpio/gpiolib.c +0 -1

@@ -702,68 +702,74 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 {
        unsigned long           flags;
        struct gpio_desc        *desc;
-       int                     status = -EINVAL;

drivers/gpio/gpiolib.c int gpio_export(unsigned gpio, bool direction_may_change) · line 713

Quoted 10 lines

drivers/gpio/gpiolib.c +4 -1

+       int                     status;
        const char              *ioname = NULL;
+       struct device           *dev;
 
        /* can't export until sysfs is available ... */
        if (!gpio_class.p) {
                pr_debug("%s: called too early!\n", __func__);
-               return -ENOENT;
+               status = -ENOENT;
+               goto fail;

Why bother with all the goto:s here since there are no resources to clean up? Just pr_debug() and return -ENOENT; is good enough.

Quoted 1 line

I don't quite see the point.

I did it this way just so that there would be a single exit point. I don't mind either way, so I'll update the ones without any clean up to simply return.