2008-12-26 05:23:21

by Magnus Damm

Subject: [PATCH] gpiolib: request fixes

From: Magnus Damm <[email protected]>

Fix request related issues in gpiolib such as:
- fix request-already-requested handling in gpio_request()
- clear FLAG_REQUESTED on request error in gpio_direction_input()
- clear FLAG_REQUESTED on request error in gpio_direction_output()

Signed-off-by: Magnus Damm <[email protected]>
---

drivers/gpio/gpiolib.c | 3 +++
1 file changed, 3 insertions(+)

--- 0001/drivers/gpio/gpiolib.c
+++ work/drivers/gpio/gpiolib.c 2008-12-26 13:09:50.000000000 +0900
@@ -789,6 +789,7 @@ int gpio_request(unsigned gpio, const ch
} else {
status = -EBUSY;
module_put(chip->owner);
+ goto done;
}

if (chip->request) {
@@ -924,6 +925,7 @@ int gpio_direction_input(unsigned gpio)
/* and it's not available to anyone else ...
* gpio_request() is the fully clean solution.
*/
+ clear_bit(FLAG_REQUESTED, &desc->flags);
goto lose;
}
}
@@ -977,6 +979,7 @@ int gpio_direction_output(unsigned gpio,
/* and it's not available to anyone else ...
* gpio_request() is the fully clean solution.
*/
+ clear_bit(FLAG_REQUESTED, &desc->flags);
goto lose;
}
}

2008-12-26 15:53:26

by David Brownell

Subject: Re: [PATCH] gpiolib: request fixes

On Thursday 25 December 2008, Magnus Damm wrote:
> From: Magnus Damm <[email protected]>
>
> Fix request related issues in gpiolib such as:
> - fix request-already-requested handling in gpio_request()
> - clear FLAG_REQUESTED on request error in gpio_direction_input()
> - clear FLAG_REQUESTED on request error in gpio_direction_output()
>
> Signed-off-by: Magnus Damm <[email protected]>
> ---
>
> drivers/gpio/gpiolib.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> --- 0001/drivers/gpio/gpiolib.c
> +++ work/drivers/gpio/gpiolib.c 2008-12-26 13:09:50.000000000 +0900
> @@ -789,6 +789,7 @@ int gpio_request(unsigned gpio, const ch
> } else {
> status = -EBUSY;
> module_put(chip->owner);
> + goto done;

Right, good catch.


> }
>
> if (chip->request) {
> @@ -924,6 +925,7 @@ int gpio_direction_input(unsigned gpio)
> /* and it's not available to anyone else ...
> * gpio_request() is the fully clean solution.
> */
> + clear_bit(FLAG_REQUESTED, &desc->flags);

NAK, this is insufficient ... it would need to drop the module
refcount and null the label too. Plus this invalidates the
comment. (Same below.)

However a basic premise is that drivers should now be avoiding
this legacy autorequest stuff, using gpio_request() instead.

Hence the comments here, below, and at ensure_requested() to
note the lack of cleanup if these legacy paths lose: small
incentives to "do the right thing". I'd rather see the work
go into making callers stop using autorequest; not making
that mechanism work better.

It may be time to make ensure_requested() use WARN(), which
will create a lot more noise than the current message ... a
larger incentive. :)


> goto lose;
> }
> }
> @@ -977,6 +979,7 @@ int gpio_direction_output(unsigned gpio,
> /* and it's not available to anyone else ...
> * gpio_request() is the fully clean solution.
> */
> + clear_bit(FLAG_REQUESTED, &desc->flags);
> goto lose;
> }
> }
>
>