Prev: [PATCH] Two fixes for my mmc/sd cardreader
Next: Winning Notification
From: Ryan Mallon on 20 Jun 2010 17:40
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(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: David Brownell on 20 Jun 2010 22:50


--- On Sun, 6/20/10, Ryan Mallon <ryan(a)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.


--
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 21 Jun 2010 01:20
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/ |
--
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: Jani Nikula on 23 Jun 2010 08:00

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.
--
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 23 Jun 2010 08:50


--- On Wed, 6/23/10, Jani Nikula <ext-jani.1.nikula(a)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


--
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/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH] Two fixes for my mmc/sd cardreader
Next: Winning Notification