* gpiolib and sleeping gpios @ 2010-06-17 21:47 Ryan Mallon 2010-06-18 5:27 ` Uwe Kleine-König 2010-06-18 6:16 ` David Brownell 0 siblings, 2 replies; 33+ messages in thread From: Ryan Mallon @ 2010-06-17 21:47 UTC (permalink / raw) To: linux kernel Cc: linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König, ext-jani.1.nikula 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 ryan@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-17 21:47 gpiolib and sleeping gpios Ryan Mallon @ 2010-06-18 5:27 ` Uwe Kleine-König 2010-06-18 6:16 ` David Brownell 1 sibling, 0 replies; 33+ messages in thread From: Uwe Kleine-König @ 2010-06-18 5:27 UTC (permalink / raw) To: Ryan Mallon Cc: linux kernel, linux-arm-kernel, Andrew Morton, David Brownell, gregkh, ext-jani.1.nikula 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/ | ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-17 21:47 gpiolib and sleeping gpios Ryan Mallon 2010-06-18 5:27 ` Uwe Kleine-König @ 2010-06-18 6:16 ` David Brownell 2010-06-18 22:01 ` Ryan Mallon 1 sibling, 1 reply; 33+ messages in thread From: David Brownell @ 2010-06-18 6:16 UTC (permalink / raw) To: linux kernel, Ryan Mallon Cc: linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König, ext-jani.1.nikula --- On Thu, 6/17/10, Ryan Mallon <ryan@bluewatersys.com> 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 > ryan@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 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-18 6:16 ` David Brownell @ 2010-06-18 22:01 ` Ryan Mallon 2010-06-19 6:21 ` David Brownell 0 siblings, 1 reply; 33+ messages in thread From: Ryan Mallon @ 2010-06-18 22:01 UTC (permalink / raw) To: David Brownell Cc: linux kernel, linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König, ext-jani.1.nikula David Brownell wrote: > > --- On Thu, 6/17/10, Ryan Mallon <ryan@bluewatersys.com> 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-18 22:01 ` Ryan Mallon @ 2010-06-19 6:21 ` David Brownell 2010-06-20 21:31 ` Ryan Mallon 2010-06-23 11:53 ` Jani Nikula 0 siblings, 2 replies; 33+ messages in thread From: David Brownell @ 2010-06-19 6:21 UTC (permalink / raw) To: Ryan Mallon Cc: linux kernel, linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König, ext-jani.1.nikula > > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-19 6:21 ` David Brownell @ 2010-06-20 21:31 ` Ryan Mallon 2010-06-21 2:40 ` David Brownell 2010-06-29 8:29 ` gpiolib and sleeping gpios CoffBeta 2010-06-23 11:53 ` Jani Nikula 1 sibling, 2 replies; 33+ messages in thread From: Ryan Mallon @ 2010-06-20 21:31 UTC (permalink / raw) To: David Brownell Cc: David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 ryan@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-20 21:31 ` Ryan Mallon @ 2010-06-21 2:40 ` David Brownell 2010-06-21 5:09 ` Uwe Kleine-König 2010-06-29 8:29 ` gpiolib and sleeping gpios CoffBeta 1 sibling, 1 reply; 33+ messages in thread From: David Brownell @ 2010-06-21 2:40 UTC (permalink / raw) To: Ryan Mallon Cc: David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel --- On Sun, 6/20/10, Ryan Mallon <ryan@bluewatersys.com> 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-21 2:40 ` David Brownell @ 2010-06-21 5:09 ` Uwe Kleine-König 2010-06-23 1:59 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) Ryan Mallon 0 siblings, 1 reply; 33+ messages in thread From: Uwe Kleine-König @ 2010-06-21 5:09 UTC (permalink / raw) To: David Brownell Cc: Ryan Mallon, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel 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/ | ^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-21 5:09 ` Uwe Kleine-König @ 2010-06-23 1:59 ` Ryan Mallon 2010-06-23 4:37 ` David Brownell 0 siblings, 1 reply; 33+ messages in thread From: Ryan Mallon @ 2010-06-23 1:59 UTC (permalink / raw) To: Uwe Kleine-König Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel 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]); ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 1:59 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) Ryan Mallon @ 2010-06-23 4:37 ` David Brownell 2010-06-23 4:58 ` Eric Miao 2010-06-23 5:02 ` Ryan Mallon 0 siblings, 2 replies; 33+ messages in thread From: David Brownell @ 2010-06-23 4:37 UTC (permalink / raw) To: Uwe Kleine-König, Ryan Mallon Cc: David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 4:37 ` David Brownell @ 2010-06-23 4:58 ` Eric Miao 2010-06-23 9:51 ` David Brownell 2010-06-23 5:02 ` Ryan Mallon 1 sibling, 1 reply; 33+ messages in thread From: Eric Miao @ 2010-06-23 4:58 UTC (permalink / raw) To: David Brownell Cc: Uwe Kleine-König, Ryan Mallon, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel 2010/6/23 David Brownell <david-b@pacbell.net>: > > > --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> 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? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 4:58 ` Eric Miao @ 2010-06-23 9:51 ` David Brownell 0 siblings, 0 replies; 33+ messages in thread From: David Brownell @ 2010-06-23 9:51 UTC (permalink / raw) To: Eric Miao Cc: Uwe Kleine-König, Ryan Mallon, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel > > > > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 4:37 ` David Brownell 2010-06-23 4:58 ` Eric Miao @ 2010-06-23 5:02 ` Ryan Mallon 2010-06-23 5:26 ` Eric Miao ` (2 more replies) 1 sibling, 3 replies; 33+ messages in thread From: Ryan Mallon @ 2010-06-23 5:02 UTC (permalink / raw) To: David Brownell Cc: Uwe Kleine-König, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel On 06/23/2010 04:37 PM, David Brownell wrote: > > > --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> 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 ryan@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 5:02 ` Ryan Mallon @ 2010-06-23 5:26 ` Eric Miao 2010-06-23 9:39 ` David Brownell 2010-06-23 22:53 ` Jamie Lokier 2 siblings, 0 replies; 33+ messages in thread From: Eric Miao @ 2010-06-23 5:26 UTC (permalink / raw) To: Ryan Mallon Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel On Wed, Jun 23, 2010 at 1:02 PM, Ryan Mallon <ryan@bluewatersys.com> wrote: > On 06/23/2010 04:37 PM, David Brownell wrote: >> >> >> --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 5:02 ` Ryan Mallon 2010-06-23 5:26 ` Eric Miao @ 2010-06-23 9:39 ` David Brownell 2010-06-23 19:12 ` Ryan Mallon 2010-06-23 22:53 ` Jamie Lokier 2 siblings, 1 reply; 33+ messages in thread From: David Brownell @ 2010-06-23 9:39 UTC (permalink / raw) To: Ryan Mallon Cc: Uwe Kleine-König, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> wrote: > > --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 9:39 ` David Brownell @ 2010-06-23 19:12 ` Ryan Mallon 2010-06-24 4:46 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey 2010-06-24 6:41 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) Uwe Kleine-König 0 siblings, 2 replies; 33+ messages in thread From: Ryan Mallon @ 2010-06-23 19:12 UTC (permalink / raw) To: David Brownell Cc: Uwe Kleine-König, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel David Brownell wrote: > > --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> wrote: > >>> --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> >> 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) 2010-06-23 19:12 ` Ryan Mallon @ 2010-06-24 4:46 ` Jon Povey 2010-06-24 8:20 ` Lars-Peter Clausen 2010-06-24 8:29 ` Jani Nikula 2010-06-24 6:41 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) Uwe Kleine-König 1 sibling, 2 replies; 33+ messages in thread From: Jon Povey @ 2010-06-24 4:46 UTC (permalink / raw) To: Ryan Mallon, David Brownell Cc: David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 jon.povey@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) 2010-06-24 4:46 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey @ 2010-06-24 8:20 ` Lars-Peter Clausen 2010-06-24 8:29 ` Jani Nikula 1 sibling, 0 replies; 33+ messages in thread From: Lars-Peter Clausen @ 2010-06-24 8:20 UTC (permalink / raw) To: Jon Povey Cc: Ryan Mallon, David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) 2010-06-24 4:46 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey 2010-06-24 8:20 ` Lars-Peter Clausen @ 2010-06-24 8:29 ` Jani Nikula 2010-06-24 10:31 ` Lars-Peter Clausen 1 sibling, 1 reply; 33+ messages in thread From: Jani Nikula @ 2010-06-24 8:29 UTC (permalink / raw) To: ext Jon Povey Cc: Ryan Mallon, David Brownell, David Brownell, gregkh, linux kernel, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) 2010-06-24 8:29 ` Jani Nikula @ 2010-06-24 10:31 ` Lars-Peter Clausen 0 siblings, 0 replies; 33+ messages in thread From: Lars-Peter Clausen @ 2010-06-24 10:31 UTC (permalink / raw) To: Jani Nikula Cc: ext Jon Povey, Ryan Mallon, David Brownell, David Brownell, gregkh, linux kernel, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 19:12 ` Ryan Mallon 2010-06-24 4:46 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey @ 2010-06-24 6:41 ` Uwe Kleine-König 1 sibling, 0 replies; 33+ messages in thread From: Uwe Kleine-König @ 2010-06-24 6:41 UTC (permalink / raw) To: Ryan Mallon Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Andrew Morton, linux-arm-kernel Hello, On Thu, Jun 24, 2010 at 07:12:50AM +1200, Ryan Mallon wrote: > David Brownell wrote: > > > > --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> wrote: > > > >>> --- On Tue, 6/22/10, Ryan Mallon <ryan@bluewatersys.com> > >> 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/ | ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 5:02 ` Ryan Mallon 2010-06-23 5:26 ` Eric Miao 2010-06-23 9:39 ` David Brownell @ 2010-06-23 22:53 ` Jamie Lokier 2010-06-23 23:06 ` Ryan Mallon 2 siblings, 1 reply; 33+ messages in thread From: Jamie Lokier @ 2010-06-23 22:53 UTC (permalink / raw) To: Ryan Mallon Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 22:53 ` Jamie Lokier @ 2010-06-23 23:06 ` Ryan Mallon 2010-06-24 0:04 ` Jamie Lokier 0 siblings, 1 reply; 33+ messages in thread From: Ryan Mallon @ 2010-06-23 23:06 UTC (permalink / raw) To: Jamie Lokier Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 ryan@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-23 23:06 ` Ryan Mallon @ 2010-06-24 0:04 ` Jamie Lokier 2010-06-24 0:10 ` Ryan Mallon 2010-06-24 4:33 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey 0 siblings, 2 replies; 33+ messages in thread From: Jamie Lokier @ 2010-06-24 0:04 UTC (permalink / raw) To: Ryan Mallon Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-24 0:04 ` Jamie Lokier @ 2010-06-24 0:10 ` Ryan Mallon 2010-06-25 7:19 ` David Brownell 2010-06-24 4:33 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey 1 sibling, 1 reply; 33+ messages in thread From: Ryan Mallon @ 2010-06-24 0:10 UTC (permalink / raw) To: Jamie Lokier Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel 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 ryan@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) 2010-06-24 0:10 ` Ryan Mallon @ 2010-06-25 7:19 ` David Brownell 0 siblings, 0 replies; 33+ messages in thread From: David Brownell @ 2010-06-25 7:19 UTC (permalink / raw) To: Jamie Lokier, Ryan Mallon Cc: David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel --- On Wed, 6/23/10, Ryan Mallon <ryan@bluewatersys.com> 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) 2010-06-24 0:04 ` Jamie Lokier 2010-06-24 0:10 ` Ryan Mallon @ 2010-06-24 4:33 ` Jon Povey 1 sibling, 0 replies; 33+ messages in thread From: Jon Povey @ 2010-06-24 4:33 UTC (permalink / raw) To: Jamie Lokier, Ryan Mallon Cc: David Brownell, linux kernel, David Brownell, linux-arm-kernel 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 jon.povey@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-20 21:31 ` Ryan Mallon 2010-06-21 2:40 ` David Brownell @ 2010-06-29 8:29 ` CoffBeta 1 sibling, 0 replies; 33+ messages in thread From: CoffBeta @ 2010-06-29 8:29 UTC (permalink / raw) To: Ryan Mallon Cc: David Brownell, David Brownell, gregkh, linux kernel, ext-jani.1.nikula, Uwe Kleine-König, Andrew Morton, linux-arm-kernel non-sleeping On Mon, Jun 21, 2010 at 05:31, Ryan Mallon <ryan@bluewatersys.com> 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 > ryan@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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-19 6:21 ` David Brownell 2010-06-20 21:31 ` Ryan Mallon @ 2010-06-23 11:53 ` Jani Nikula 2010-06-23 12:40 ` David Brownell 1 sibling, 1 reply; 33+ messages in thread From: Jani Nikula @ 2010-06-23 11:53 UTC (permalink / raw) To: ext David Brownell Cc: Ryan Mallon, linux kernel, linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-23 11:53 ` Jani Nikula @ 2010-06-23 12:40 ` David Brownell 2010-06-23 13:22 ` Jani Nikula 0 siblings, 1 reply; 33+ messages in thread From: David Brownell @ 2010-06-23 12:40 UTC (permalink / raw) To: Jani Nikula Cc: Ryan Mallon, linux kernel, linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König --- On Wed, 6/23/10, Jani Nikula <ext-jani.1.nikula@nokia.com> 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-23 12:40 ` David Brownell @ 2010-06-23 13:22 ` Jani Nikula 2010-06-23 13:39 ` David Brownell 0 siblings, 1 reply; 33+ messages in thread From: Jani Nikula @ 2010-06-23 13:22 UTC (permalink / raw) To: ext David Brownell Cc: Ryan Mallon, linux kernel, linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König On Wed, 23 Jun 2010, ext David Brownell wrote: > --- On Wed, 6/23/10, Jani Nikula <ext-jani.1.nikula@nokia.com> 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); ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: gpiolib and sleeping gpios 2010-06-23 13:22 ` Jani Nikula @ 2010-06-23 13:39 ` David Brownell 0 siblings, 0 replies; 33+ messages in thread From: David Brownell @ 2010-06-23 13:39 UTC (permalink / raw) To: Jani Nikula Cc: Ryan Mallon, linux kernel, linux-arm-kernel, Andrew Morton, David Brownell, gregkh, Uwe Kleine-König --- On Wed, 6/23/10, Jani Nikula <ext-jani.1.nikula@nokia.com> 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)
@ 2010-06-24 10:36 David Brownell
0 siblings, 0 replies; 33+ messages in thread
From: David Brownell @ 2010-06-24 10:36 UTC (permalink / raw)
To: Ryan Mallon, Jon Povey
Cc: David Brownell, gregkh, linux kernel, ext-jani.1.nikula,
Uwe Kleine-König, Andrew Morton, linux-arm-kernel
--- On Wed, 6/23/10, Jon Povey <Jon.Povey@racelogic.co.uk> wrote:
>
> Date: Wednesday, June 23, 2010, 9:46 PM
> 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
>
> gpio_request_cansleep would be the same as current
> gpio_request
I wonder if, by the time I catch up on this
ever-extending email thread
... someone else will have noted that
because gpio_request() can now poke the GPIO
chip, that call might actually need to sleep.
So there'd be a difference between the two
calls: one would *NEED* to be called in a
sleepable thread context, vs. that just being
well advised (e.g. as part of board setup in
arch init code after tasking is working)...
So that couldn't work quite that way.
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2010-06-29 8:29 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-06-17 21:47 gpiolib and sleeping gpios Ryan Mallon 2010-06-18 5:27 ` Uwe Kleine-König 2010-06-18 6:16 ` David Brownell 2010-06-18 22:01 ` Ryan Mallon 2010-06-19 6:21 ` David Brownell 2010-06-20 21:31 ` Ryan Mallon 2010-06-21 2:40 ` David Brownell 2010-06-21 5:09 ` Uwe Kleine-König 2010-06-23 1:59 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) Ryan Mallon 2010-06-23 4:37 ` David Brownell 2010-06-23 4:58 ` Eric Miao 2010-06-23 9:51 ` David Brownell 2010-06-23 5:02 ` Ryan Mallon 2010-06-23 5:26 ` Eric Miao 2010-06-23 9:39 ` David Brownell 2010-06-23 19:12 ` Ryan Mallon 2010-06-24 4:46 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey 2010-06-24 8:20 ` Lars-Peter Clausen 2010-06-24 8:29 ` Jani Nikula 2010-06-24 10:31 ` Lars-Peter Clausen 2010-06-24 6:41 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios) Uwe Kleine-König 2010-06-23 22:53 ` Jamie Lokier 2010-06-23 23:06 ` Ryan Mallon 2010-06-24 0:04 ` Jamie Lokier 2010-06-24 0:10 ` Ryan Mallon 2010-06-25 7:19 ` David Brownell 2010-06-24 4:33 ` [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) Jon Povey 2010-06-29 8:29 ` gpiolib and sleeping gpios CoffBeta 2010-06-23 11:53 ` Jani Nikula 2010-06-23 12:40 ` David Brownell 2010-06-23 13:22 ` Jani Nikula 2010-06-23 13:39 ` David Brownell 2010-06-24 10:36 [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios) David Brownell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).