Hi,
Currently implementors of gpiolib must provide implementations for
gpio_get_value, gpio_set_value and gpio_cansleep. Most of the
implementations just #define these to the double underscore prefixed
versions in drivers/gpio/gpiolib.c. A few implementations have a simple
wrapper function which provides a fast path for the SoC gpios, and calls
gpiolib for the any additional gpios, such as those added by an io expander.
Although gpio_chips know whether or not they may sleep, gpios which can
sleep need to call gpio_[set/get]_value_cansleep. The only difference
between __gpio_(set/get)_value and gpio_(set/get)_value_cansleep is that
the cansleep versions calls might_sleep_if. Most drivers call
gpio_(get/set)_value, rather than the cansleep variants. I haven't done
a full audit of all of the drivers (which is a reasonably involved
task), but I would hazard a guess that some of these could be replaced
by the cansleep versions.
Would it not be simpler to combine the calls and have something like this:
void __gpio_get_value(unsigned gpio, int value)
{
struct gpio_chip *chip;
chip = gpio_to_chip(gpio);
might_sleep_if(extra_checks && chip->can_sleep);
chip->set(chip, gpio - chip->base, value);
}
Then all drivers can just call gpio_(set/get)_value and any attempts to
use sleeping gpios from an non-sleeping context will be caught by the
might_sleep_if check. Is there something I am missing about this?
I can prepare a patch which combines the non-sleeping and sleeping
variants, but I wanted to check that I'm not missing something
fundamental first.
Thanks,
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
Hi Ryan,
On Fri, Jun 18, 2010 at 09:47:59AM +1200, Ryan Mallon wrote:
> Then all drivers can just call gpio_(set/get)_value and any attempts to
> use sleeping gpios from an non-sleeping context will be caught by the
> might_sleep_if check. Is there something I am missing about this?
The downside is that you change the semantic of gpio_get_value (and
gpio_set_value I assume?). But as calling gpio_get_value with a gpio
that gpio_cansleep() is an error anyhow, so I think that's OK. The big
pro is that the API is simplified.
> I can prepare a patch which combines the non-sleeping and sleeping
> variants, but I wanted to check that I'm not missing something
> fundamental first.
I will happily look at such a patch and give my comments.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--- On Thu, 6/17/10, Ryan Mallon <[email protected]> wrote:
> Currently implementors of gpiolib must provide
> implementations for
> gpio_get_value, gpio_set_value and gpio_cansleep.
Not true. There is ONE implementation of gpiolib.
I think you mean "implementors of the GPIO calls".
many of those implementors will use gpiolib, to
make their lives easier. They are not, however,
re-implementing gpiolib itself...
Most of
> the
> implementations just #define these to the double underscore
> prefixed
> versions in drivers/gpio/gpiolib.c. A few implementations
> have a simple
> wrapper function which provides a fast path for the SoC
> gpios, and calls
> gpiolib for the any additional gpios, such as those added
> by an io expander.
Right...
> Although gpio_chips know whether or not they may sleep,
> gpios which can
> sleep need to call gpio_[set/get]_value_cansleep.
GPIOsare hardware, or maybe software abstractions;
either way, can't call those.
Driver level code does
when it accesses a GPIO.
for example,peripheral setup logic might.
The only
> difference
> between __gpio_(set/get)_value and
> gpio_(set/get)_value_cansleep is that
only the cansleep() versions may be used for
GPIOs whose operation requires use of sleeping
calls. Most SoC GPIOs are safe to access with
IRQs disabled, spinlocks, held and so on.
That's why only the non-cansleep() versions
are documented as being spinlock-safe.
> the cansleep versions calls might_sleep_if. Most drivers
That's an implemenation detail, internal to the
gpiolib code.
Note that "can" sleep means exactly that... It
doesn't mean "must sleep". A valid way to
implement a cansleep() variant is to
just call the spinlock-safe routine, so that
it never sleeps.
THe converse is not true; the spinlock-safe
routine must never call a cansleep() version.
> Most drivers call > gpio_(get/set)_value, rather than the cansleep variants. I
> haven't done
> a full audit of all of the drivers (which is a reasonably
> involved
> task), but I would hazard a guess that some of these could
> be replaced
> by the cansleep versions.
One hopes that the runtime warnings have gotten
folk to fix bugs where the cansleep() version
should have been used, but wasn't...
Better IMO not to make that change except as
part
of a bugfix: e.g. remove a runtime warning
that boils down to not using the cansleep()
version when it should have been used.
>
> Would it not be simpler to combine the calls and have
> something like this:
>
> void __gpio_get_value(unsigned gpio, int value)
> {
> ??? struct gpio_chip *chip;
>
> ??? chip = gpio_to_chip(gpio);
> ??? might_sleep_if(extra_checks &&
> chip->can_sleep);
> ??? chip->set(chip, gpio - chip->base,
> value);
calling chip->set() when chip->can_sleep
and the call context can't be preempted,
would be a bug. So that code is wrong ...
it should at least warn about the error
made by the caller. If we had error returns from gpio get/set calls 9sigh), it'd be right to
fail that call.
> }
>
> Then all drivers can just call gpio_(set/get)_value
Bad idea. Those calls are only for
use in non-sleeping contexts; don't
train developers to misuse calls.
and any
> attempts to
> use sleeping gpios from an non-sleeping context will be
> caught by the
> might_sleep_if check. Is there something I am missing about
> this?
The fact that such attempts are errors
in the calling code, and should not be
swept under therug...
>
> I can prepare a patch which combines the non-sleeping and
> sleeping
> variants, but I wanted to check that I'm not missing
> something
> fundamental first.
>
> Thanks,
> ~Ryan
>
> --
> Bluewater Systems Ltd - ARM Technology Solution Centre
>
> Ryan Mallon? ? ?
> ?????? ??? 5
> Amuri Park, 404 Barbadoes St
> [email protected]?
> ? ? ?????? PO Box 13
> 889, Christchurch 8013
> http://www.bluewatersys.com??? New
> Zealand
> Phone: +64 3 3779127??? ???
> Freecall: Australia 1800 148 751
> Fax:???+64 3 3779135???
> ??? ??? ? USA 1800 261
> 2934
>
David Brownell wrote:
>
> --- On Thu, 6/17/10, Ryan Mallon <[email protected]> wrote:
>> Currently implementors of gpiolib must provide
>> implementations for
>> gpio_get_value, gpio_set_value and gpio_cansleep.
>
> Not true. There is ONE implementation of gpiolib.
>
> I think you mean "implementors of the GPIO calls".
> many of those implementors will use gpiolib, to
> make their lives easier. They are not, however,
> re-implementing gpiolib itself...
Okay, so I got the semantics wrong. You know what I mean :-).
<snip>
>
> only the cansleep() versions may be used for
> GPIOs whose operation requires use of sleeping
> calls. Most SoC GPIOs are safe to access with
> IRQs disabled, spinlocks, held and so on.
>
> That's why only the non-cansleep() versions
> are documented as being spinlock-safe.
>
>
>> the cansleep versions calls might_sleep_if. Most drivers
>
>
> That's an implemenation detail, internal to the
> gpiolib code.
>
> Note that "can" sleep means exactly that... It
> doesn't mean "must sleep". A valid way to
> implement a cansleep() variant is to
>
> just call the spinlock-safe routine, so that
> it never sleeps.
>
> THe converse is not true; the spinlock-safe
> routine must never call a cansleep() version.
Right, so it is just an implementation detail. When can easily change
that to gpio_(set/get)_value is only safe to call from spinlock context
if the gpio will not sleep. Having the the might_sleep_if check inside
those calls will give a warning if that condition is not met.
>
>> Most drivers call > gpio_(get/set)_value, rather than the cansleep variants. I
>> haven't done
>> a full audit of all of the drivers (which is a reasonably
>> involved
>> task), but I would hazard a guess that some of these could
>> be replaced
>> by the cansleep versions.
>
>
> One hopes that the runtime warnings have gotten
> folk to fix bugs where the cansleep() version
> should have been used, but wasn't...
The runtime warnings will only show instances where the non-sleeping
versions where called instead of the sleeping versions. There is no
warning to say that we are calling the spinlock safe version, where it
is possible to sleep.
The point I was trying to make is that there are lots of drivers which
will not work with gpios on sleeping io expanders because they call the
spinlock safe gpio calls. Some of these drivers may be able to use the
sleeping variants. There are two possible ways to fix this:
1) Audit each driver which uses non-sleeping gpio calls and determine
whether they can use the sleeping versions instead.
2) Modify the gpiolib calls as I suggested. Any attempts to use a
sleeping gpio with a driver which cannot support it will result in a
warning.
>
> Better IMO not to make that change except as
> part
> of a bugfix: e.g. remove a runtime warning
> that boils down to not using the cansleep()
> version when it should have been used.
>> Would it not be simpler to combine the calls and have
>> something like this:
>>
>> void __gpio_get_value(unsigned gpio, int value)
>> {
>> struct gpio_chip *chip;
>>
>> chip = gpio_to_chip(gpio);
>> might_sleep_if(extra_checks &&
>> chip->can_sleep);
>> chip->set(chip, gpio - chip->base,
>
>> value);
>
> calling chip->set() when chip->can_sleep
> and the call context can't be preempted,
> would be a bug. So that code is wrong ...
> it should at least warn about the error
> made by the caller. If we had error returns from gpio get/set calls 9sigh), it'd be right to
> fail that call.
Not quite sure what you mean here? Many drivers are generic in that they
get passed a gpio to use with the gpiolib calls via some platform/driver
data. Some drivers will be able to use sleeping gpios, but may not be
correctly using the cansleep variants.
I agree about the errors from the gpio_(set/get)_value calls. set isn't
too bad, but returning an error and a value from the get calls is a bit
of a pain.
Possibly a solution is to have an additional argument to gpio_request
which specifies whether the driver this gpio is able to be a sleeping
gpio or not. That way the request can fail immediately if a sleeping
gpio is passed to a driver that calls gpiolibs calls from sleeping context.
~Ryan
>
> The runtime warnings will only show instances where the
> non-sleeping
> versions where called instead of the sleeping versions.
... *AND* the GPIO requires the cansleep() version...
Right; such calls are errors. We issue
warnings since fault returns are inapplicable.
> There is no
> warning to say that we are calling the spinlock safe
> version, where it is possible to sleep.
The call context isn't what controls whether
gpio_get_value() or gpio_get_value_cansleep()
is appropriate ... it's the GPIO itself, and
how its implementation works.
"possible to sleep" is a GPIO attribute,
exposed by a predicate. If spinlock-safe
calls are used on GPIOs with that attribute,
a warning *IS* issued.
>
> The point I was trying to make is that there are lots of
> drivers which
> will not work with gpios on sleeping io expanders because
> they call the
> spinlock safe gpio calls.
And they will trigger runtime warnings, and
thus eventually get fixed. The way to do that
is to check if the GPIO needs the cansleep()
call
That's the first category above: the driver
should have used the cansleep() variant, and
sotriggers a runtime warning.
On 06/19/2010 06:21 PM, David Brownell wrote:
>
>>
>> The runtime warnings will only show instances where the
>> non-sleeping
>> versions where called instead of the sleeping versions.
>
> ... *AND* the GPIO requires the cansleep() version...
>
> Right; such calls are errors. We issue
> warnings since fault returns are inapplicable.
A driver which only uses the non-sleeping versions, but _could_ use the
cansleep variants (ie all calls to gpio_(set/get)_value are made from
contexts where it is possible to sleep) is not so easy to spot. Passing
a sleeping to gpio to such a driver will result in spurious warnings.
>> There is no
>> warning to say that we are calling the spinlock safe
>> version, where it is possible to sleep.
>
> The call context isn't what controls whether
> gpio_get_value() or gpio_get_value_cansleep()
> is appropriate ... it's the GPIO itself, and
> how its implementation works.
No, a driver should not know anything about a gpio which is passed to
it. If a driver is able to call the cansleep variants, then it should,
and it will allow any gpio, sleeping or non-sleeping, to be used with
that driver.
If a driver uses a gpio in such a way that it cannot sleep, ie the
gpio_(get/set)_value calls are made from spinlock context, then only
gpios which do not sleep may be used with that driver.
Thats why I think specifying whether the gpio is able to sleep when it
is requested is a good idea. A driver which cannot use a sleeping gpio
> "possible to sleep" is a GPIO attribute,
> exposed by a predicate. If spinlock-safe
> calls are used on GPIOs with that attribute,
> a warning *IS* issued.
Possible to sleep is also an attribute of how a driver _uses_ a gpio.
>>
>> The point I was trying to make is that there are lots of
>> drivers which
>> will not work with gpios on sleeping io expanders because
>> they call the
>> spinlock safe gpio calls.
>
> And they will trigger runtime warnings, and
> thus eventually get fixed. The way to do that
> is to check if the GPIO needs the cansleep()
> call
Hmm, maybe this then for drivers which cannot accept sleeping gpios:
if (gpio_cansleep(some_gpio)) {
dev_err(&dev, "This driver only supports non-sleeping gpios");
return -EINVAL;
}
err = gpio_request(some_gpio, "some_gpio");
I think ideally, gpio_request should specify this via a flags argument, ie:
#define GPIOF_NO_SLEEP 0x0
#define GPIOF_CANSLEEP 0x1
err = gpio_request(some_gpio, "some_gpio", GPIOF_NO_SLEEP);
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
--- On Sun, 6/20/10, Ryan Mallon <[email protected]> wrote:
> >> The point I was trying to make is that
> >> there are lots of drivers which
> >> will not work with gpios on sleeping io expandersbecause they call the
> >> spinlock safe gpio calls.
"Lots"? You mean there are lots of
maintainers who aren't even bothering to
provide trivial fixes for bug which are
clearly reported to them by warnings?
These one-liner fixes are not hard...
Such problems are people-problems, not issues
with any framework.
> >
> > And they will trigger runtime warnings, and
> > thus eventually get fixed.
> \
> ? }
>
> ? err = gpio_request(some_gpio, "some_gpio",
> GPIOF_NO_SLEEP);
NAK ... keep it simple. Such flags are
clearly not necessary...
I understand that some folk are bothered
by concepts/frameworks that seem "too simple"
and thus want to complexify them. In this
case I am in a position to help avoid that.
Complexity is not a virtue.
Hi,
On Sun, Jun 20, 2010 at 07:40:46PM -0700, David Brownell wrote:
> > > And they will trigger runtime warnings, and
> > > thus eventually get fixed.
> > \
> > ? }
> >
> > ? err = gpio_request(some_gpio, "some_gpio",
> > GPIOF_NO_SLEEP);
>
>
> NAK ... keep it simple. Such flags are
> clearly not necessary...
>
> I understand that some folk are bothered
> by concepts/frameworks that seem "too simple"
> and thus want to complexify them. In this
> case I am in a position to help avoid that.
> Complexity is not a virtue.
I'm against such an additional flag, too. But I still think merging
gpio_get_value and gpio_get_value_cansleep is nice.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 06/21/2010 05:09 PM, Uwe Kleine-K?nig wrote:
> Hi,
>
> On Sun, Jun 20, 2010 at 07:40:46PM -0700, David Brownell wrote:
>
>>>> And they will trigger runtime warnings, and
>>>> thus eventually get fixed.
>>>>
>>> \
>>> }
>>>
>>> err = gpio_request(some_gpio, "some_gpio",
>>> GPIOF_NO_SLEEP);
>>>
>>
>> NAK ... keep it simple. Such flags are
>> clearly not necessary...
>>
>> I understand that some folk are bothered
>> by concepts/frameworks that seem "too simple"
>> and thus want to complexify them. In this
>> case I am in a position to help avoid that.
>> Complexity is not a virtue.
>>
> I'm against such an additional flag, too. But I still think merging
> gpio_get_value and gpio_get_value_cansleep is nice.
>
> Best regards
> Uwe
>
>
Okay, here is a rough patch to demonstrate how I think the gpiolib
framework could be simplified for cansleep gpios.
'Can sleep' for a gpio has two different meanings depending on context:
1) When talking about a gpio chip, 'can sleep' refers to whether or not
the set/get functions for the gpio may sleep. For example, io expanders
on the i2c or spi bus may sleep.
2) When talking about a drivers use of a gpio, 'can sleep' refers to
whether or not the the driver ever calls gpio_(set/get)_value for a
particular gpio in a context where it is not possible to sleep. For
example, if a driver calls gpio_get_value(gpio) from an interupt handler
then the gpio must not be a sleeping gpio.
This patch introduces a new flag, FLAG_CANSLEEP, internal to gpiolib
which denotes that a requested gpio may be used in a sleeping context. A
new request function, gpio_request_cansleep, requests a gpio which may
only be used from sleep possible contexts (ie cannot be used from
interrupt handlers, inside spinlocks, etc). The existing gpio_request
function requests a gpio, but does not allow it to be used from a
context where sleep is not possible. If a sleeping gpio is passed to
gpio_request then -EINVAL is returned.
The gpio_(set/get)_value_cansleep functions are removed, and all callers
now use gpio_(set/get)_value. might_sleep_if is used to warn about
incorrect uses of sleeping gpios.
The benefits I see to this approach are:
- Using gpio_request/gpio_request_cansleep means that passing a
sleeping gpio to a driver that calls gpio_(set/get)_value from non-sleep
safe context is caught at request time and handled as an error.
- The API is simplified by combining gpio_(set/get)_value and
gpio_(set/get)_value_cansleep
~Ryan
---
arch/arm/mach-davinci/board-dm355-evm.c | 12 ++--
arch/arm/mach-davinci/board-dm355-leopard.c | 12 ++--
arch/arm/mach-davinci/board-dm644x-evm.c | 4 +-
drivers/gpio/gpiolib.c | 67 ++++++++++++++-------------
drivers/input/keyboard/matrix_keypad.c | 10 ++--
drivers/leds/leds-gpio.c | 7 ++-
drivers/leds/leds-lt3593.c | 14 +++---
drivers/mmc/host/omap_hsmmc.c | 11 ++--
drivers/regulator/fixed.c | 6 +-
drivers/usb/musb/davinci.c | 4 +-
drivers/watchdog/wm831x_wdt.c | 4 +-
include/asm-generic/gpio.h | 16 ------
include/linux/gpio.h | 18 ++-----
sound/soc/s3c24xx/s3c24xx_simtec.c | 8 ++--
14 files changed, 88 insertions(+), 105 deletions(-)
diff --git a/arch/arm/mach-davinci/board-dm355-evm.c b/arch/arm/mach-davinci/board-dm355-evm.c
index a319101..853fd12 100644
--- a/arch/arm/mach-davinci/board-dm355-evm.c
+++ b/arch/arm/mach-davinci/board-dm355-evm.c
@@ -118,10 +118,10 @@ static int dm355evm_mmc_gpios = -EINVAL;
static void dm355evm_mmcsd_gpios(unsigned gpio)
{
- gpio_request(gpio + 0, "mmc0_ro");
- gpio_request(gpio + 1, "mmc0_cd");
- gpio_request(gpio + 2, "mmc1_ro");
- gpio_request(gpio + 3, "mmc1_cd");
+ gpio_request_cansleep(gpio + 0, "mmc0_ro");
+ gpio_request_cansleep(gpio + 1, "mmc0_cd");
+ gpio_request_cansleep(gpio + 2, "mmc1_ro");
+ gpio_request_cansleep(gpio + 3, "mmc1_cd");
/* we "know" these are input-only so we don't
* need to call gpio_direction_input()
@@ -262,7 +262,7 @@ static int dm355evm_mmc_get_cd(int module)
if (!gpio_is_valid(dm355evm_mmc_gpios))
return -ENXIO;
/* low == card present */
- return !gpio_get_value_cansleep(dm355evm_mmc_gpios + 2 * module + 1);
+ return !gpio_get_value(dm355evm_mmc_gpios + 2 * module + 1);
}
static int dm355evm_mmc_get_ro(int module)
@@ -270,7 +270,7 @@ static int dm355evm_mmc_get_ro(int module)
if (!gpio_is_valid(dm355evm_mmc_gpios))
return -ENXIO;
/* high == card's write protect switch active */
- return gpio_get_value_cansleep(dm355evm_mmc_gpios + 2 * module + 0);
+ return gpio_get_value(dm355evm_mmc_gpios + 2 * module + 0);
}
static struct davinci_mmc_config dm355evm_mmc_config = {
diff --git a/arch/arm/mach-davinci/board-dm355-leopard.c b/arch/arm/mach-davinci/board-dm355-leopard.c
index f1d8132..99a9a9c 100644
--- a/arch/arm/mach-davinci/board-dm355-leopard.c
+++ b/arch/arm/mach-davinci/board-dm355-leopard.c
@@ -110,10 +110,10 @@ static int leopard_mmc_gpio = -EINVAL;
static void dm355leopard_mmcsd_gpios(unsigned gpio)
{
- gpio_request(gpio + 0, "mmc0_ro");
- gpio_request(gpio + 1, "mmc0_cd");
- gpio_request(gpio + 2, "mmc1_ro");
- gpio_request(gpio + 3, "mmc1_cd");
+ gpio_request_cansleep(gpio + 0, "mmc0_ro");
+ gpio_request_cansleep(gpio + 1, "mmc0_cd");
+ gpio_request_cansleep(gpio + 2, "mmc1_ro");
+ gpio_request_cansleep(gpio + 3, "mmc1_cd");
/* we "know" these are input-only so we don't
* need to call gpio_direction_input()
@@ -185,7 +185,7 @@ static int dm355leopard_mmc_get_cd(int module)
if (!gpio_is_valid(leopard_mmc_gpio))
return -ENXIO;
/* low == card present */
- return !gpio_get_value_cansleep(leopard_mmc_gpio + 2 * module + 1);
+ return !gpio_get_value(leopard_mmc_gpio + 2 * module + 1);
}
static int dm355leopard_mmc_get_ro(int module)
@@ -193,7 +193,7 @@ static int dm355leopard_mmc_get_ro(int module)
if (!gpio_is_valid(leopard_mmc_gpio))
return -ENXIO;
/* high == card's write protect switch active */
- return gpio_get_value_cansleep(leopard_mmc_gpio + 2 * module + 0);
+ return gpio_get_value(leopard_mmc_gpio + 2 * module + 0);
}
static struct davinci_mmc_config dm355leopard_mmc_config = {
diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c
index 34c8b41..5cf037c 100644
--- a/arch/arm/mach-davinci/board-dm644x-evm.c
+++ b/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -335,7 +335,7 @@ static int sw_gpio;
static ssize_t
sw_show(struct device *d, struct device_attribute *a, char *buf)
{
- char *s = gpio_get_value_cansleep(sw_gpio) ? "on\n" : "off\n";
+ char *s = gpio_get_value(sw_gpio) ? "on\n" : "off\n";
strcpy(buf, s);
return strlen(s);
@@ -350,7 +350,7 @@ evm_u18_setup(struct i2c_client *client, int gpio, unsigned ngpio, void *c)
/* export dip switch option */
sw_gpio = gpio + 7;
- status = gpio_request(sw_gpio, "user_sw");
+ status = gpio_request_cansleep(sw_gpio, "user_sw");
if (status == 0)
status = gpio_direction_input(sw_gpio);
if (status == 0)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3ca3654..6090ddc 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -55,6 +55,7 @@ struct gpio_desc {
#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
#define FLAG_ACTIVE_LOW 7 /* sysfs value has active low */
+#define FLAG_CANSLEEP 8 /* gpio may sleep during get/set */
#define PDESC_ID_SHIFT 16 /* add new flags before this one */
@@ -279,7 +280,7 @@ static ssize_t gpio_value_show(struct device *dev,
} else {
int value;
- value = !!gpio_get_value_cansleep(gpio);
+ value = !!__gpio_get_value(gpio);
if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
value = !value;
@@ -310,7 +311,7 @@ static ssize_t gpio_value_store(struct device *dev,
if (status == 0) {
if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
value = !value;
- gpio_set_value_cansleep(gpio, value != 0);
+ __gpio_set_value(gpio, value != 0);
status = size;
}
}
@@ -773,8 +774,10 @@ int gpio_export(unsigned gpio, bool direction_may_change)
device_unregister(dev);
} else
status = PTR_ERR(dev);
- if (status == 0)
+ if (status == 0) {
set_bit(FLAG_EXPORT, &desc->flags);
+ set_bit(FLAG_CANSLEEP, &desc->flags);
+ }
}
mutex_unlock(&sysfs_lock);
@@ -1152,7 +1155,7 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
* on each other, and help provide better diagnostics in debugfs.
* They're called even less than the "set direction" calls.
*/
-int gpio_request(unsigned gpio, const char *label)
+static int __gpio_request(unsigned gpio, const char *label, int cansleep)
{
struct gpio_desc *desc;
struct gpio_chip *chip;
@@ -1171,6 +1174,9 @@ int gpio_request(unsigned gpio, const char *label)
if (!try_module_get(chip->owner))
goto done;
+ if (!cansleep && chip->cansleep)
+ goto done;
+
/* NOTE: gpio_request() can be called in early boot,
* before IRQs are enabled, for non-sleeping (SOC) GPIOs.
*/
@@ -1184,6 +1190,9 @@ int gpio_request(unsigned gpio, const char *label)
goto done;
}
+ if (cansleep)
+ set_bit(FLAG_CANSLEEP, &desc->flags);
+
if (chip->request) {
/* chip->request may sleep */
spin_unlock_irqrestore(&gpio_lock, flags);
@@ -1204,8 +1213,19 @@ done:
spin_unlock_irqrestore(&gpio_lock, flags);
return status;
}
+
+int gpio_request(unsigned gpio, const char *label)
+{
+ return __gpio_request(gpio, label, 0);
+}
EXPORT_SYMBOL_GPL(gpio_request);
+int gpio_request_cansleep(unsigned gpio, const char *label)
+{
+ return __gpio_request(gpio, label, 1);
+}
+EXPORT_SYMBOL_GPL(gpio_request_cansleep);
+
void gpio_free(unsigned gpio)
{
unsigned long flags;
@@ -1525,9 +1545,13 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce);
int __gpio_get_value(unsigned gpio)
{
struct gpio_chip *chip;
+ struct gpio_desc *desc;
chip = gpio_to_chip(gpio);
- WARN_ON(extra_checks && chip->can_sleep);
+ desc = &gpio_desc[gpio];
+
+ might_sleep_if(extra_checks && (chip->cansleep ||
+ test_bit(FLAG_CANSLEEP, &desc->flags));
return chip->get ? chip->get(chip, gpio - chip->base) : 0;
}
EXPORT_SYMBOL_GPL(__gpio_get_value);
@@ -1544,9 +1568,13 @@ EXPORT_SYMBOL_GPL(__gpio_get_value);
void __gpio_set_value(unsigned gpio, int value)
{
struct gpio_chip *chip;
+ struct gpio_desc *desc;
chip = gpio_to_chip(gpio);
- WARN_ON(extra_checks && chip->can_sleep);
+ desc = &gpio_desc[gpio];
+
+ might_sleep_if(extra_checks && (chip->cansleep ||
+ test_bit(FLAG_CANSLEEP, &desc->flags));
chip->set(chip, gpio - chip->base, value);
}
EXPORT_SYMBOL_GPL(__gpio_set_value);
@@ -1588,33 +1616,6 @@ int __gpio_to_irq(unsigned gpio)
}
EXPORT_SYMBOL_GPL(__gpio_to_irq);
-
-
-/* There's no value in making it easy to inline GPIO calls that may sleep.
- * Common examples include ones connected to I2C or SPI chips.
- */
-
-int gpio_get_value_cansleep(unsigned gpio)
-{
- struct gpio_chip *chip;
-
- might_sleep_if(extra_checks);
- chip = gpio_to_chip(gpio);
- return chip->get ? chip->get(chip, gpio - chip->base) : 0;
-}
-EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
-
-void gpio_set_value_cansleep(unsigned gpio, int value)
-{
- struct gpio_chip *chip;
-
- might_sleep_if(extra_checks);
- chip = gpio_to_chip(gpio);
- chip->set(chip, gpio - chip->base, value);
-}
-EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
-
-
#ifdef CONFIG_DEBUG_FS
static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index b443e08..e0b3884 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -52,7 +52,7 @@ static void __activate_col(const struct matrix_keypad_platform_data *pdata,
if (on) {
gpio_direction_output(pdata->col_gpios[col], level_on);
} else {
- gpio_set_value_cansleep(pdata->col_gpios[col], !level_on);
+ gpio_set_value(pdata->col_gpios[col], !level_on);
gpio_direction_input(pdata->col_gpios[col]);
}
}
@@ -78,7 +78,7 @@ static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
int row)
{
- return gpio_get_value_cansleep(pdata->row_gpios[row]) ?
+ return gpio_get_value(pdata->row_gpios[row]) ?
!pdata->active_low : pdata->active_low;
}
@@ -273,7 +273,8 @@ static int __devinit init_matrix_gpio(struct platform_device *pdev,
/* initialized strobe lines as outputs, activated */
for (i = 0; i < pdata->num_col_gpios; i++) {
- err = gpio_request(pdata->col_gpios[i], "matrix_kbd_col");
+ err = gpio_request_cansleep(pdata->col_gpios[i],
+ "matrix_kbd_col");
if (err) {
dev_err(&pdev->dev,
"failed to request GPIO%d for COL%d\n",
@@ -285,7 +286,8 @@ static int __devinit init_matrix_gpio(struct platform_device *pdev,
}
for (i = 0; i < pdata->num_row_gpios; i++) {
- err = gpio_request(pdata->row_gpios[i], "matrix_kbd_row");
+ err = gpio_request_cansleep(pdata->row_gpios[i],
+ "matrix_kbd_row");
if (err) {
dev_err(&pdev->dev,
"failed to request GPIO%d for ROW%d\n",
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index cc22eee..89a8278 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -42,7 +42,7 @@ static void gpio_led_work(struct work_struct *work)
NULL, NULL);
led_dat->blinking = 0;
} else
- gpio_set_value_cansleep(led_dat->gpio, led_dat->new_level);
+ gpio_set_value(led_dat->gpio, led_dat->new_level);
}
static void gpio_led_set(struct led_classdev *led_cdev,
@@ -103,7 +103,10 @@ static int __devinit create_gpio_led(const struct gpio_led *template,
return 0;
}
- ret = gpio_request(template->gpio, template->name);
+ if (gpio_cansleep(template->gpio))
+ ret = gpio_request_cansleep(template->gpio, template->name);
+ else
+ ret = gpio_request(template->gpio);
if (ret < 0)
return ret;
diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index 2579678..605c6ca 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -48,25 +48,25 @@ static void lt3593_led_work(struct work_struct *work)
*/
if (led_dat->new_level == 0) {
- gpio_set_value_cansleep(led_dat->gpio, 0);
+ gpio_set_value(led_dat->gpio, 0);
return;
}
pulses = 32 - (led_dat->new_level * 32) / 255;
if (pulses == 0) {
- gpio_set_value_cansleep(led_dat->gpio, 0);
+ gpio_set_value(led_dat->gpio, 0);
mdelay(1);
- gpio_set_value_cansleep(led_dat->gpio, 1);
+ gpio_set_value(led_dat->gpio, 1);
return;
}
- gpio_set_value_cansleep(led_dat->gpio, 1);
+ gpio_set_value(led_dat->gpio, 1);
while (pulses--) {
- gpio_set_value_cansleep(led_dat->gpio, 0);
+ gpio_set_value(led_dat->gpio, 0);
udelay(1);
- gpio_set_value_cansleep(led_dat->gpio, 1);
+ gpio_set_value(led_dat->gpio, 1);
udelay(1);
}
}
@@ -93,7 +93,7 @@ static int __devinit create_lt3593_led(const struct gpio_led *template,
return 0;
}
- ret = gpio_request(template->gpio, template->name);
+ ret = gpio_request_cansleep(template->gpio, template->name);
if (ret < 0)
return ret;
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index b032828..0b362a6 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -191,7 +191,7 @@ static int omap_hsmmc_card_detect(struct device *dev, int slot)
struct omap_mmc_platform_data *mmc = dev->platform_data;
/* NOTE: assumes card detect signal is active-low */
- return !gpio_get_value_cansleep(mmc->slots[0].switch_pin);
+ return !gpio_get_value(mmc->slots[0].switch_pin);
}
static int omap_hsmmc_get_wp(struct device *dev, int slot)
@@ -199,7 +199,7 @@ static int omap_hsmmc_get_wp(struct device *dev, int slot)
struct omap_mmc_platform_data *mmc = dev->platform_data;
/* NOTE: assumes write protect signal is active-high */
- return gpio_get_value_cansleep(mmc->slots[0].gpio_wp);
+ return gpio_get_value(mmc->slots[0].gpio_wp);
}
static int omap_hsmmc_get_cover_state(struct device *dev, int slot)
@@ -207,7 +207,7 @@ static int omap_hsmmc_get_cover_state(struct device *dev, int slot)
struct omap_mmc_platform_data *mmc = dev->platform_data;
/* NOTE: assumes card detect signal is active-low */
- return !gpio_get_value_cansleep(mmc->slots[0].switch_pin);
+ return !gpio_get_value(mmc->slots[0].switch_pin);
}
#ifdef CONFIG_PM
@@ -473,7 +473,8 @@ static int omap_hsmmc_gpio_init(struct omap_mmc_platform_data *pdata)
pdata->slots[0].card_detect = omap_hsmmc_card_detect;
pdata->slots[0].card_detect_irq =
gpio_to_irq(pdata->slots[0].switch_pin);
- ret = gpio_request(pdata->slots[0].switch_pin, "mmc_cd");
+ ret = gpio_request_cansleep(pdata->slots[0].switch_pin,
+ "mmc_cd");
if (ret)
return ret;
ret = gpio_direction_input(pdata->slots[0].switch_pin);
@@ -484,7 +485,7 @@ static int omap_hsmmc_gpio_init(struct omap_mmc_platform_data *pdata)
if (gpio_is_valid(pdata->slots[0].gpio_wp)) {
pdata->slots[0].get_ro = omap_hsmmc_get_wp;
- ret = gpio_request(pdata->slots[0].gpio_wp, "mmc_wp");
+ ret = gpio_request_cansleep(pdata->slots[0].gpio_wp, "mmc_wp");
if (ret)
goto err_free_cd;
ret = gpio_direction_input(pdata->slots[0].gpio_wp);
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 2fe9d99..1e9392d 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -49,7 +49,7 @@ static int fixed_voltage_enable(struct regulator_dev *dev)
struct fixed_voltage_data *data = rdev_get_drvdata(dev);
if (gpio_is_valid(data->gpio)) {
- gpio_set_value_cansleep(data->gpio, data->enable_high);
+ gpio_set_value(data->gpio, data->enable_high);
data->is_enabled = true;
}
@@ -61,7 +61,7 @@ static int fixed_voltage_disable(struct regulator_dev *dev)
struct fixed_voltage_data *data = rdev_get_drvdata(dev);
if (gpio_is_valid(data->gpio)) {
- gpio_set_value_cansleep(data->gpio, !data->enable_high);
+ gpio_set_value(data->gpio, !data->enable_high);
data->is_enabled = false;
}
@@ -148,7 +148,7 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
dev_warn(&pdev->dev,
"using GPIO 0 for regulator enable control\n");
- ret = gpio_request(config->gpio, config->supply_name);
+ ret = gpio_request_cansleep(config->gpio, config->supply_name);
if (ret) {
dev_err(&pdev->dev,
"Could not obtain regulator enable GPIO %d: %d\n",
diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
index 5762436..48d72d4 100644
--- a/drivers/usb/musb/davinci.c
+++ b/drivers/usb/musb/davinci.c
@@ -161,7 +161,7 @@ static int vbus_state = -1;
*/
static void evm_deferred_drvvbus(struct work_struct *ignored)
{
- gpio_set_value_cansleep(GPIO_nVBUS_DRV, vbus_state);
+ gpio_set_value(GPIO_nVBUS_DRV, vbus_state);
vbus_state = !vbus_state;
}
@@ -181,7 +181,7 @@ static void davinci_source_power(struct musb *musb, int is_on, int immediate)
static DECLARE_WORK(evm_vbus_work, evm_deferred_drvvbus);
if (immediate)
- gpio_set_value_cansleep(GPIO_nVBUS_DRV, vbus_state);
+ gpio_set_value(GPIO_nVBUS_DRV, vbus_state);
else
schedule_work(&evm_vbus_work);
}
diff --git a/drivers/watchdog/wm831x_wdt.c b/drivers/watchdog/wm831x_wdt.c
index 8c4b2d5..1f37574 100644
--- a/drivers/watchdog/wm831x_wdt.c
+++ b/drivers/watchdog/wm831x_wdt.c
@@ -123,7 +123,7 @@ static int wm831x_wdt_kick(struct wm831x *wm831x)
mutex_lock(&wdt_mutex);
if (update_gpio) {
- gpio_set_value_cansleep(update_gpio, update_state);
+ gpio_set_value(update_gpio, update_state);
update_state = !update_state;
ret = 0;
goto out;
@@ -350,7 +350,7 @@ static int __devinit wm831x_wdt_probe(struct platform_device *pdev)
reg |= pdata->software << WM831X_WDOG_RST_SRC_SHIFT;
if (pdata->update_gpio) {
- ret = gpio_request(pdata->update_gpio,
+ ret = gpio_request_cansleep(pdata->update_gpio,
"Watchdog update");
if (ret < 0) {
dev_err(wm831x->dev,
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 4f3d75e..8ae678b 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -128,10 +128,6 @@ extern int gpio_direction_output(unsigned gpio, int value);
extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
-extern int gpio_get_value_cansleep(unsigned gpio);
-extern void gpio_set_value_cansleep(unsigned gpio, int value);
-
-
/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
* the GPIO is constant and refers to some always-present controller,
* giving direct access to chip registers and tight bitbanging loops.
@@ -200,18 +196,6 @@ static inline int gpio_cansleep(unsigned gpio)
return 0;
}
-static inline int gpio_get_value_cansleep(unsigned gpio)
-{
- might_sleep();
- return gpio_get_value(gpio);
-}
-
-static inline void gpio_set_value_cansleep(unsigned gpio, int value)
-{
- might_sleep();
- gpio_set_value(gpio, value);
-}
-
#endif /* !CONFIG_HAVE_GPIO_LIB */
#ifndef CONFIG_GPIO_SYSFS
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 03f616b..d02e4cd 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -33,6 +33,11 @@ static inline int gpio_request(unsigned gpio, const char *label)
return -ENOSYS;
}
+static inline int gpio_request_cansleep(unsigned gpio, const char *label)
+{
+ return -ENOSYS;
+}
+
static inline void gpio_free(unsigned gpio)
{
might_sleep();
@@ -76,19 +81,6 @@ static inline int gpio_cansleep(unsigned gpio)
return 0;
}
-static inline int gpio_get_value_cansleep(unsigned gpio)
-{
- /* GPIO can never have been requested or set as {in,out}put */
- WARN_ON(1);
- return 0;
-}
-
-static inline void gpio_set_value_cansleep(unsigned gpio, int value)
-{
- /* GPIO can never have been requested or set as output */
- WARN_ON(1);
-}
-
static inline int gpio_export(unsigned gpio, bool direction_may_change)
{
/* GPIO can never have been requested or set as {in,out}put */
diff --git a/sound/soc/s3c24xx/s3c24xx_simtec.c b/sound/soc/s3c24xx/s3c24xx_simtec.c
index 4984754..b22b0d4 100644
--- a/sound/soc/s3c24xx/s3c24xx_simtec.c
+++ b/sound/soc/s3c24xx/s3c24xx_simtec.c
@@ -51,8 +51,8 @@ static int speaker_gain_get(struct snd_kcontrol *kcontrol,
*/
static void speaker_gain_set(int value)
{
- gpio_set_value_cansleep(pdata->amp_gain[0], value & 1);
- gpio_set_value_cansleep(pdata->amp_gain[1], value >> 1);
+ gpio_set_value(pdata->amp_gain[0], value & 1);
+ gpio_set_value(pdata->amp_gain[1], value >> 1);
}
/**
@@ -253,13 +253,13 @@ static int attach_gpio_amp(struct device *dev,
/* attach gpio amp gain (if any) */
if (pdata->amp_gain[0] > 0) {
- ret = gpio_request(pd->amp_gain[0], "gpio-amp-gain0");
+ ret = gpio_request_cansleep(pd->amp_gain[0], "gpio-amp-gain0");
if (ret) {
dev_err(dev, "cannot get amp gpio gain0\n");
return ret;
}
- ret = gpio_request(pd->amp_gain[1], "gpio-amp-gain1");
+ ret = gpio_request_cansleep(pd->amp_gain[1], "gpio-amp-gain1");
if (ret) {
dev_err(dev, "cannot get amp gpio gain1\n");
gpio_free(pdata->amp_gain[0]);
--- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
> gpiolib
Again, you're talking about "gpiolib" when
you seem to mean the GPIO framework itself
(for which gpiolib is only an implementation
option)...
> framework could be simplified for cansleep gpios.
>
> 'Can sleep' for a gpio has two different meanings depending
> on context
NO; for the GPIO itself it's only ever had one
meaning, regardless of context.
You're trying to conflate the GPIO and one
of the contexts in which it's used. That's
the problem you seem to be struggling with.
Please stop conflating/confusing
those two disparate concepts...
I hope you don't have such a hard time with
the distinction in other contexts. Like,
the fact that some calls can't be made while
holding spinlocks. That notion is everywhere
in Linux.
> example, if a driver calls gpio_get_value(gpio) from an
> interupt handler
> then the gpio must not be a sleeping gpio.
In a threaded IRQ handler it's OK to use
the get_value_cansleep() option..
>
> This patch introduces a new flag, FLAG_CANSLEEP, internal
> to gpiolib
NAK; Superfluous; the gpio_chip already has
that information recorded.
> new request function, gpio_request_cansleep, requests a
> gpio which may
> only be used from sleep possible contexts
Also superfluous.
The existing
> gpio_request
> function requests a gpio, but does not allow it to be used
> from a
> context where sleep is not possible.
Changing semantics of existing calls is a big
mess, and should be avoided even if it seems
appropriate.
Since the request is just reserving a resource
that's already been identified (and which has
known characteristics, like whether the GPIO
value must be accessed only from sleeping
contexts), this call would also be superfluous.
If you want to ensure the GPIO is a cansleep()
one, just check that before reserving it. There
is no need for new calls to support that model;
it works today.
(NAK...)
> The benefits I see to this approach are:
> ...
> - The API is simplified by combining gpio_(set/get)_value
> and
> gpio_(set/get)_value_cansleep
You have a strange definition of "simplified"...
Recognize that you're also proposing to remove
an API characteristic which much simplifies
code review: you can look at calls and see
that because they're the cansleep() version,
they are unsafe in IRQ context ...
That is, you're making code (and patch)
reviews much harder and more error-prone.
This isn't good, and doesn't simplify any
process I can think of...
So, NAK on these proposed changes.
- Dave
2010/6/23 David Brownell <[email protected]>:
>
>
> --- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
>
>
>> gpiolib
>
>
> Again, you're talking about "gpiolib" when
> you seem to mean the GPIO framework itself
> (for which gpiolib is only an implementation
> option)...
>
>> framework could be simplified for cansleep gpios.
>>
>> 'Can sleep' for a gpio has two different meanings depending
>> on context
>
> NO; for the GPIO itself it's only ever had one
> meaning, regardless of context.
>
> You're trying to conflate the GPIO and one
> of the contexts in which it's used. That's
> the problem you seem to be struggling with.
>
> Please stop conflating/confusing
> those two disparate concepts...
>
> I hope you don't have such a hard time with
> the distinction in other contexts. Like,
> the fact that some calls can't be made while
> holding spinlocks. That notion is everywhere
> in Linux.
>
>
>> example, if a driver calls gpio_get_value(gpio) from an
>> interupt handler
>> then the gpio must not be a sleeping gpio.
>
> In a threaded IRQ handler it's OK to use
> the get_value_cansleep() option..
>
>
>
>>
>> This patch introduces a new flag, FLAG_CANSLEEP, internal
>> to gpiolib
>
> NAK; Superfluous; the gpio_chip already has
> that information recorded.
>
>
>> new request function, gpio_request_cansleep, requests a
>> gpio which may
>> only be used from sleep possible contexts
>
> Also superfluous.
>
>
> The existing
>> gpio_request
>> function requests a gpio, but does not allow it to be used
>> from a
>> context where sleep is not possible.
>
> Changing semantics of existing calls is a big
> mess, and should be avoided even if it seems
> appropriate.
>
> Since the request is just reserving a resource
> that's already been identified (and which has
> known characteristics, like whether the GPIO
> value must be accessed only from sleeping
> contexts), this call would also be superfluous.
>
> If you want to ensure the GPIO is a cansleep()
> one, just check that before reserving it. There
> is no need for new calls to support that model;
> it works today.
>
> (NAK...)
>
>
>
>> The benefits I see to this approach are:
>> ...
>> - The API is simplified by combining gpio_(set/get)_value
>> and
>> gpio_(set/get)_value_cansleep
>
>
> You have a strange definition of "simplified"...
>
> Recognize that you're also proposing to remove
> an API characteristic which much simplifies
> code review: you can look at calls and see
> that because they're the cansleep() version,
> they are unsafe in IRQ context ...
>
> That is, you're making code (and patch)
> reviews much harder and more error-prone.
> This isn't good, and doesn't simplify any
> process I can think of...
>
> So, NAK on these proposed changes.
>
Hi David,
You are really a NAKing machine ;-) People get confused for a reason, and
I believe you have a good solution for this?
On 06/23/2010 04:37 PM, David Brownell wrote:
>
>
> --- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
>
>
>> gpiolib
>
>
> Again, you're talking about "gpiolib" when
> you seem to mean the GPIO framework itself
> (for which gpiolib is only an implementation
> option)...
Fine, its just semantics. I think everyone knows what I mean when I
refer to gpiolib.
>> framework could be simplified for cansleep gpios.
>>
>> 'Can sleep' for a gpio has two different meanings depending
>> on context
>
> NO; for the GPIO itself it's only ever had one
> meaning, regardless of context.
>
> You're trying to conflate the GPIO and one
> of the contexts in which it's used. That's
> the problem you seem to be struggling with.
>
> Please stop conflating/confusing
> those two disparate concepts...
I'm not. Some gpios, such as those on io expanders, may sleep in their
implementations of the gpio_(set/get) functions.
Drivers, which use a gpio, may call gpio_(set/get) functions for a given
gpio from a context where it is not safe to sleep. In this case, a gpio
which may sleep (ie one on an i2c io-expander) cannot be used with this
driver. The gpio_request will succeed, but any call to
gpio_(set/get)_value will produce a warning.
>> example, if a driver calls gpio_get_value(gpio) from an
>> interupt handler
>> then the gpio must not be a sleeping gpio.
>
> In a threaded IRQ handler it's OK to use
> the get_value_cansleep() option..
Ugh, you are really twisting my words. replace 'interrupt handler' with
'non sleep safe context'.
>>
>> This patch introduces a new flag, FLAG_CANSLEEP, internal
>> to gpiolib
>
> NAK; Superfluous; the gpio_chip already has
> that information recorded.
It has a different meaning, but I think you are still correct here. The
might_sleep_if in gpio_(set/get)_value is only necessary if
chip->cansleep is set.
>
>> new request function, gpio_request_cansleep, requests a
>> gpio which may
>> only be used from sleep possible contexts
>
> Also superfluous.
Not so. In my opinion, it is better to have the gpio_request fail
immediately if it is being used incorrectly. For example, say you have
two gpios:
soc_gpio: An SoC gpio which does not sleep on set/get
sleepy_gpio: An i2c io-expander gpio which may sleep on get/set
and some driver code which does this:
gpio_request(gpio, "some_gpio");
...
// Non-sleep safe context
gpio_set_value(gpio, 0);
If you pass sleepy_gpio as the gpio to this driver the gpio_request will
succeed, but you will get a warning on the gpio_set_value because its
usage is incorrect.
In my proposed change, the gpio_request would fail because sleepy_gpio
might sleep, and therefore cannot be used by drivers which use gpios in
non sleep safe context. Basically I am moving the cansleep part of the
api from the usage of gpios to the registration time. In my opinion this
is better because it means there is only one set of gpio_(set/get)_value
calls and incorrect usage of sleeping gpios will be caught when the gpio
is registered, not when it is used.
>
> The existing
>> gpio_request
>> function requests a gpio, but does not allow it to be used
>> from a
>> context where sleep is not possible.
>
> Changing semantics of existing calls is a big
> mess, and should be avoided even if it seems
> appropriate.
>
> Since the request is just reserving a resource
> that's already been identified (and which has
> known characteristics, like whether the GPIO
> value must be accessed only from sleeping
> contexts), this call would also be superfluous.
>
> If you want to ensure the GPIO is a cansleep()
> one, just check that before reserving it. There
> is no need for new calls to support that model;
> it works today.
Except that drivers do not do this.
> (NAK...)
>
>
>
>> The benefits I see to this approach are:
>> ...
>> - The API is simplified by combining gpio_(set/get)_value
>> and
>> gpio_(set/get)_value_cansleep
>
>
> You have a strange definition of "simplified"...
>
> Recognize that you're also proposing to remove
> an API characteristic which much simplifies
> code review: you can look at calls and see
> that because they're the cansleep() version,
> they are unsafe in IRQ context ...
Hmm, fair point.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
On Wed, Jun 23, 2010 at 1:02 PM, Ryan Mallon <[email protected]> wrote:
> On 06/23/2010 04:37 PM, David Brownell wrote:
>>
>>
>> --- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
>>
>>
>>> gpiolib
>>
>>
>> Again, you're talking about "gpiolib" when
>> you seem to mean the GPIO framework itself
>> (for which gpiolib is only an implementation
>> option)...
>
> Fine, its just semantics. I think everyone knows what I mean when I
> refer to gpiolib.
>
>>> framework could be simplified for cansleep gpios.
>>>
>>> 'Can sleep' for a gpio has two different meanings depending
>>> on context
>>
>> NO; for the GPIO itself it's only ever had one
>> meaning, regardless of context.
>>
>> You're trying to conflate the GPIO and one
>> of the contexts in which it's used. That's
>> the problem you seem to be struggling with.
>>
>> Please stop conflating/confusing
>> those two disparate concepts...
>
> I'm not. Some gpios, such as those on io expanders, may sleep in their
> implementations of the gpio_(set/get) functions.
>
> Drivers, which use a gpio, may call gpio_(set/get) functions for a given
> gpio from a context where it is not safe to sleep. In this case, a gpio
> which may sleep (ie one on an i2c io-expander) cannot be used with this
> driver. The gpio_request will succeed, but any call to
> gpio_(set/get)_value will produce a warning.
>
>>> example, if a driver calls gpio_get_value(gpio) from an
>>> interupt handler
>>> then the gpio must not be a sleeping gpio.
>>
>> In a threaded IRQ handler it's OK to use
>> the get_value_cansleep() option..
>
> Ugh, you are really twisting my words. replace 'interrupt handler' with
> 'non sleep safe context'.
>
>>>
>>> This patch introduces a new flag, FLAG_CANSLEEP, internal
>>> to gpiolib
>>
>> NAK; Superfluous; the gpio_chip already has
>> that information recorded.
>
> It has a different meaning, but I think you are still correct here. The
> might_sleep_if in gpio_(set/get)_value is only necessary if
> chip->cansleep is set.
>
>>
>>> new request function, gpio_request_cansleep, requests a
>>> gpio which may
>>> only be used from sleep possible contexts
>>
>> Also superfluous.
>
> Not so. In my opinion, it is better to have the gpio_request fail
> immediately if it is being used incorrectly. For example, say you have
> two gpios:
>
> soc_gpio: An SoC gpio which does not sleep on set/get
> sleepy_gpio: An i2c io-expander gpio which may sleep on get/set
>
> and some driver code which does this:
>
> gpio_request(gpio, "some_gpio");
> ...
> // Non-sleep safe context
> gpio_set_value(gpio, 0);
>
> If you pass sleepy_gpio as the gpio to this driver the gpio_request will
> succeed, but you will get a warning on the gpio_set_value because its
> usage is incorrect.
>
> In my proposed change, the gpio_request would fail because sleepy_gpio
> might sleep, and therefore cannot be used by drivers which use gpios in
> non sleep safe context. Basically I am moving the cansleep part of the
> api from the usage of gpios to the registration time. In my opinion this
> is better because it means there is only one set of gpio_(set/get)_value
> calls and incorrect usage of sleeping gpios will be caught when the gpio
> is registered, not when it is used.
>
>>
>> The existing
>>> gpio_request
>>> function requests a gpio, but does not allow it to be used
>>> from a
>>> context where sleep is not possible.
>>
>> Changing semantics of existing calls is a big
>> mess, and should be avoided even if it seems
>> appropriate.
>>
>> Since the request is just reserving a resource
>> that's already been identified (and which has
>> known characteristics, like whether the GPIO
>> value must be accessed only from sleeping
>> contexts), this call would also be superfluous.
>>
>> If you want to ensure the GPIO is a cansleep()
>> one, just check that before reserving it. There
>> is no need for new calls to support that model;
>> it works today.
>
> Except that drivers do not do this.
>
>> (NAK...)
>>
>>
>>
>>> The benefits I see to this approach are:
>>> ...
>>> - The API is simplified by combining gpio_(set/get)_value
>>> and
>>> gpio_(set/get)_value_cansleep
>>
>>
>> You have a strange definition of "simplified"...
>>
>> Recognize that you're also proposing to remove
>> an API characteristic which much simplifies
>> code review: you can look at calls and see
>> that because they're the cansleep() version,
>> they are unsafe in IRQ context ...
>
> Hmm, fair point.
>
My two cents, if it feels confused to use the _cansleep version, why don't
we just introduce an _atomic() version and use that in atomic situation,
since most usage of gpio API can actually _sleep_. I didn't read all the
scroll back, so if it's wrong, ignore me.
--- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
> > --- On Tue, 6/22/10, Ryan Mallon <[email protected]>
> wrote:
> >
> >
> >> gpiolib
> >
> >
> > Again, you're talking about "gpiolib" when
> > you seem to mean the GPIO framework itself
> > (for which gpiolib is only an implementation
> > option)...
>
> Fine, its just semantics.
Yes, just the foundation of what you're
saying. Stuff that, when you get it wrong,
prevents communication. Try to get it right.
If you can't be bothered to get the basics
right, I'll just have to assume you're being
a troll. There's enough evidence of that in
other parts of your latest message; sorry.
I think everyone knows what I
> mean when I
> refer to gpiolib.
Not without first wasting tiime finding
that you don't really mean "gpiolib"; you
instead mean something else entirely. Sigh.
> >> 'Can sleep' for a gpio has two different meanings
> depending
> >> on context
> >
> > NO; for the GPIO itself it's only ever had one
> > meaning, regardless of context.
> >
> > You're trying to conflate the GPIO and one
> > of the contexts in which it's used.? That's
> > the problem you seem to be struggling with.
> >
> > Please stop conflating/confusing
> > those two disparate concepts...
>
> I'm not.
BUT Your "counter" example below is solid
proof that you are: it shows exactly the
confusion I pointed out: call context versus
the GPIO itself. There's no way I can read
that as anything except "you are"...
Your intent here seems perhaps more to
be a troll than to address any real
technical issues. I don't see much
point participating any further.
Some gpios, such as those on io expanders, may
> sleep in their
> implementations of the gpio_(set/get) functions.
>
Such GPIOs have a "cansleep" attribute, in short.
> Drivers, which use a gpio, may call gpio_(set/get)
> functions for a given
> gpio from a context where it is not safe to sleep.
And that's the call dontext
(in this case, from a driver).
QED. You are confusing two disparate concepts.
In this
> case, a gpio
> which may sleep (ie one on an i2c io-expander) cannot be
> used with this
> driver. The gpio_request will succeed, but any call to
> gpio_(set/get)_value will produce a warning.
>
> >> example, if a driver calls gpio_get_value(gpio)
> from an
> >> interupt handler
(YOU introduce interrupt/IRQ handlers...)
> >> then the gpio must not be a sleeping gpio.
> >
> > In a threaded IRQ handler it's OK to use
> > the get_value_cansleep() option..
>
> Ugh, you are really twisting my words.
You said IRQ handler, so did I. In what csense could I
possibly be "twisting" your words"???
STOP TROLLING.
replace 'interrupt
> handler' with
> 'non sleep safe context'.
No, that would really be "twisting", by
moving words from one place to another
and significantly changing meanings.
> >
> > So, NAK on these proposed changes.
> >
>
> Hi David,
>
> You are really a NAKing machine ;-)
I limit NAKs to bad ideas/code, like this.
People get confused for
> a reason, and
> I believe you have a good solution for this?
Ryan seems to need a cluebat application. I've
tried, but he's resisted, and even denied the most
blatant sources of his personal confusion. While
he esists, I can't do much. for him.
On Sat, 19 Jun 2010, ext David Brownell wrote:
>> The point I was trying to make is that there are lots of drivers which
>> will not work with gpios on sleeping io expanders because they call the
>> spinlock safe gpio calls.
>
> And they will trigger runtime warnings, and thus eventually get fixed.
> The way to do that is to check if the GPIO needs the cansleep() call
>
> That's the first category above: the driver should have used the
> cansleep() variant, and sotriggers a runtime warning.
Hi David -
Part of the reason why such drivers haven't been fixed might be that the
runtime warnings are only issued if DEBUG is defined in gpiolib.c:
/* When debugging, extend minimal trust to callers and platform code.
* Also emit diagnostic messages that may help initial bringup, when
* board setup or driver bugs are most common.
*
* Otherwise, minimize overhead in what may be bitbanging codepaths.
*/
#ifdef DEBUG
#define extra_checks 1
#else
#define extra_checks 0
#endif
...
int __gpio_get_value(unsigned gpio)
{
struct gpio_chip *chip;
chip = gpio_to_chip(gpio);
WARN_ON(extra_checks && chip->can_sleep);
return chip->get ? chip->get(chip, gpio - chip->base) : 0;
}
Do you think it would do more harm than good to unconditionally enable the
extra checks? I do see the comment about overhead there, but having them
enabled would probably aid driver developers in fixing existing code and
choosing the correct calls in the future.
BR,
Jani.
--- On Wed, 6/23/10, Jani Nikula <[email protected]> wrote:
> Hi David -
>
> Part of the reason why such drivers haven't been fixed
> might be that the runtime warnings are only issued if DEBUG
> is defined in gpiolib.c:
A very good point. those cansleep() warnings
should perhaps be issued more consistently.
(Other warnings are safer to skip.)
I think the normal case for the GPIO calls is
the spinlock-safe code path, so I'd probably
ack a patch which incurs the extra costs only
for gpio chips that are already sleeping.
(The desire to trim costs for bitbanging is
not going to affect gpio chips accesssed over
I2C or SPI ... ;)
- Dave
On Wed, 23 Jun 2010, ext David Brownell wrote:
> --- On Wed, 6/23/10, Jani Nikula <[email protected]> wrote:
>
>> Hi David -
>>
>> Part of the reason why such drivers haven't been fixed
>> might be that the runtime warnings are only issued if DEBUG
>> is defined in gpiolib.c:
>
> A very good point. those cansleep() warnings
> should perhaps be issued more consistently.
>
> (Other warnings are safer to skip.)
>
> I think the normal case for the GPIO calls is
> the spinlock-safe code path, so I'd probably
> ack a patch which incurs the extra costs only
> for gpio chips that are already sleeping.
Hi -
I'd think the most important and useful warning would be about
gpio_{get,set}_value() in atomic context on a gpio chip that might sleep.
I seem to have some trouble with my foreign language parser here, so let's
move to a more natural language - see patch below. ;)
If you'd be willing to accept that, with the overhead of one conditional
statement in atomic context for non-sleepy chips, I see no reason not to
go all the way and modify whole gpiolib.c to match extra_check == 1.
BR,
Jani.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3ca3654..33d82b7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1527,7 +1527,7 @@ int __gpio_get_value(unsigned gpio)
struct gpio_chip *chip;
chip = gpio_to_chip(gpio);
- WARN_ON(extra_checks && chip->can_sleep);
+ might_sleep_if(chip->can_sleep);
return chip->get ? chip->get(chip, gpio - chip->base) : 0;
}
EXPORT_SYMBOL_GPL(__gpio_get_value);
@@ -1546,7 +1546,7 @@ void __gpio_set_value(unsigned gpio, int value)
struct gpio_chip *chip;
chip = gpio_to_chip(gpio);
- WARN_ON(extra_checks && chip->can_sleep);
+ might_sleep_if(chip->can_sleep);
chip->set(chip, gpio - chip->base, value);
}
EXPORT_SYMBOL_GPL(__gpio_set_value);
--- On Wed, 6/23/10, Jani Nikula <[email protected]> wrote:
> -??? WARN_ON(extra_checks &&
> chip->can_sleep);
> +??? might_sleep_if(chip->can_sleep);
That looks like the right track. For
those cases the "extra_checks" should
not be optional ... thanks.
- Dave
David Brownell wrote:
>
> --- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
>
>>> --- On Tue, 6/22/10, Ryan Mallon <[email protected]>
>> wrote:
>>>> 'Can sleep' for a gpio has two different meanings
>> depending
>>>> on context
>>> NO; for the GPIO itself it's only ever had one
>>> meaning, regardless of context.
>>>
>>> You're trying to conflate the GPIO and one
>>> of the contexts in which it's used. That's
>>> the problem you seem to be struggling with.
>>>
>>> Please stop conflating/confusing
>>> those two disparate concepts...
>> I'm not.
>
> BUT Your "counter" example below is solid
> proof that you are: it shows exactly the
> confusion I pointed out: call context versus
> the GPIO itself. There's no way I can read
> that as anything except "you are"...
>
>
> Your intent here seems perhaps more to
> be a troll than to address any real
> technical issues. I don't see much
> point participating any further.
>
>
> Some gpios, such as those on io expanders, may
>> sleep in their
>> implementations of the gpio_(set/get) functions.
>>
>
> Such GPIOs have a "cansleep" attribute, in short.
>
>
>> Drivers, which use a gpio, may call gpio_(set/get)
>> functions for a given
>> gpio from a context where it is not safe to sleep.
>
> And that's the call dontext
> (in this case, from a driver).
Yes.
> QED. You are confusing two disparate concepts.
We are saying exactly the same thing.
>
> In this
>> case, a gpio
>> which may sleep (ie one on an i2c io-expander) cannot be
>> used with this
>> driver. The gpio_request will succeed, but any call to
>> gpio_(set/get)_value will produce a warning.
>>
>>>> example, if a driver calls gpio_get_value(gpio)
>> from an
>>>> interupt handler
>
>
> (YOU introduce interrupt/IRQ handlers...)
>
>>>> then the gpio must not be a sleeping gpio.
>>> In a threaded IRQ handler it's OK to use
>>> the get_value_cansleep() option..
>> Ugh, you are really twisting my words.
>
>
> You said IRQ handler, so did I. In what csense could I
> possibly be "twisting" your words"???
>
>
> STOP TROLLING.
Okay, I messed up the wording an used 'interrupt handler' as an example
of a non-sleep safe context. If I had said 'atomic' or 'spinlock'
context you would probably be telling me off for missing some other
non-sleep safe contexts.
The point is that we are discussing the issue of calls which may sleep.
Even if I was not entirely clear by getting the wording wrong, you _do_
know what I am talking about. You could correct on the bits on I get
wrong instead of labeling me a troll.
If we strip my patch back to just introducing gpio_request_cansleep,
which would be used in any driver where all of the calls are
gpio_(set/get)_cansleep, and make gpio_request only allow non-sleeping
gpios then incorrect use of gpios would be caught at request time and
returned to the caller as an error.
~Ryan
Ryan Mallon wrote:
> On 06/23/2010 04:37 PM, David Brownell wrote:
> I'm not. Some gpios, such as those on io expanders, may sleep in their
> implementations of the gpio_(set/get) functions.
I'm having a hard time figuring out where some GPIOs I'm using fit
into this picture.
I have some hardware that is currently using a 2.4.26 kernel, but I
look from time to time at forward-porting all the drivers to 2.6.recent.
It has an I2C driven GPIO expander, with a watchdog reset chip hanging
off the expander.
The watchdog is kept alive off the back end of a timer BH, which means
the I2C GPIO routines are written to be safe in BH context (which
isn't sleepable), but they can't be used in IRQ context because the
necessary spin_lock_irqsave() would turn off interrupts for too long
for other subsystems to function properly.
How should I flag those GPIO routines in your scheme? They're safe to
use in some non-sleeping contexts, but not safe in irq context.
Thanks,
-- Jamie
On 06/24/2010 10:53 AM, Jamie Lokier wrote:
> Ryan Mallon wrote:
>> On 06/23/2010 04:37 PM, David Brownell wrote:
>> I'm not. Some gpios, such as those on io expanders, may sleep in their
>> implementations of the gpio_(set/get) functions.
>
> I'm having a hard time figuring out where some GPIOs I'm using fit
> into this picture.
>
> I have some hardware that is currently using a 2.4.26 kernel, but I
> look from time to time at forward-porting all the drivers to 2.6.recent.
>
> It has an I2C driven GPIO expander, with a watchdog reset chip hanging
> off the expander.
>
> The watchdog is kept alive off the back end of a timer BH, which means
> the I2C GPIO routines are written to be safe in BH context (which
> isn't sleepable), but they can't be used in IRQ context because the
> necessary spin_lock_irqsave() would turn off interrupts for too long
> for other subsystems to function properly.
Do the implementations of the get/set calls for the io expander gpios
sleep at all?
> How should I flag those GPIO routines in your scheme? They're safe to
> use in some non-sleeping contexts, but not safe in irq context.
The idea in my proposal is to use gpio_request in a driver if the
requested gpio can never sleep (ie because of the context it is used
in), and gpio_request_cansleep if the gpio is never used from non-sleep
safe context in a driver. I suggested stripping back the patch to just
add the gpio_request_cansleep function.
In the current code, if a driver ever calls gpio_(set/get)_value on a
gpio then you cannot pass a sleeping gpio to that driver. The request
will succeed, but you will get warnings with the get/get calls are made.
My idea is basically to move the denotation of whether a gpio will be
used in non-sleep safe context to the gpio request.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
Ryan Mallon wrote:
> On 06/24/2010 10:53 AM, Jamie Lokier wrote:
> > Ryan Mallon wrote:
> >> On 06/23/2010 04:37 PM, David Brownell wrote:
> >> I'm not. Some gpios, such as those on io expanders, may sleep in their
> >> implementations of the gpio_(set/get) functions.
> >
> > I'm having a hard time figuring out where some GPIOs I'm using fit
> > into this picture.
> >
> > I have some hardware that is currently using a 2.4.26 kernel, but I
> > look from time to time at forward-porting all the drivers to 2.6.recent.
> >
> > It has an I2C driven GPIO expander, with a watchdog reset chip hanging
> > off the expander.
> >
> > The watchdog is kept alive off the back end of a timer BH, which means
> > the I2C GPIO routines are written to be safe in BH context (which
> > isn't sleepable), but they can't be used in IRQ context because the
> > necessary spin_lock_irqsave() would turn off interrupts for too long
> > for other subsystems to function properly.
>
> Do the implementations of the get/set calls for the io expander gpios
> sleep at all?
No, because sleeping isn't allowed in BH context. (Note that this is
2.4.26 code - things have changed a bit for 2.6, but the hardware is
the same, and still needs the I2C watchdog to be driven from a BH-like
context).
> > How should I flag those GPIO routines in your scheme? They're safe to
> > use in some non-sleeping contexts, but not safe in irq context.
>
> The idea in my proposal is to use gpio_request in a driver if the
> requested gpio can never sleep (ie because of the context it is used
> in), and gpio_request_cansleep if the gpio is never used from non-sleep
> safe context in a driver. I suggested stripping back the patch to just
> add the gpio_request_cansleep function.
>
> In the current code, if a driver ever calls gpio_(set/get)_value on a
> gpio then you cannot pass a sleeping gpio to that driver. The request
> will succeed, but you will get warnings with the get/get calls are made.
> My idea is basically to move the denotation of whether a gpio will be
> used in non-sleep safe context to the gpio request.
The reason I'm asking about my scenario is because the GPIO routines
can't sleep and are used from a non-sleep safe context - but they are
not safe to call in irq contexts.
So my watchdog driver would have to call gpio_request (not _cansleep)
- that's fine. But if I connected other GPIOs from the same GPIO
driver (other lines on the same I/O expander chip) to another
GPIO-using driver which happens to use them from irq context, then
your changes won't detect the problem - the code will just break at
runtime.
Of course if I did that, it would be my fault and my problem. I get
to keep both pieces etc. But it's a scenario which your proposal
would fail to catch at compile time, that's why I bring it up.
-- Jamie
On 06/24/2010 12:04 PM, Jamie Lokier wrote:
> Ryan Mallon wrote:
>> On 06/24/2010 10:53 AM, Jamie Lokier wrote:
>>> Ryan Mallon wrote:
>>>> On 06/23/2010 04:37 PM, David Brownell wrote:
>>>> I'm not. Some gpios, such as those on io expanders, may sleep in their
>>>> implementations of the gpio_(set/get) functions.
>>>
>>> I'm having a hard time figuring out where some GPIOs I'm using fit
>>> into this picture.
>>>
>>> I have some hardware that is currently using a 2.4.26 kernel, but I
>>> look from time to time at forward-porting all the drivers to 2.6.recent.
>>>
>>> It has an I2C driven GPIO expander, with a watchdog reset chip hanging
>>> off the expander.
>>>
>>> The watchdog is kept alive off the back end of a timer BH, which means
>>> the I2C GPIO routines are written to be safe in BH context (which
>>> isn't sleepable), but they can't be used in IRQ context because the
>>> necessary spin_lock_irqsave() would turn off interrupts for too long
>>> for other subsystems to function properly.
>>
>> Do the implementations of the get/set calls for the io expander gpios
>> sleep at all?
>
> No, because sleeping isn't allowed in BH context. (Note that this is
> 2.4.26 code - things have changed a bit for 2.6, but the hardware is
> the same, and still needs the I2C watchdog to be driven from a BH-like
> context).
>
>>> How should I flag those GPIO routines in your scheme? They're safe to
>>> use in some non-sleeping contexts, but not safe in irq context.
>>
>> The idea in my proposal is to use gpio_request in a driver if the
>> requested gpio can never sleep (ie because of the context it is used
>> in), and gpio_request_cansleep if the gpio is never used from non-sleep
>> safe context in a driver. I suggested stripping back the patch to just
>> add the gpio_request_cansleep function.
>>
>> In the current code, if a driver ever calls gpio_(set/get)_value on a
>> gpio then you cannot pass a sleeping gpio to that driver. The request
>> will succeed, but you will get warnings with the get/get calls are made.
>> My idea is basically to move the denotation of whether a gpio will be
>> used in non-sleep safe context to the gpio request.
>
> The reason I'm asking about my scenario is because the GPIO routines
> can't sleep and are used from a non-sleep safe context - but they are
> not safe to call in irq contexts.
>
> So my watchdog driver would have to call gpio_request (not _cansleep)
> - that's fine. But if I connected other GPIOs from the same GPIO
> driver (other lines on the same I/O expander chip) to another
> GPIO-using driver which happens to use them from irq context, then
> your changes won't detect the problem - the code will just break at
> runtime.
>
> Of course if I did that, it would be my fault and my problem. I get
> to keep both pieces etc. But it's a scenario which your proposal
> would fail to catch at compile time, that's why I bring it up.
That's true. It wouldn't be caught in the current code either. I'm not
sure how you would go about sensibly writing code to handle that
situation; it's really a problem that needs to be caught during patch
review.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
Ryan Mallon wrote:
> If we strip my patch back to just introducing gpio_request_cansleep,
> which would be used in any driver where all of the calls are
> gpio_(set/get)_cansleep, and make gpio_request only allow non-sleeping
> gpios then incorrect use of gpios would be caught at request time and
> returned to the caller as an error.
It seems like a good idea to catch these at request time. There is support in the API for this already (gpio_cansleep), but driver writers are not steered towards checking and thinking in these ways by the current API or documentation. Perhaps a documentation change with some cut and paste boilerplate would be enough, but I think some API enforcement/encouragement would be helpful.
I think this agrees with you, Ryan:
gpio_request_cansleep would be the same as current gpio_request
gpio_request changes to error if this is a sleepy gpio.
Imagine a situation where GPIOs are being assigned and passed around between drivers in some dynamic way, or some way unpredictable to the driver writer. In development only non-sleeping GPIOs have been seen and everything is fine. One day someone feeds it a GPIO on an I2C expander and the driver crashes.
If gpio_request had this built-in check the driver could gracefuly fail to load instead with an appropriate error message.
--
Jon Povey
[email protected]
Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .
The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network
Jamie Lokier wrote:
> Ryan Mallon wrote:
>> On 06/24/2010 10:53 AM, Jamie Lokier wrote:
>>> It has an I2C driven GPIO expander, with a watchdog reset chip
>>> hanging off the expander.
>>>
>>> The watchdog is kept alive off the back end of a timer BH, which
>>> means the I2C GPIO routines are written to be safe in BH context
>>> (which
>>> isn't sleepable), but they can't be used in IRQ context because the
>>> necessary spin_lock_irqsave() would turn off interrupts for too long
>>> for other subsystems to function properly.
>>
>> Do the implementations of the get/set calls for the io expander gpios
>> sleep at all?
>
> No, because sleeping isn't allowed in BH context. (Note that this is
> 2.4.26 code - things have changed a bit for 2.6, but the hardware is
> the same, and still needs the I2C watchdog to be driven from a BH-like
> context).
Could you use a workqueue instead to prod the watchdog?
Then the GPIOs could sleep, you could rewrite them as such, and other things could use them safely.
Disabling IRQs while you talk to an I2C peripheral sounds like a hack that will impact performance.
Although, if it does that then I think your GPIOs qualify as non-sleeping.. so "safe" to use from a can't-sleep context but probably unacceptably slow as you say.
--
Jon Povey
[email protected]
Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .
The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network
Hello,
On Thu, Jun 24, 2010 at 07:12:50AM +1200, Ryan Mallon wrote:
> David Brownell wrote:
> >
> > --- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
> >
> >>> --- On Tue, 6/22/10, Ryan Mallon <[email protected]>
> >> wrote:
>
> >>>> 'Can sleep' for a gpio has two different meanings
> >> depending
> >>>> on context
> >>> NO; for the GPIO itself it's only ever had one
> >>> meaning, regardless of context.
> >>>
> >>> You're trying to conflate the GPIO and one
> >>> of the contexts in which it's used. That's
> >>> the problem you seem to be struggling with.
> >>>
> >>> Please stop conflating/confusing
> >>> those two disparate concepts...
> >> I'm not.
> >
> > BUT Your "counter" example below is solid
> > proof that you are: it shows exactly the
> > confusion I pointed out: call context versus
> > the GPIO itself. There's no way I can read
> > that as anything except "you are"...
> >
> >
> > Your intent here seems perhaps more to
> > be a troll than to address any real
> > technical issues. I don't see much
> > point participating any further.
> >
> >
> > Some gpios, such as those on io expanders, may
> >> sleep in their
> >> implementations of the gpio_(set/get) functions.
> >>
> >
> > Such GPIOs have a "cansleep" attribute, in short.
> >
> >
> >> Drivers, which use a gpio, may call gpio_(set/get)
> >> functions for a given
> >> gpio from a context where it is not safe to sleep.
> >
> > And that's the call dontext
> > (in this case, from a driver).
>
> Yes.
>
> > QED. You are confusing two disparate concepts.
>
> We are saying exactly the same thing.
>
> >
> > In this
> >> case, a gpio
> >> which may sleep (ie one on an i2c io-expander) cannot be
> >> used with this
> >> driver. The gpio_request will succeed, but any call to
> >> gpio_(set/get)_value will produce a warning.
> >>
> >>>> example, if a driver calls gpio_get_value(gpio)
> >> from an
> >>>> interupt handler
> >
> >
> > (YOU introduce interrupt/IRQ handlers...)
> >
> >>>> then the gpio must not be a sleeping gpio.
> >>> In a threaded IRQ handler it's OK to use
> >>> the get_value_cansleep() option..
> >> Ugh, you are really twisting my words.
> >
> >
> > You said IRQ handler, so did I. In what csense could I
> > possibly be "twisting" your words"???
> >
> >
> > STOP TROLLING.
>
> Okay, I messed up the wording an used 'interrupt handler' as an example
> of a non-sleep safe context. If I had said 'atomic' or 'spinlock'
> context you would probably be telling me off for missing some other
> non-sleep safe contexts.
>
> The point is that we are discussing the issue of calls which may sleep.
> Even if I was not entirely clear by getting the wording wrong, you _do_
> know what I am talking about. You could correct on the bits on I get
> wrong instead of labeling me a troll.
>
> If we strip my patch back to just introducing gpio_request_cansleep,
> which would be used in any driver where all of the calls are
> gpio_(set/get)_cansleep, and make gpio_request only allow non-sleeping
> gpios then incorrect use of gpios would be caught at request time and
> returned to the caller as an error.
I'm not sure that changing the API in this way is sensible. I'd do
either what Jani Nikula suggested (i.e. substitute some WARN_ON(extra_checks
&& chip->can_sleep); by might_sleep_if(chip->can_sleep);) or
alternatively let gpio_get_value et al. return < 0 if they are called in
atomic context with chip->can_sleep != 0. Maybe even return < 0
independant of the current context?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Jon Povey wrote:
> Ryan Mallon wrote:
>
>
>> If we strip my patch back to just introducing gpio_request_cansleep,
>> which would be used in any driver where all of the calls are
>> gpio_(set/get)_cansleep, and make gpio_request only allow non-sleeping
>> gpios then incorrect use of gpios would be caught at request time and
>> returned to the caller as an error.
>>
>
> It seems like a good idea to catch these at request time. There is support in the API for this already (gpio_cansleep), but driver writers are not steered towards checking and thinking in these ways by the current API or documentation. Perhaps a documentation change with some cut and paste boilerplate would be enough, but I think some API enforcement/encouragement would be helpful.
>
> I think this agrees with you, Ryan:
>
> gpio_request_cansleep would be the same as current gpio_request
> gpio_request changes to error if this is a sleepy gpio.
>
> Imagine a situation where GPIOs are being assigned and passed around between drivers in some dynamic way, or some way unpredictable to the driver writer. In development only non-sleeping GPIOs have been seen and everything is fine. One day someone feeds it a GPIO on an I2C expander and the driver crashes.
> If gpio_request had this built-in check the driver could gracefuly fail to load instead with an appropriate error message.
>
>
>
Hi
Given that that most users will only access the gpios in context where
sleeping is possible it would make more sense to add a special case for
those who use it in contexts where sleeping is not possible.
This wouldn't change the semantics of the current API and furthermore it
is possible to implement it as a small helper function on top of the
current API.
Something like:
int gpio_request_atomic(unsigned gpio, const char *label)
{
if (gpio_cansleep(gpio))
return -EINVAL;
return gpio_request(gpio, label);
}
- Lars
On Thu, 24 Jun 2010, ext Jon Povey wrote:
> Ryan Mallon wrote:
>
>> If we strip my patch back to just introducing gpio_request_cansleep,
>> which would be used in any driver where all of the calls are
>> gpio_(set/get)_cansleep, and make gpio_request only allow non-sleeping
>> gpios then incorrect use of gpios would be caught at request time and
>> returned to the caller as an error.
>
> It seems like a good idea to catch these at request time. There is
> support in the API for this already (gpio_cansleep), but driver writers
> are not steered towards checking and thinking in these ways by the
> current API or documentation. Perhaps a documentation change with some
> cut and paste boilerplate would be enough, but I think some API
> enforcement/encouragement would be helpful.
>
> I think this agrees with you, Ryan:
>
> gpio_request_cansleep would be the same as current gpio_request
> gpio_request changes to error if this is a sleepy gpio.
>
> Imagine a situation where GPIOs are being assigned and passed around
> between drivers in some dynamic way, or some way unpredictable to the
> driver writer. In development only non-sleeping GPIOs have been seen and
> everything is fine. One day someone feeds it a GPIO on an I2C expander
> and the driver crashes. If gpio_request had this built-in check the
> driver could gracefuly fail to load instead with an appropriate error
> message.
Hi -
There's no need to imagine such situations. It's not at all uncommon to
request GPIOs in board files, and pass the already requested GPIO numbers
to drivers. Replacing gpio_request() with gpio_request_cansleep() (or
gpio_request_atomic() as suggested in another mail) in the board files
does *nothing* to help such drivers use the correct gpio get/set calls.
The driver will need to know what it's doing, in what contexts. Some
drivers might not work with "sleepy" GPIOs, and that's fine - they can
check using gpio_cansleep() and fail gracefully.
I'd just improve the documentation of the various calls and have gpiolib.c
emit more warnings about incorrect use.
BR,
Jani.
Jani Nikula wrote:
>
> On Thu, 24 Jun 2010, ext Jon Povey wrote:
>
>> Ryan Mallon wrote:
>>
>>> If we strip my patch back to just introducing gpio_request_cansleep,
>>> which would be used in any driver where all of the calls are
>>> gpio_(set/get)_cansleep, and make gpio_request only allow
>>> non-sleeping gpios then incorrect use of gpios would be caught at
>>> request time and returned to the caller as an error.
>>
>> It seems like a good idea to catch these at request time. There is
>> support in the API for this already (gpio_cansleep), but driver
>> writers are not steered towards checking and thinking in these ways
>> by the current API or documentation. Perhaps a documentation change
>> with some cut and paste boilerplate would be enough, but I think some
>> API enforcement/encouragement would be helpful.
>>
>> I think this agrees with you, Ryan:
>>
>> gpio_request_cansleep would be the same as current gpio_request
>> gpio_request changes to error if this is a sleepy gpio.
>>
>> Imagine a situation where GPIOs are being assigned and passed around
>> between drivers in some dynamic way, or some way unpredictable to the
>> driver writer. In development only non-sleeping GPIOs have been seen
>> and everything is fine. One day someone feeds it a GPIO on an I2C
>> expander and the driver crashes. If gpio_request had this built-in
>> check the driver could gracefuly fail to load instead with an
>> appropriate error message.
>
> Hi -
>
> There's no need to imagine such situations. It's not at all uncommon
> to request GPIOs in board files, and pass the already requested GPIO
> numbers to drivers. Replacing gpio_request() with
> gpio_request_cansleep() (or gpio_request_atomic() as suggested in
> another mail) in the board files does *nothing* to help such drivers
> use the correct gpio get/set calls. The driver will need to know what
> it's doing, in what contexts. Some drivers might not work with
> "sleepy" GPIOs, and that's fine - they can check using gpio_cansleep()
> and fail gracefully.
Is there a reason, why a gpio is requested in the board file and not in
the driver? I would have considered that the later is far more common.
Sure, drivers which do not request the gpios themselves would have to
call gpio_cansleep, but for those which request the gpios themselves it
would help to reduce code clutter to have a gpio_request_atomic. The
only argument speaking against adding such a helper function would be
that drivers accessing gpios in contexts where they can not sleep are
far to rare to bother.
- Lars
--- On Wed, 6/23/10, Ryan Mallon <[email protected]> wrote:
> >> In the current code, if a driver ever calls
> gpio_(set/get)_value on a
> >> gpio then you cannot pass a sleeping gpio to that
> driver. The request
> >> will succeed,
No it won't. Recall that those bit accessor
functions don't return success or failure!!
I'll be submitting a patch soonish, which is
a variant of Jani's patch to make the runtime
warnings trigger more consistently in the
face of such programming errors.
non-sleeping
On Mon, Jun 21, 2010 at 05:31, Ryan Mallon <[email protected]> wrote:
> On 06/19/2010 06:21 PM, David Brownell wrote:
>>
>>>
>>> The runtime warnings will only show instances where the
>>> non-sleeping
>>> versions where called instead of the sleeping versions.
>>
>> ... *AND* the GPIO requires the cansleep() version...
>>
>> Right; such calls are errors. ?We issue
>> warnings since fault returns are inapplicable.
>
> A driver which only uses the non-sleeping versions, but _could_ use the
> cansleep variants (ie all calls to gpio_(set/get)_value are made from
> contexts where it is possible to sleep) is not so easy to spot. Passing
> a sleeping to gpio to such a driver will result in spurious warnings.
>
>>> There is no
>>> warning to say that we are calling the spinlock safe
>>> version, where it is possible to sleep.
>>
>> The call context isn't what controls whether
>> gpio_get_value() or gpio_get_value_cansleep()
>> is appropriate ... it's the GPIO itself, and
>> how its implementation works.
>
> No, a driver should not know anything about a gpio which is passed to
> it. If a driver is able to call the cansleep variants, then it should,
> and it will allow any gpio, sleeping or non-sleeping, to be used with
> that driver.
>
> If a driver uses a gpio in such a way that it cannot sleep, ie the
> gpio_(get/set)_value calls are made from spinlock context, then only
> gpios which do not sleep may be used with that driver.
>
> Thats why I think specifying whether the gpio is able to sleep when it
> is requested is a good idea. A driver which cannot use a sleeping gpio
>
>
>> "possible to sleep" is a GPIO attribute,
>> exposed by a predicate. ?If spinlock-safe
>> calls are used on GPIOs with that attribute,
>> ?a warning *IS* issued.
>
> Possible to sleep is also an attribute of how a driver _uses_ a gpio.
>
>>>
>>> The point I was trying to make is that there are lots of
>>> drivers which
>>> will not work with gpios on sleeping io expanders because
>>> they call the
>>> spinlock safe gpio calls.
>>
>> And they will trigger runtime warnings, and
>> thus eventually get fixed. ?The way to do that
>> is to check if the GPIO needs the cansleep()
>> call
>
> Hmm, maybe this then for drivers which cannot accept sleeping gpios:
>
> ?if (gpio_cansleep(some_gpio)) {
> ? ? ? ? ?dev_err(&dev, "This driver only supports non-sleeping gpios");
> ? ? ? ? ?return -EINVAL;
> ?}
>
> ?err = gpio_request(some_gpio, "some_gpio");
>
> I think ideally, gpio_request should specify this via a flags argument, ie:
>
> ?#define GPIOF_NO_SLEEP ? ? ? ?0x0
> ?#define GPIOF_CANSLEEP ? ? ? ?0x1
>
> ?err = gpio_request(some_gpio, "some_gpio", GPIOF_NO_SLEEP);
>
> ~Ryan
>
> --
> Bluewater Systems Ltd - ARM Technology Solution Centre
>
> Ryan Mallon ? ? ? ? ? ? ? ? ? ? 5 Amuri Park, 404 Barbadoes St
> [email protected] ? ? ? ? ? PO Box 13 889, Christchurch 8013
> http://www.bluewatersys.com ? ? New Zealand
> Phone: +64 3 3779127 ? ? ? ? ? ?Freecall: Australia 1800 148 751
> Fax: ? +64 3 3779135 ? ? ? ? ? ? ? ? ? ? ?USA 1800 261 2934
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>