Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.

[Trent Piepho]

On Thu, Sep 8, 2011 at 9:07 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 8 Sep 2011 20:54:55 -0700 Trent Piepho <xyzzy@xxxxxxxxxxxxx> wrote:
>> On Thu, Sep 8, 2011 at 6:35 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>> > gpio_get_value() is an architecture-specific function whereas
>> > gpio_get_value_cansleep() is not.  Hence all architectures will now be
>> > forced to use the same code.  Why is this OK?
>
>> The non-cansleep version is only supposed to be different from
>>  gpio_get_value() (which is virtually the same code) in that it can
>> inline a fast gpio operation.  So calling cansleep vs the non-cansleep
>> shouldn't result in any change that would break anything.  If it did
>> it would be flaw in that architecture's version of gpio_get_value().
>> It should just mean a call that could be inlined won't be.
>>
>> I suppose one could ask if gpio_get_value_cansleep() needs to exist.
>
> Here's the unicore gpio_get_value():
>
> : static inline int gpio_get_value(unsigned gpio)
> : {
> :       if (__builtin_constant_p(gpio) && (gpio <= GPIO_MAX))
> :               return readl(GPIO_GPLR) & GPIO_GPIO(gpio);
> :       else
> :               return __gpio_get_value(gpio);
> : }
>
> And here's gpio_get_value_cansleep()
>
> int gpio_get_value_cansleep(unsigned gpio)
> {
>        struct gpio_chip        *chip;
>        int value;
>
>        might_sleep_if(extra_checks);
>        chip = gpio_to_chip(gpio);
>        value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
>        trace_gpio_value(gpio, 1, value);
>        return value;
> }
>
> They're very different.  Why is it OK to replace one with the other??

What's supposed to happen is chip->get() will be a method that does
"readl(GPIO_GPLR) & GPIO_GPIO(gpio);" or whatever the inlined bit in
gpio_get_value() is.  So calling gpio_get_value_cansleep() should
still get the correct value for the gpio.  It just won't be an inlined
register read anymore.

For instance, all the arch versions that use builtin_constant_p() will
not take the inline path, since the gpio number if obviously not a
constant when gpio_get_value() is called in this leds function.  So
they inline into a call to __gpio_get_value().  Which as you've
pointed out is nearly exactly the same as gpio_get_value_cansleep().
The only change is debugging related, that of the might_sleep_if() to
a WARN_ON().

One could have:
static inline int __gpio_get_value(gpio) { return
_gpio_get_value(gpio, GFP_ATOMIC); }
static inline int gpio_get_value_cansleep(gpio) { return
_gpio_get_value(gpio, GFP_KERNEL); }

Then _gpio_get_value(gpio, context) would be the current code that's
common to both __gpio_get_value() and gpio_get_value_cansleep(),
except it uses context solely to spit a warning if the gpio can't be
done from the requested context or if the context isn't allowable from
whence the call was made.

And then one would ask if all this complexity in the interface is
really the best way to throw warnings about sleeping in atomic
context?  Because if there was a better way to do that, we could just
s/(gpio_get_value)_cansleep/\1/g and not worry about these someone
did/didn't call cansleep() when they should done the opposite patches.

>> > Asides:
>> >
>> > The duplication of code between  gpio_get_value() and
>> > gpio_get_value_cansleep() is daft.
>> >
>> > The comment over gpio_get_value_cansleep() sucks mud rocks.
>
> Preserving this...

Well, inlining code that sits on a waitq doesn't make a lot of sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/