gpiolib: Refactor gpio_export
PATCH v2 1/1 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.
---
Changes since v1:
- Return immediately if no unwinding needed
- Added pr_debug to immediate returns
- Changed unavailable (!requested || exported) to -EPERM
drivers/gpio/gpiolib.c +46 -39
@@ -702,8 +702,9 @@ 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) {
@@ -711,59 +712,65 @@ int gpio_export(unsigned gpio, bool direction_may_change)return -ENOENT ; } -if (!gpio_is_valid (gpio)) - goto done; +if (!gpio_is_valid (gpio)) { +pr_debug ("%s: gpio %d is not valid\n" , __func__, gpio); +return -EINVAL ; + }mutex_lock (&sysfs_lock);spin_lock_irqsave (&gpio_lock, flags); desc = &gpio_desc[gpio]; -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); +pr_debug ("%s: gpio %d unavailable (requested=%d, exported=%d)\n" , + __func__, gpio, +test_bit (FLAG_REQUESTED , &desc->flags), +test_bit (FLAG_EXPORT , &desc->flags)); +return -EPERM ; } + +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; } -mutex_unlock (&sysfs_lock); - -done: + status =sysfs_create_group (&dev->kobj, &gpio_attr_group);if (status) -pr_debug ("%s: gpio%d status %d\n" , __func__, gpio, status); + goto fail_unregister_device; + +if (direction_may_change) { + status =device_create_file (dev, &dev_attr_direction); +if (status) + goto fail_unregister_device; + } +if (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) + goto fail_unregister_device; + } + +set_bit (FLAG_EXPORT , &desc->flags); +mutex_unlock (&sysfs_lock); +return 0 ; + +fail_unregister_device: +device_unregister (dev); +fail_unlock: +mutex_unlock (&sysfs_lock); +pr_debug ("%s: gpio%d status %d\n" , __func__, gpio, status);return status; }EXPORT_SYMBOL_GPL (gpio_export);
Quoted 6 lines · Ryan Mallon
The
gpio_exportfunction 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.
Splendid, patch applied!
Quoted 1 line
Signed-off-by Ryan Mallon
Interesting that you sign off without an email address, I just added it thinking it was a mistake.
Yours,
Linus Walleij
Quoted 10 lines · Linus Walleij
Quoted 6 lines · Ryan Mallon
The
gpio_exportfunction 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.Splendid, patch applied!
Thanks.
Quoted 4 lines
Quoted 1 line
Signed-off-by Ryan MallonInteresting that you sign off without an email address, I just added it thinking it was a mistake.
Whoops. Yeah, should be:
Thanks,
~Ryan