On Wed, Oct 17, 2012 at 1:41 AM, Ryan Mallon <rmal...@gmail.com> wrote:
> 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!
> +++ b/drivers/gpio/gpiolib.c
> @@ -702,68 +702,74 @@ int gpio_export(unsigned gpio, bool
> direction_may_change)
> {
> unsigned long flags;
> struct gpio_desc *desc;
> - int status = -EINVAL;
> + 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.
> - if (!gpio_is_valid(gpio))
> - goto done;
> + if (!gpio_is_valid(gpio)) {
> + status = -EINVAL;
> + goto fail;
> + }
Why not just pr_err(..); return -EINVAL; ?
> - if (test_bit(FLAG_REQUESTED, &desc->flags)
> - && !test_bit(FLAG_EXPORT, &desc->flags)) {
> - status = 0;
> - if (!desc->chip->direction_input
> - || !desc->chip->direction_output)
> - direction_may_change = false;
> + if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
> + test_bit(FLAG_EXPORT, &desc->flags)) {
> + spin_unlock_irqrestore(&gpio_lock, flags);
> + status = -ENOENT;
> + goto fail;
I would just print and return here as well.
> +
> + 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)
> - device_unregister(dev);
> - } else
> - status = PTR_ERR(dev);
> - if (status == 0)
> - set_bit(FLAG_EXPORT, &desc->flags);
> + dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
> + desc, ioname ? ioname : "gpio%u", gpio);
> + if (IS_ERR(dev)) {
> + status = PTR_ERR(dev);
> + goto fail_unlock;
Since this involves clean-up in the form of unlocking the mutex this
goto is fine however.
Same for unregister_device:.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
