Prev: [PATCH] Two fixes for my mmc/sd cardreader
Next: Winning Notification
From: Ryan Mallon on 17 Jun 2010 17:50
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(a)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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Uwe Kleine-König on 18 Jun 2010 01:30
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/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Brownell on 18 Jun 2010 02:20


--- On Thu, 6/17/10, Ryan Mallon <ryan(a)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(a)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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ryan Mallon on 18 Jun 2010 18:10
David Brownell wrote:
>
> --- On Thu, 6/17/10, Ryan Mallon <ryan(a)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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Brownell on 19 Jun 2010 02:30

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH] Two fixes for my mmc/sd cardreader
Next: Winning Notification