* [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
@ 2011-08-30 23:39 David Daney
2011-09-09 1:35 ` Andrew Morton
0 siblings, 1 reply; 9+ messages in thread
From: David Daney @ 2011-08-30 23:39 UTC (permalink / raw)
To: linux-kernel; +Cc: David Daney, Richard Purdie, Trent Piepho, Grant Likely
I get the following warning:
------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:1559 __gpio_get_value+0x90/0x98()
Modules linked in:
Call Trace:
[<ffffffff81440950>] dump_stack+0x8/0x34
[<ffffffff81141478>] warn_slowpath_common+0x78/0xa0
[<ffffffff812f0958>] __gpio_get_value+0x90/0x98
[<ffffffff81434f04>] create_gpio_led+0xdc/0x194
[<ffffffff8143524c>] gpio_led_probe+0x290/0x36c
[<ffffffff8130e8b0>] driver_probe_device+0x78/0x1b0
[<ffffffff8130eaa8>] __driver_attach+0xc0/0xc8
[<ffffffff8130d7ac>] bus_for_each_dev+0x64/0xb0
[<ffffffff8130e130>] bus_add_driver+0x1c8/0x2a8
[<ffffffff8130f100>] driver_register+0x90/0x180
[<ffffffff81100438>] do_one_initcall+0x38/0x160
---[ end trace ee38723fbefcd65c ]---
My GPIOs are on an I2C port expander, so we must use the *_cansleep()
variant of the GPIO functions. This is was not being done in
create_gpio_led().
We can change gpio_get_value() to gpio_get_value_cansleep() because it
is only called from the platform_driver probe function, which is a
context where we can sleep.
Only tested on my gpio_cansleep() system, but it seems safe for all
systems.
Signed-off-by: David Daney <david.daney@cavium.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Trent Piepho <xyzzy@speakeasy.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
drivers/leds/leds-gpio.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 3d8bc32..504cc26 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -121,7 +121,7 @@ static int __devinit create_gpio_led(const struct gpio_led *template,
}
led_dat->cdev.brightness_set = gpio_led_set;
if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP)
- state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
+ state = !!gpio_get_value_cansleep(led_dat->gpio) ^ led_dat->active_low;
else
state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-08-30 23:39 [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing David Daney
@ 2011-09-09 1:35 ` Andrew Morton
2011-09-09 3:54 ` Trent Piepho
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2011-09-09 1:35 UTC (permalink / raw)
To: David Daney; +Cc: linux-kernel, Richard Purdie, Trent Piepho, Grant Likely
On Tue, 30 Aug 2011 16:39:52 -0700 David Daney <david.daney@cavium.com> wrote:
> I get the following warning:
>
> ------------[ cut here ]------------
> WARNING: at drivers/gpio/gpiolib.c:1559 __gpio_get_value+0x90/0x98()
> Modules linked in:
> Call Trace:
> [<ffffffff81440950>] dump_stack+0x8/0x34
> [<ffffffff81141478>] warn_slowpath_common+0x78/0xa0
> [<ffffffff812f0958>] __gpio_get_value+0x90/0x98
> [<ffffffff81434f04>] create_gpio_led+0xdc/0x194
> [<ffffffff8143524c>] gpio_led_probe+0x290/0x36c
> [<ffffffff8130e8b0>] driver_probe_device+0x78/0x1b0
> [<ffffffff8130eaa8>] __driver_attach+0xc0/0xc8
> [<ffffffff8130d7ac>] bus_for_each_dev+0x64/0xb0
> [<ffffffff8130e130>] bus_add_driver+0x1c8/0x2a8
> [<ffffffff8130f100>] driver_register+0x90/0x180
> [<ffffffff81100438>] do_one_initcall+0x38/0x160
>
> ---[ end trace ee38723fbefcd65c ]---
>
> My GPIOs are on an I2C port expander, so we must use the *_cansleep()
> variant of the GPIO functions. This is was not being done in
> create_gpio_led().
>
> We can change gpio_get_value() to gpio_get_value_cansleep() because it
> is only called from the platform_driver probe function, which is a
> context where we can sleep.
>
> Only tested on my gpio_cansleep() system, but it seems safe for all
> systems.
>
> ...
>
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -121,7 +121,7 @@ static int __devinit create_gpio_led(const struct gpio_led *template,
> }
> led_dat->cdev.brightness_set = gpio_led_set;
> if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP)
> - state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
> + state = !!gpio_get_value_cansleep(led_dat->gpio) ^ led_dat->active_low;
> else
> state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
> led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
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?
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-09-09 1:35 ` Andrew Morton
@ 2011-09-09 3:54 ` Trent Piepho
2011-09-09 4:07 ` Andrew Morton
0 siblings, 1 reply; 9+ messages in thread
From: Trent Piepho @ 2011-09-09 3:54 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Daney, linux-kernel, Richard Purdie, Grant Likely
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.
On Thu, Sep 8, 2011 at 6:35 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 30 Aug 2011 16:39:52 -0700 David Daney <david.daney@cavium.com> wrote:
>
> > I get the following warning:
> >
> > ------------[ cut here ]------------
> > WARNING: at drivers/gpio/gpiolib.c:1559 __gpio_get_value+0x90/0x98()
> > Modules linked in:
> > Call Trace:
> > [<ffffffff81440950>] dump_stack+0x8/0x34
> > [<ffffffff81141478>] warn_slowpath_common+0x78/0xa0
> > [<ffffffff812f0958>] __gpio_get_value+0x90/0x98
> > [<ffffffff81434f04>] create_gpio_led+0xdc/0x194
> > [<ffffffff8143524c>] gpio_led_probe+0x290/0x36c
> > [<ffffffff8130e8b0>] driver_probe_device+0x78/0x1b0
> > [<ffffffff8130eaa8>] __driver_attach+0xc0/0xc8
> > [<ffffffff8130d7ac>] bus_for_each_dev+0x64/0xb0
> > [<ffffffff8130e130>] bus_add_driver+0x1c8/0x2a8
> > [<ffffffff8130f100>] driver_register+0x90/0x180
> > [<ffffffff81100438>] do_one_initcall+0x38/0x160
> >
> > ---[ end trace ee38723fbefcd65c ]---
> >
> > My GPIOs are on an I2C port expander, so we must use the *_cansleep()
> > variant of the GPIO functions. This is was not being done in
> > create_gpio_led().
> >
> > We can change gpio_get_value() to gpio_get_value_cansleep() because it
> > is only called from the platform_driver probe function, which is a
> > context where we can sleep.
> >
> > Only tested on my gpio_cansleep() system, but it seems safe for all
> > systems.
> >
> > ...
> >
> > --- a/drivers/leds/leds-gpio.c
> > +++ b/drivers/leds/leds-gpio.c
> > @@ -121,7 +121,7 @@ static int __devinit create_gpio_led(const struct gpio_led *template,
> > }
> > led_dat->cdev.brightness_set = gpio_led_set;
> > if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP)
> > - state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
> > + state = !!gpio_get_value_cansleep(led_dat->gpio) ^ led_dat->active_low;
> > else
> > state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
> > led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
>
> 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?
>
> 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.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-09-09 3:54 ` Trent Piepho
@ 2011-09-09 4:07 ` Andrew Morton
2011-09-09 5:30 ` Trent Piepho
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2011-09-09 4:07 UTC (permalink / raw)
To: Trent Piepho; +Cc: David Daney, linux-kernel, Richard Purdie, Grant Likely
On Thu, 8 Sep 2011 20:54:55 -0700 Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Thu, Sep 8, 2011 at 6:35 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 30 Aug 2011 16:39:52 -0700 David Daney <david.daney@cavium.com> wrote:
> >
> > > I get the following warning:
> > >
> > > ------------[ cut here ]------------
> > > WARNING: at drivers/gpio/gpiolib.c:1559 gpio_get_value+0x90/0x98()
> > > Modules linked in:
> > > Call Trace:
> > > [<ffffffff81440950>] dump_stack+0x8/0x34
> > > [<ffffffff81141478>] warn_slowpath_common+0x78/0xa0
> > > [<ffffffff812f0958>] gpio_get_value+0x90/0x98
> > > [<ffffffff81434f04>] create_gpio_led+0xdc/0x194
> > > [<ffffffff8143524c>] gpio_led_probe+0x290/0x36c
> > > [<ffffffff8130e8b0>] driver_probe_device+0x78/0x1b0
> > > [<ffffffff8130eaa8>] driver_attach+0xc0/0xc8
> > > [<ffffffff8130d7ac>] bus_for_each_dev+0x64/0xb0
> > > [<ffffffff8130e130>] bus_add_driver+0x1c8/0x2a8
> > > [<ffffffff8130f100>] driver_register+0x90/0x180
> > > [<ffffffff81100438>] do_one_initcall+0x38/0x160
> > >
> > > ---[ end trace ee38723fbefcd65c ]---
> > >
> > > My GPIOs are on an I2C port expander, so we must use the *_cansleep()
> > > variant of the GPIO functions. This is was not being done in
> > > create_gpio_led().
> > >
> > > We can change gpio_get_value() to gpio_get_value_cansleep() because it
> > > is only called from the platform_driver probe function, which is a
> > > context where we can sleep.
> > >
> > > Only tested on my gpio_cansleep() system, but it seems safe for all
> > > systems.
> > >
> > > ...
> > >
> > > --- a/drivers/leds/leds-gpio.c
> > > +++ b/drivers/leds/leds-gpio.c
> > > @@ -121,7 +121,7 @@ static int devinit create_gpio_led(const struct gpio_led *template,
> > > }
> > > led_dat->cdev.brightness_set = gpio_led_set;
> > > if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP)
> > > - state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
> > > + state = !!gpio_get_value_cansleep(led_dat->gpio) ^ led_dat->active_low;
> > > else
> > > state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
> > > led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
> >
> > 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?
(top-posting repaired. Please don't do that)
> 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);
: }
blackfin:
: static inline int gpio_get_value(unsigned int gpio)
: {
: if (gpio < MAX_BLACKFIN_GPIOS)
: return bfin_gpio_get_value(gpio);
: else
: return __gpio_get_value(gpio);
: }
m68k:
: static inline int gpio_get_value(unsigned gpio)
: {
: if (__builtin_constant_p(gpio) && gpio < MCFGPIO_PIN_MAX)
: return mcfgpio_read(__mcf_gpio_ppdr(gpio)) & mcfgpio_bit(gpio);
: else
: return __gpio_get_value(gpio);
: }
:
etcetera.
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??
> > 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...
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-09-09 4:07 ` Andrew Morton
@ 2011-09-09 5:30 ` Trent Piepho
2011-09-09 5:44 ` Andrew Morton
0 siblings, 1 reply; 9+ messages in thread
From: Trent Piepho @ 2011-09-09 5:30 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Daney, linux-kernel, Richard Purdie, Grant Likely
On Thu, Sep 8, 2011 at 9:07 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 8 Sep 2011 20:54:55 -0700 Trent Piepho <xyzzy@speakeasy.org> wrote:
>> On Thu, Sep 8, 2011 at 6:35 PM, Andrew Morton <akpm@linux-foundation.org> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-09-09 5:30 ` Trent Piepho
@ 2011-09-09 5:44 ` Andrew Morton
2011-09-09 16:15 ` David Daney
2011-09-09 17:41 ` Trent Piepho
0 siblings, 2 replies; 9+ messages in thread
From: Andrew Morton @ 2011-09-09 5:44 UTC (permalink / raw)
To: Trent Piepho; +Cc: David Daney, linux-kernel, Richard Purdie, Grant Likely
On Thu, 8 Sep 2011 22:30:58 -0700 Trent Piepho <tpiepho@gmail.com> wrote:
> >
> > 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.
Well, that may be the case with the current in-tree implementations (I
didn't check), but from a design point of view the core code shouldn't
"know" how the architecture is implementing gpio_get_value().
> 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.
Yes, it sounds like the interface around there needs a rethink.
I don't actually know why gpio_get_value_cansleep() exists. I looked
at the silly comment and for some reason didn't feel like working this
out.
> >> > 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.
No need for inlining - just a function call. One which the compiler
might turn into a tailcall for us.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-09-09 5:44 ` Andrew Morton
@ 2011-09-09 16:15 ` David Daney
2011-09-09 17:48 ` Andrew Morton
2011-09-09 17:41 ` Trent Piepho
1 sibling, 1 reply; 9+ messages in thread
From: David Daney @ 2011-09-09 16:15 UTC (permalink / raw)
To: Andrew Morton; +Cc: Trent Piepho, linux-kernel, Richard Purdie, Grant Likely
On 09/08/2011 10:44 PM, Andrew Morton wrote:
> On Thu, 8 Sep 2011 22:30:58 -0700 Trent Piepho<tpiepho@gmail.com> wrote:
>
>>>
>>> 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.
>
> Well, that may be the case with the current in-tree implementations (I
> didn't check), but from a design point of view the core code shouldn't
> "know" how the architecture is implementing gpio_get_value().
Really there are two separate issues here:
1) Should the patch be applied?
2) Is there room to improve the libgpio API?
It is unclear to me if these are currently being conflated.
In any event, from a purely selfish point of view, I would like to see
the patch applied as I cannot boot my boards with out it. As for
improving the GPIO APIs, it seems slightly less urgent, but also a good
idea.
Thanks,
David Daney
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-09-09 16:15 ` David Daney
@ 2011-09-09 17:48 ` Andrew Morton
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Morton @ 2011-09-09 17:48 UTC (permalink / raw)
To: David Daney; +Cc: Trent Piepho, linux-kernel, Richard Purdie, Grant Likely
On Fri, 09 Sep 2011 09:15:58 -0700 David Daney <david.daney@cavium.com> wrote:
> On 09/08/2011 10:44 PM, Andrew Morton wrote:
> > On Thu, 8 Sep 2011 22:30:58 -0700 Trent Piepho<tpiepho@gmail.com> wrote:
> >
> >>>
> >>> 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.
> >
> > Well, that may be the case with the current in-tree implementations (I
> > didn't check), but from a design point of view the core code shouldn't
> > "know" how the architecture is implementing gpio_get_value().
>
> Really there are two separate issues here:
>
> 1) Should the patch be applied?
>
> 2) Is there room to improve the libgpio API?
>
> It is unclear to me if these are currently being conflated.
>
> In any event, from a purely selfish point of view, I would like to see
> the patch applied as I cannot boot my boards with out it. As for
> improving the GPIO APIs, it seems slightly less urgent, but also a good
> idea.
Yeah. One option is to apply it as a stopgap and promise ourselves
that we'll fix things for real later. I'm waiting for Grant to pop up
and tell us when he'll be fixing his junk ;)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing.
2011-09-09 5:44 ` Andrew Morton
2011-09-09 16:15 ` David Daney
@ 2011-09-09 17:41 ` Trent Piepho
1 sibling, 0 replies; 9+ messages in thread
From: Trent Piepho @ 2011-09-09 17:41 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Daney, linux-kernel, Richard Purdie, Grant Likely
On Fri, Sep 9, 2011 at 1:44 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 8 Sep 2011 22:30:58 -0700 Trent Piepho <tpiepho@gmail.com> wrote:
>> >
>> > 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.
>
> Well, that may be the case with the current in-tree implementations (I
> didn't check), but from a design point of view the core code shouldn't
> "know" how the architecture is implementing gpio_get_value().
The architectures aren't free to make gpio_get_value() do whatever
they want. They're supposed to provide one that follows certain
semantics. And those semantics mean we can replace any
gpio_get_value() call with __gpio_get_value() or
gpio_get_value_cansleep(), depending on the context the call is made
from. If we can't, then the architecture's gpio_get_value() is
broken.
>> 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.
>
> Yes, it sounds like the interface around there needs a rethink.
The idea was that someone would write code that used a gpio from an
irq handler or while holding a spinlock. Which is fine. And their
gpio would be one that doesn't need to sleep so there isn't a problem.
But then someone else uses that code with a different gpio driver and
this gpio sleeps, because it's on an I2C chip for instance. So now
you get a hang. Sure would be nice if the kernel would tell us, "Hey,
you can't use that gpio with this code!" instead of just randomly
hanging. Which is the warning David got. Except the warning was
false, since we weren't in atomic context.
So there is probably a better way to do that than have two different
functions for getting a gpio value. Maybe one that doesn't generate
false warnings like this.
So I'd purpose getting rid of gpio_get_value_cansleep(). Instead put
a check in __gpio_get_value() to pop up a warning if it's called from
atomic context on a gpio that can sleep.
> I don't actually know why gpio_get_value_cansleep() exists. I looked
> at the silly comment and for some reason didn't feel like working this
> out.
I think the logic for gpio_get_value_cansleep()'s comment is flawed.
We have, "It makes sense to inline gpio code that's a single register
read, but doesn't make sense to inline code that will sit on a waitq."
That's true. Then we have, "The former gpio code can be called from
atomic context while the latter can't." That's true too. But then
the comment for gpio_get_value_cansleep() seems to have combined those
two into, "It doesn't make sense to inline code that is called from
non-atomic context." Which doesn't follow. What it should be is, "It
doesn't make sense to inline code that can't be called from atomic
context."
But the rule seems to be that one should use gpio_get_value_cansleep()
if you are in non-atomic context, which is following the flawed logic
that in non-atomic context you don't care about making sure fast gpios
stay fast.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-09-09 17:43 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-30 23:39 [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() when initializing David Daney
2011-09-09 1:35 ` Andrew Morton
2011-09-09 3:54 ` Trent Piepho
2011-09-09 4:07 ` Andrew Morton
2011-09-09 5:30 ` Trent Piepho
2011-09-09 5:44 ` Andrew Morton
2011-09-09 16:15 ` David Daney
2011-09-09 17:48 ` Andrew Morton
2011-09-09 17:41 ` Trent Piepho
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.