* [RFC PATCH] gpiolib: Refactor gpio_export
@ 2012-10-16 23:41 Ryan Mallon
2012-10-19 10:07 ` Linus Walleij
0 siblings, 1 reply; 4+ messages in thread
From: Ryan Mallon @ 2012-10-16 23:41 UTC (permalink / raw)
To: Grant Likely, Linus Walleij, linux-kernel@vger.kernel.org
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
---
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5d6c71e..a142246 100644
--- a/drivers/gpio/gpiolib.c
+++ 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;
}
- if (!gpio_is_valid(gpio))
- goto done;
+ if (!gpio_is_valid(gpio)) {
+ status = -EINVAL;
+ goto fail;
+ }
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);
+ status = -ENOENT;
+ goto fail;
}
+
+ 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);
+fail:
+ pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
return status;
}
EXPORT_SYMBOL_GPL(gpio_export);
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [RFC PATCH] gpiolib: Refactor gpio_export
2012-10-16 23:41 [RFC PATCH] gpiolib: Refactor gpio_export Ryan Mallon
@ 2012-10-19 10:07 ` Linus Walleij
2012-10-19 22:14 ` Ryan Mallon
0 siblings, 1 reply; 4+ messages in thread
From: Linus Walleij @ 2012-10-19 10:07 UTC (permalink / raw)
To: Ryan Mallon; +Cc: Grant Likely, linux-kernel@vger.kernel.org
On Wed, Oct 17, 2012 at 1:41 AM, Ryan Mallon <rmallon@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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] gpiolib: Refactor gpio_export
2012-10-19 10:07 ` Linus Walleij
@ 2012-10-19 22:14 ` Ryan Mallon
2012-10-19 22:43 ` Grant Likely
0 siblings, 1 reply; 4+ messages in thread
From: Ryan Mallon @ 2012-10-19 22:14 UTC (permalink / raw)
To: Linus Walleij; +Cc: Grant Likely, linux-kernel@vger.kernel.org
On 19/10/12 21:07, Linus Walleij wrote:
> On Wed, Oct 17, 2012 at 1:41 AM, Ryan Mallon <rmallon@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.
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.
> Arguably this should be pr_err() or something BTW, just debug()
> may hide serious bugs.
Not sure about that. User-space can check the return code if it
wants to find out why the call failed. We shouldn't
unconditionally spam the log with error messages on sysfs
access since it makes an easy DoS vector on the syslog. The
pr_debug messages I think are really for people who are
developing on gpiolib.
~Ryan
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] gpiolib: Refactor gpio_export
2012-10-19 22:14 ` Ryan Mallon
@ 2012-10-19 22:43 ` Grant Likely
0 siblings, 0 replies; 4+ messages in thread
From: Grant Likely @ 2012-10-19 22:43 UTC (permalink / raw)
To: Ryan Mallon; +Cc: Linus Walleij, linux-kernel@vger.kernel.org
On Fri, Oct 19, 2012 at 11:14 PM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 19/10/12 21:07, Linus Walleij wrote:
>
>> On Wed, Oct 17, 2012 at 1:41 AM, Ryan Mallon <rmallon@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.
>
> 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.
Single exit points are overrated. The code is shorter and easier to
read to return immediately if there isn't any unwinding to do.
g.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-10-19 22:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-16 23:41 [RFC PATCH] gpiolib: Refactor gpio_export Ryan Mallon
2012-10-19 10:07 ` Linus Walleij
2012-10-19 22:14 ` Ryan Mallon
2012-10-19 22:43 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox