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 sleeping gpios)

[Jamie Lokier]

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

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

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

[Ryan Mallon]

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

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)

[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)

[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]

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