RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)

RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)

[David Brownell]

--- 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.

Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)

[Lars-Peter Clausen]

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

RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)

[Jani Nikula]

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.

Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)

[Lars-Peter Clausen]

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

RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)

[Jon Povey]

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

RE: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleepinggpios)

[Jon Povey]

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