gpiolib and sleeping gpios
Subsystems (16)
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
Signature · 7 lines
Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St ryan at bluewatersys.com 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,
Quoted 3 lines · Ryan Mallon
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_ifcheck. 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.
Quoted 3 lines
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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
Quoted 3 lines · Ryan Mallon
Currently implementors of gpiolib must provide
implementations forgpio_get_value,gpio_set_valueandgpio_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
Quoted 9 lines
the
implementations just #define these to the double underscore prefixed
versions indrivers/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...
Quoted 3 lines
Although
gpio_chipsknow 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
Quoted 3 lines
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.
Quoted 1 line
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.
Quoted 7 lines
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.
Quoted 8 lines
Would it not be simpler to combine the calls and have something like this:
void
__gpio_get_value(unsigned gpio, int value)
{
??? structgpio_chip*chip;
Quoted 6 lines
??? 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.
Quoted 3 lines
}
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
Quoted 5 lines
attempts to
use sleeping gpios from an non-sleeping context will be caught by themight_sleep_ifcheck. 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...
Quoted 27 lines
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,
~RyanSignature · 15 lines
Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon? ? ? ?????? ??? 5 Amuri Park, 404 Barbadoes St ryan at bluewatersys.com? ? ? ?????? 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
Quoted 6 lines · David Brownell
Quoted 3 lines · Ryan Mallon
Currently implementors of gpiolib must provide
implementations forgpio_get_value,gpio_set_valueandgpio_cansleep.
Quoted 6 lines
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>
Quoted 20 lines
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.
Quoted 1 line
the cansleep versions calls
might_sleep_if. Most driversThat'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
Quoted 5 lines
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.
Quoted 10 lines
Quoted 8 lines
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.
Quoted 3 lines
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.
Quoted 18 lines
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 thecansleep()version when it should have been used.Quoted 11 lines
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,
Quoted 8 lines
Quoted 1 line
value);
calling
chip->set()whenchip->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
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 10 lines · David Brownell
Quoted 5 lines
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.
Quoted 8 lines
Quoted 3 lines
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()orgpio_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
Quoted 4 lines
"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.
Quoted 11 lines
Quoted 6 lines
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
Signature · 7 lines
Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St ryan at bluewatersys.com 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
Quoted 4 lines · Ryan Mallon
Quoted 4 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 3 lines
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,
Quoted 9 lines · David Brownell
Quoted 7 lines
Quoted 2 lines
And they will trigger runtime warnings, and
thus eventually get fixed.\
? }? err = gpio_request(some_gpio, "
some_gpio",GPIOF_NO_SLEEP);
Quoted 8 lines
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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
RFC PATCH 1/1 Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 23 lines · Uwe Kleine-K?nig
Hi,
Quoted 10 lines · David Brownell
Quoted 9 lines
Quoted 3 lines
And they will trigger runtime warnings, and
thus eventually get fixed.\
}err = gpio_request(some_gpio, "
some_gpio",GPIOF_NO_SLEEP);Quoted 9 lines
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.
Quoted 7 lines
I'm against such an additional flag, too. But I still think merging
gpio_get_valueandgpio_get_value_cansleepis 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(-)
arch/arm/mach-davinci/board-dm355-evm.c +6 -6
@@ -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 = {
arch/arm/mach-davinci/board-dm355-leopard.c +6 -6
@@ -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 = {
arch/arm/mach-davinci/board-dm644x-evm.c +2 -2
@@ -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 )
drivers/gpio/gpiolib.c +34 -33
140 lines not on this page · raw message
drivers/input/keyboard/matrix_keypad.c +6 -4
@@ -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",
drivers/leds/leds-gpio.c +5 -2
@@ -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;
drivers/leds/leds-lt3593.c +7 -7
@@ -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;
drivers/mmc/host/omap_hsmmc.c +6 -5
@@ -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);
drivers/regulator/fixed.c +3 -3
@@ -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",
drivers/usb/musb/davinci.c +2 -2
@@ -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); }
drivers/watchdog/wm831x_wdt.c +2 -2
@@ -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,
include/asm-generic/gpio.h +0 -16
@@ -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
include/linux/gpio.h +5 -13
@@ -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 */
sound/soc/s3c24xx/s3c24xx_simtec.c +4 -4
@@ -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 ]);
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 1 line · Ryan Mallon
gpiolib
Again, you're talking about "gpiolib" when you seem to mean the GPIO framework itself (for which gpiolib is only an implementation option)...
Quoted 4 lines
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.
Quoted 3 lines
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..
Quoted 3 lines
This patch introduces a new flag,
FLAG_CANSLEEP, internal to gpiolib
NAK; Superfluous; the gpio_chip already has that information recorded.
Quoted 3 lines
new request function,
gpio_request_cansleep, requests a gpio which may
only be used from sleep possible contexts
Also superfluous.
The existing
Quoted 4 lines
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...)
Quoted 5 lines
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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
2010/6/23 David Brownell <[email protected]>:
Quoted 99 lines
Quoted 1 line · Ryan Mallon
gpiolib
Again, you're talking about "gpiolib" when you seem to mean the GPIO framework itself (for which gpiolib is only an implementation option)...
Quoted 4 lines
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.
Quoted 3 lines
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..Quoted 3 lines
This patch introduces a new flag,
FLAG_CANSLEEP, internal to gpiolibNAK; Superfluous; the
gpio_chipalready has that information recorded.Quoted 3 lines
new request function,
gpio_request_cansleep, requests a gpio which may
only be used from sleep possible contextsAlso superfluous.
?The existing
Quoted 4 lines
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...)
Quoted 5 lines
The benefits I see to this approach are:
?...
?- The API is simplified by combining gpio_(set/get)_value and
gpio_(set/get)_value_cansleepYou 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 ...
Quoted 7 lines
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?
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 8 lines
Quoted 4 lines
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
Quoted 2 lines
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.
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
In reply to David BrownellQuoted 5 lines · David Brownell
--- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
Quoted 7 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
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.
Quoted 6 lines
Quoted 3 lines
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'.
Quoted 6 lines
Quoted 3 lines
This patch introduces a new flag,
FLAG_CANSLEEP, internal to gpiolibNAK; Superfluous; the
gpio_chipalready 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.
Quoted 6 lines
Quoted 4 lines
new request function,
gpio_request_cansleep, requests a gpio which may
only be used from sleep possible contextsAlso 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.
Quoted 17 lines
The existing
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 11 lines
(NAK...)
Quoted 5 lines
The benefits I see to this approach are:
...
- The API is simplified by combining gpio_(set/get)_value
and
gpio_(set/get)_value_cansleep
Quoted 7 lines
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
Signature · 7 lines
Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St ryan at bluewatersys.com 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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 132 lines · Ryan Mallon
Quoted 5 lines · David Brownell
--- On Tue, 6/22/10, Ryan Mallon <[email protected]> wrote:
Quoted 7 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
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. Thegpio_requestwill succeed, but any call to gpio_(set/get)_value will produce a warning.Quoted 6 lines
Quoted 3 lines
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'.
Quoted 6 lines
Quoted 3 lines
This patch introduces a new flag,
FLAG_CANSLEEP, internal to gpiolibNAK; Superfluous; the
gpio_chipalready has that information recorded.It has a different meaning, but I think you are still correct here. The
might_sleep_ifin gpio_(set/get)_value is only necessary ifchip->cansleepis set.Quoted 6 lines
Quoted 4 lines
new request function,
gpio_request_cansleep, requests a gpio which may
only be used from sleep possible contextsAlso superfluous.
Not so. In my opinion, it is better to have the
gpio_requestfail 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_gpioas the gpio to this driver thegpio_requestwill succeed, but you will get a warning on thegpio_set_valuebecause its usage is incorrect.In my proposed change, the
gpio_requestwould fail becausesleepy_gpiomight 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.Quoted 17 lines
?The existing
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 13 lines
(NAK...)
Quoted 5 lines
The benefits I see to this approach are:
?...
?- The API is simplified by combining gpio_(set/get)_value and
gpio_(set/get)_value_cansleepYou have a strange definition of "simplified"...
Quoted 8 lines
Quoted 5 lines
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.
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
In reply to Ryan MallonQuoted 7 lines · Ryan Mallon
Quoted 1 line
--- On Tue, 6/22/10, Ryan Mallon <[email protected]>
Quoted 5 lines · wrote:
Quoted 3 lines
gpiolib
Quoted 6 lines
Quoted 4 lines
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
Quoted 2 lines
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.
Quoted 7 lines
Quoted 1 line
Quoted 1 line
'Can sleep' for a gpio has two different meanings
depending
Quoted 5 lines
Quoted 1 line
on context
NO; for the GPIO itself it's only ever had one meaning, regardless of context.
Quoted 8 lines
Quoted 6 lines
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
Quoted 3 lines
sleep in their
implementations of thegpio_(set/get)functions.
Such GPIOs have a "cansleep" attribute, in short.
Quoted 3 lines
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
Quoted 9 lines
case, a gpio
which may sleep (ie one on an i2c io-expander) cannot be used with this
driver. Thegpio_requestwill succeed, but any call to gpio_(set/get)_value will produce a warning.Quoted 1 line
Quoted 1 line
example, if a driver calls
gpio_get_value(gpio)from an
Quoted 1 line
Quoted 1 line
interupt handler
(YOU introduce interrupt/IRQ handlers...)
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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
Quoted 2 lines
handler' with
'non sleep safe context'.
No, that would really be "twisting", by moving words from one place to another and significantly changing meanings.
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 41 lines · David Brownell
Quoted 9 lines · Ryan Mallon
Quoted 1 line
--- On Tue, 6/22/10, Ryan Mallon <[email protected]>
Quoted 1 line · wrote:
Quoted 1 line
'Can sleep' for a gpio has two different meanings
depending
Quoted 4 lines
Quoted 1 line
on context
NO; for the GPIO itself it's only ever had one meaning, regardless of context.
Quoted 7 lines
Quoted 6 lines
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 versusthe 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
Quoted 3 lines
sleep in their
implementations of thegpio_(set/get)functions.Such GPIOs have a "cansleep" attribute, in short.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 1 line
QED. You are confusing two disparate concepts.
We are saying exactly the same thing.
Quoted 18 lines
In this
Quoted 9 lines
case, a gpio
which may sleep (ie one on an i2c io-expander) cannot be used with this
driver. Thegpio_requestwill succeed, but any call to gpio_(set/get)_value will produce a warning.Quoted 1 line
Quoted 1 line
example, if a driver calls
gpio_get_value(gpio)from an
Quoted 1 line
Quoted 1 line
interupt handler
(YOU introduce interrupt/IRQ handlers...)
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
then the gpio must not be a sleeping gpio.
In a threaded IRQ handler it's OK to use the
get_value_cansleep()option..
Quoted 8 lines
Quoted 1 line
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
RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)
Quoted 5 lines · Ryan Mallon
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 makegpio_requestonly 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
jon.povey at racelogic.co.uk
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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)
Quoted 12 lines · Jon Povey
Quoted 6 lines · Ryan Mallon
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 makegpio_requestonly 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.
Quoted 10 lines
I think this agrees with you, Ryan:
gpio_request_cansleepwould be the same as currentgpio_requestgpio_requestchanges 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_requesthad 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
RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)
In reply to Jon PoveyQuoted 20 lines · ext Jon Povey
Quoted 5 lines · Ryan Mallon
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 makegpio_requestonly 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_cansleepwould be the same as currentgpio_requestgpio_requestchanges to error if this is a sleepy gpio.
Quoted 7 lines
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_requesthad 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.
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)
Quoted 33 lines · Jani Nikula
Quoted 20 lines · ext Jon Povey
Quoted 5 lines · Ryan Mallon
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 makegpio_requestonly 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_cansleepwould be the same as currentgpio_requestgpio_requestchanges to error if this is a sleepy gpio.Quoted 7 lines
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_requesthad this built-in check the driver could gracefuly fail to load instead with an appropriate error message.Hi -
Quoted 9 lines
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()withgpio_request_cansleep()(orgpio_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 usinggpio_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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
In reply to Ryan MallonHello,
Quoted 92 lines · Ryan Mallon
Quoted 41 lines · David Brownell
Quoted 9 lines · Ryan Mallon
Quoted 1 line
--- On Tue, 6/22/10, Ryan Mallon <[email protected]>
Quoted 1 line · wrote:
Quoted 1 line
'Can sleep' for a gpio has two different meanings
depending
Quoted 4 lines
Quoted 1 line
on context
NO; for the GPIO itself it's only ever had one meaning, regardless of context.
Quoted 7 lines
Quoted 6 lines
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 versusthe 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
Quoted 3 lines
sleep in their
implementations of thegpio_(set/get)functions.Such GPIOs have a "cansleep" attribute, in short.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 1 line
QED. You are confusing two disparate concepts.
We are saying exactly the same thing.
Quoted 18 lines
In this
Quoted 9 lines
case, a gpio
which may sleep (ie one on an i2c io-expander) cannot be used with this
driver. Thegpio_requestwill succeed, but any call to gpio_(set/get)_value will produce a warning.Quoted 1 line
Quoted 1 line
example, if a driver calls
gpio_get_value(gpio)from an
Quoted 1 line
Quoted 1 line
interupt handler
(YOU introduce interrupt/IRQ handlers...)
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
then the gpio must not be a sleeping gpio.
In a threaded IRQ handler it's OK to use the
get_value_cansleep()option..Quoted 8 lines
Quoted 1 line
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.
Quoted 5 lines
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 makegpio_requestonly 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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
In reply to Ryan MallonQuoted 3 lines · Ryan Mallon
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 thegpio_(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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 11 lines · Jamie Lokier
Quoted 3 lines · Ryan Mallon
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 thegpio_(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.
Quoted 8 lines
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?
Quoted 2 lines
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
Signature · 7 lines
Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St ryan at bluewatersys.com 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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 15 lines · Ryan Mallon
Quoted 14 lines · Jamie Lokier
Quoted 3 lines · Ryan Mallon
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 thegpio_(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.
Quoted 8 lines
Quoted 5 lines
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).
Quoted 9 lines
Quoted 2 lines
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_requestin a driver if the requested gpio can never sleep (ie because of the context it is used in), andgpio_request_cansleepif the gpio is never used from non-sleep safe context in a driver. I suggested stripping back the patch to just add thegpio_request_cansleepfunction.
Quoted 5 lines
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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 56 lines · Jamie Lokier
Quoted 15 lines · Ryan Mallon
Quoted 14 lines · Jamie Lokier
Quoted 3 lines · Ryan Mallon
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 thegpio_(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.
Quoted 8 lines
Quoted 5 lines
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).
Quoted 9 lines
Quoted 2 lines
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_requestin a driver if the requested gpio can never sleep (ie because of the context it is used in), andgpio_request_cansleepif the gpio is never used from non-sleep safe context in a driver. I suggested stripping back the patch to just add thegpio_request_cansleepfunction.Quoted 5 lines
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.
Quoted 3 lines
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
Signature · 7 lines
Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St ryan at bluewatersys.com 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
Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)
Quoted 5 lines · Ryan Mallon
Quoted 1 line
Quoted 1 line
In the current code, if a driver ever calls
gpio_(set/get)_value on a
Quoted 1 line
Quoted 1 line
gpio then you cannot pass a sleeping gpio to that
driver. The request
Quoted 1 line
Quoted 1 line
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.
RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)
In reply to Jamie LokierQuoted 13 lines · Jamie Lokier
Quoted 12 lines · Ryan Mallon
Quoted 9 lines · Jamie Lokier
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 necessaryspin_lock_irqsave()would turn off interrupts for too long for other subsystems to function properly.
Quoted 7 lines
Quoted 2 lines
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
jon.povey at racelogic.co.uk
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
non-sleeping
Quoted 90 lines · Ryan Mallon
Quoted 10 lines · David Brownell
Quoted 5 lines
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.
Quoted 8 lines
Quoted 3 lines
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()orgpio_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
Quoted 4 lines
"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.
Quoted 11 lines
Quoted 6 lines
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()callHmm, 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_requestshould specify this via a flags argument, ie:?#define
GPIOF_NO_SLEEP? ? ? ?0x0
?#defineGPIOF_CANSLEEP? ? ? ?0x1?err =
gpio_request(some_gpio, "some_gpio", GPIOF_NO_SLEEP);~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution CentreRyan Mallon ? ? ? ? ? ? ? ? ? ? 5 Amuri Park, 404 Barbadoes St ryan at bluewatersys.com ? ? ? ? ? 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 majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html Please read the FAQ at ?http://www.tux.org/lkml/
Quoted 9 lines · ext David Brownell
Quoted 3 lines
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()callThat'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.
Quoted 5 lines · Jani Nikula
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
Quoted 11 lines · ext David Brownell
Quoted 5 lines · Jani Nikula
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.
Quoted 6 lines
(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.
drivers/gpio/gpiolib.c +2 -2
@@ -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);
Quoted 3 lines · Jani Nikula
-??? 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