[PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[Jingoo Han]

From: Marko Katic <dromede@gmail.com>

Changing backlight intensity on an Akita (Sharp Zaurus C-1000)
triggers WARN_ON message:

WARNING: at drivers/gpio/gpiolib.c:1672 __gpio_set_value+0x38/0xa4()
Modules linked in:
Backtrace:
[<c000c0ac>] (dump_backtrace+0x0/0x110) from [<c02c8278>] (dump_stack+0x18/0x1c)
 r6:c0158fc8 r5:00000009 r4:00000000 r3:c03d4f70
[<c02c8260>] (dump_stack+0x0/0x1c) from [<c0019194>] (warn_slowpath_common+0x54/0x6c)
[<c0019140>] (warn_slowpath_common+0x0/0x6c) from [<c00191d0>] (warn_slowpath_null+0x24/0x2c)
 r8:c38d5c00 r7:c03f82c0 r6:00000000 r5:000000d0 r4:c384e4fc
r3:00000009
[<c00191ac>] (warn_slowpath_null+0x0/0x2c) from [<c0158fc8>] (__gpio_set_value+0x38/0xa4)
[<c0158f90>] (__gpio_set_value+0x0/0xa4) from [<c0169b4c>] (corgi_bl_set_intensity+0x44/0x74)
 r7:c3933418 r6:c3933400 r5:c392cdf0 r4:0000002f
[<c0169b08>] (corgi_bl_set_intensity+0x0/0x74) from [<c0169c1c>] (corgi_bl_update_status+0x5c/0x64)
 r5:c03d31f0 r4:c3933400
[<c0169bc0>] (corgi_bl_update_status+0x0/0x64) from [<c02c3a68>] (corgi_lcd_probe+0x1a8/0x258)
 r4:c392cdf0 r3:c0169bc0
[<c02c38c0>] (corgi_lcd_probe+0x0/0x258) from [<c01da7a4>] (spi_drv_probe+0x20/0x24)
 r8:00000052 r7:c0192d9c r6:c03da6e8 r5:c38d5c34 r4:c38d5c00
[<c01da784>] (spi_drv_probe+0x0/0x24) from [<c0192c44>] (driver_probe_device+0xb0/0x208)
[<c0192b94>] (driver_probe_device+0x0/0x208) from [<c0192e0c>] (__driver_attach+0x70/0x94)
 r6:c03da6e8 r5:c38d5c34 r4:c38d5c00 r3:00000000
[<c0192d9c>] (__driver_attach+0x0/0x94) from [<c0191268>] (bus_for_each_dev+0x54/0x90)
 r6:c03da6e8 r5:c3827e80 r4:00000000 r3:00000000
[<c0191214>] (bus_for_each_dev+0x0/0x90) from [<c01927a4>] (driver_attach+0x20/0x28)
 r7:00000000 r6:c03e29ec r5:c3932980 r4:c03da6e8
[<c0192784>] (driver_attach+0x0/0x28) from [<c0192340>] (bus_add_driver+0xd4/0x22c)
[<c019226c>] (bus_add_driver+0x0/0x22c) from [<c019335c>] (driver_register+0xa4/0x134)
 r8:00000052 r7:c03ea900 r6:c03c32ac r5:c03bdfc8 r4:c03da6e8
[<c01932b8>] (driver_register+0x0/0x134) from [<c01db7ec>] (spi_register_driver+0x4c/0x60)
[<c01db7a0>] (spi_register_driver+0x0/0x60) from [<c03b3ce0>] (corgi_lcd_driver_init+0x14/0x1c)
[<c03b3ccc>] (corgi_lcd_driver_init+0x0/0x1c) from [<c000868c>] (do_one_initcall+0x9c/0x174)
[<c00085f0>] (do_one_initcall+0x0/0x174) from [<c02c1b94>] (kernel_init+0xf4/0x2a8)
[<c02c1aa0>] (kernel_init+0x0/0x2a8) from [<c0009270>] (ret_from_fork+0x14/0x24)
---[ end trace a863a63f242ee38c ]---

Akita machines have backlight controls hooked to a gpio expander chip,
max7310 using i2c transfers which can sleep.
In this case, pca953x_gpio_set_value() can be called to control
gpio, and pca953x_setup_gpio() sets can_sleep flag. Therefore,
gpio_set_value_cansleep() should be used in order to avoid WARN_ON on
akita machines.

Akita is the only exception in this case since other users of corgi_lcd
access backlight gpio controls through a different gpio expander
which does not set the can_sleep flag.

[jg1.han@samsung.com: used gpio_cansleep()]
Signed-off-by: Marko Katic <dromede@gmail.com>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
Change since v3:
- remove redundant wording from commit message
Change since v2:
- use gpio_cansleep() 

 drivers/video/backlight/corgi_lcd.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/video/backlight/corgi_lcd.c b/drivers/video/backlight/corgi_lcd.c
index e2e1b62..e5168f4 100644
--- a/drivers/video/backlight/corgi_lcd.c
+++ b/drivers/video/backlight/corgi_lcd.c
@@ -408,11 +408,20 @@ static int corgi_bl_set_intensity(struct corgi_lcd *lcd, int intensity)
 	/* Bit 5 via GPIO_BACKLIGHT_CONT */
 	cont = !!(intensity & 0x20) ^ lcd->gpio_backlight_cont_inverted;
 
-	if (gpio_is_valid(lcd->gpio_backlight_cont))
-		gpio_set_value(lcd->gpio_backlight_cont, cont);
+	if (gpio_is_valid(lcd->gpio_backlight_cont)) {
+		if (gpio_cansleep(lcd->gpio_backlight_cont))
+			gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
+		else
+			gpio_set_value(lcd->gpio_backlight_cont, cont);
+	}
 
-	if (gpio_is_valid(lcd->gpio_backlight_on))
-		gpio_set_value(lcd->gpio_backlight_on, intensity);
+	if (gpio_is_valid(lcd->gpio_backlight_on)) {
+		if (gpio_cansleep(lcd->gpio_backlight_on))
+			gpio_set_value_cansleep(lcd->gpio_backlight_on,
+						intensity);
+		else
+			gpio_set_value(lcd->gpio_backlight_on, intensity);
+	}
 
 	if (lcd->kick_battery)
 		lcd->kick_battery();
-- 
1.7.2.5

Re: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[Russell King - ARM Linux]

On Wed, Dec 05, 2012 at 09:59:07AM +0900, Jingoo Han wrote:
> -	if (gpio_is_valid(lcd->gpio_backlight_cont))
> -		gpio_set_value(lcd->gpio_backlight_cont, cont);
> +	if (gpio_is_valid(lcd->gpio_backlight_cont)) {
> +		if (gpio_cansleep(lcd->gpio_backlight_cont))
> +			gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> +		else
> +			gpio_set_value(lcd->gpio_backlight_cont, cont);
> +	}

Why not simply:

+	if (gpio_is_valid(lcd->gpio_backlight_cont))
+		gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);

If you read the gpiolib code and documentation, what you will realise is
that the two calls are identical except for the "might_sleep_if()" in
gpio_set_value_cansleep().  You will also note that gpiolib itself _only_
calls gpio_set_value_cansleep() without checking gpio_cansleep() in
contexts where sleeping is possible.  So if it's good enough for gpiolib,
it should be good enough here.

Re: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[Marko Katić]

On Wed, Dec 5, 2012 at 10:30 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Dec 05, 2012 at 09:59:07AM +0900, Jingoo Han wrote:
>> -     if (gpio_is_valid(lcd->gpio_backlight_cont))
>> -             gpio_set_value(lcd->gpio_backlight_cont, cont);
>> +     if (gpio_is_valid(lcd->gpio_backlight_cont)) {
>> +             if (gpio_cansleep(lcd->gpio_backlight_cont))
>> +                     gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
>> +             else
>> +                     gpio_set_value(lcd->gpio_backlight_cont, cont);
>> +     }
>
> Why not simply:
>
> +       if (gpio_is_valid(lcd->gpio_backlight_cont))
> +               gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);

My first patch did exactly this but there were complains about it's
commit message.

And i just found out that Marek Vasut posted the exact same patch more
than a year ago.

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/046955.html

It was not applied for various reasons.

>
> If you read the gpiolib code and documentation, what you will realise is
> that the two calls are identical except for the "might_sleep_if()" in
> gpio_set_value_cansleep().  You will also note that gpiolib itself _only_
> calls gpio_set_value_cansleep() without checking gpio_cansleep() in
> contexts where sleeping is possible.  So if it's good enough for gpiolib,
> it should be good enough here.

The documentation tells which calls to use when you don't need to sleep
and which calls to use when you might sleep. And here we have a case
where the same call to gpio_set_value might sleep or doesn't have to,
depending on the model.
In this case, i'd rather use gpio_cansleep check as Andrew proposed.

I will also say that the distinction between gpio_set_value and
gpio_set_value_cansleep.
is rather confusing at this point. Is it really necessary to have both ?

Re: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[Russell King - ARM Linux]

On Wed, Dec 05, 2012 at 07:20:00PM +0100, Marko Katić wrote:
> On Wed, Dec 5, 2012 at 10:30 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Wed, Dec 05, 2012 at 09:59:07AM +0900, Jingoo Han wrote:
> >> -     if (gpio_is_valid(lcd->gpio_backlight_cont))
> >> -             gpio_set_value(lcd->gpio_backlight_cont, cont);
> >> +     if (gpio_is_valid(lcd->gpio_backlight_cont)) {
> >> +             if (gpio_cansleep(lcd->gpio_backlight_cont))
> >> +                     gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> >> +             else
> >> +                     gpio_set_value(lcd->gpio_backlight_cont, cont);
> >> +     }
> >
> > Why not simply:
> >
> > +       if (gpio_is_valid(lcd->gpio_backlight_cont))
> > +               gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> 
> My first patch did exactly this but there were complains about it's
> commit message.

So that's a reason to drop the patch?  Err, forgive me for being thick
as a medieval castle wall, but what does complaints about the commit
message have to do with the contents of the patch?  Why can't you just
fix the commit message?

> And i just found out that Marek Vasut posted the exact same patch more
> than a year ago.
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/046955.html
> 
> It was not applied for various reasons.

Looking at that thread (which is corrupted btw, probably thanks to the
crappy python based locking in mailman) - here's a better archiver:

http://lists.arm.linux.org.uk/lurker/thread/20110402.014316.74101499.en.html

it didn't go anywhere because the discussion was distracted by the loss
of David Brownell.

Eric shares my opinion of the _cansleep() mess, but unfortunately it's
what we have and no one's come up with any better solutions to it.  (I
argued from the outset that the gpio_xxx_cansleep() should've been
gpio_xxx() and the non-cansleep() version should be called
gpio_xxx_atomic() so that by default people use the version which _can_
sleep, but have to think about it when they want to manipulate GPIOs in
non-task contexts.)

That's off-topic though.  Using just the _cansleep() version is far
better than messing around with stuff like:

	if (gpio_cansleep(gpio))
		gpio_xxx_cansleep(gpio);
	else
		gpio_xxx(gpio);

> > If you read the gpiolib code and documentation, what you will realise is
> > that the two calls are identical except for the "might_sleep_if()" in
> > gpio_set_value_cansleep().  You will also note that gpiolib itself _only_
> > calls gpio_set_value_cansleep() without checking gpio_cansleep() in
> > contexts where sleeping is possible.  So if it's good enough for gpiolib,
> > it should be good enough here.
> 
> The documentation tells which calls to use when you don't need to sleep
> and which calls to use when you might sleep. And here we have a case
> where the same call to gpio_set_value might sleep or doesn't have to,
> depending on the model.
> In this case, i'd rather use gpio_cansleep check as Andrew proposed.
> 
> I will also say that the distinction between gpio_set_value and
> gpio_set_value_cansleep.
> is rather confusing at this point. Is it really necessary to have both ?

No.  You can call gpio_set_value_cansleep() from task contexts for any
GPIO just fine, but you can't call it from atomic contexts (it will
complain).  It doesn't matter whether the GPIO can sleep or not.

You can call gpio_set_value() from any context without it complaining,
however, gpio_set_value() can't be used with a GPIO which sleeps.

Look, when it comes down to it, in _task_ context, where sleeps are
permissible:

	gpio_set_value(gpio, xxx);
and
	gpio_set_value_cansleep(gpio, xxx);

are exactly the same thing; they will both set the value of a GPIO
output, whether it be an atomic or a sleeping gpio to the requested
value.

The difference between the two becomes important if you're not in task
context, where only the non-_cansleep() versions can be used.  This is
enforced by the _cansleep() versions issuing a WARN_ON() if they're
used in non-task contexts.  And conversely, the non-_cansleep() versions
will warn (as you've found) if you use that call with a GPIO which will
sleep.

There is another solution to this mess:

void __gpio_set_value(unsigned gpio, int value)
{
	struct gpio_chip	*chip;

	chip = gpio_to_chip(gpio);
	/* Should be using gpio_set_value_cansleep() */
-	WARN_ON(chip->can_sleep);
+	might_sleep_if(chip->can_sleep);
	trace_gpio_value(gpio, 0, value);
	if (test_bit(FLAG_OPEN_DRAIN,  &gpio_desc[gpio].flags))
		_gpio_set_open_drain_value(gpio, chip, value);
	else if (test_bit(FLAG_OPEN_SOURCE,  &gpio_desc[gpio].flags))
		_gpio_set_open_source_value(gpio, chip, value);
	else
		chip->set(chip, gpio - chip->base, value);
}
EXPORT_SYMBOL_GPL(__gpio_set_value);

With the above change (and an equivalent change everywhere else), it means
gpio_set_value() is callable from task contexts on GPIOs which can sleep.

However, it loses us some checking - we no longer have the cross-platform
checking that we get with the existing API, and that's why it's undesirable.

As I say above, IMHO it would've been much better to rename these functions
to be the other way around but David was always very dismissive of any
comments I had against any code he'd written.

Re: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[Grant Likely]

On Wed, Dec 5, 2012 at 7:21 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> As I say above, IMHO it would've been much better to rename these functions
> to be the other way around but David was always very dismissive of any
> comments I had against any code he'd written.

FWIW, I'll gladly take a patch to rename them now if someone does the legwork.

g.

Re: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[Jingoo Han]

On Thursday, December 06, 2012 4:22 AM, Russell King - ARM Linux wrote
> On Wed, Dec 05, 2012 at 07:20:00PM +0100, Marko Katić wrote:
> > On Wed, Dec 5, 2012 at 10:30 AM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Wed, Dec 05, 2012 at 09:59:07AM +0900, Jingoo Han wrote:
> > >> -     if (gpio_is_valid(lcd->gpio_backlight_cont))
> > >> -             gpio_set_value(lcd->gpio_backlight_cont, cont);
> > >> +     if (gpio_is_valid(lcd->gpio_backlight_cont)) {
> > >> +             if (gpio_cansleep(lcd->gpio_backlight_cont))
> > >> +                     gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> > >> +             else
> > >> +                     gpio_set_value(lcd->gpio_backlight_cont, cont);
> > >> +     }
> > >
> > > Why not simply:
> > >
> > > +       if (gpio_is_valid(lcd->gpio_backlight_cont))
> > > +               gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> >
> > My first patch did exactly this but there were complains about it's
> > commit message.
> 
> So that's a reason to drop the patch?  Err, forgive me for being thick
> as a medieval castle wall, but what does complaints about the commit
> message have to do with the contents of the patch?  Why can't you just
> fix the commit message?
> 
> > And i just found out that Marek Vasut posted the exact same patch more
> > than a year ago.
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/046955.html
> >
> > It was not applied for various reasons.
> 
> Looking at that thread (which is corrupted btw, probably thanks to the
> crappy python based locking in mailman) - here's a better archiver:
> 
> http://lists.arm.linux.org.uk/lurker/thread/20110402.014316.74101499.en.html
> 
> it didn't go anywhere because the discussion was distracted by the loss
> of David Brownell.
> 
> Eric shares my opinion of the _cansleep() mess, but unfortunately it's
> what we have and no one's come up with any better solutions to it.  (I
> argued from the outset that the gpio_xxx_cansleep() should've been
> gpio_xxx() and the non-cansleep() version should be called
> gpio_xxx_atomic() so that by default people use the version which _can_
> sleep, but have to think about it when they want to manipulate GPIOs in
> non-task contexts.)

Hi Russell,

Thank you for your explanation. It is very helpful for getting hold of.
I have been confused by the current function name such as gpio_xxx_cansleep().
As you mentioned, gpio_xxx_cansleep()and gpio_xxx_atomic() would be better.

> 
> That's off-topic though.  Using just the _cansleep() version is far
> better than messing around with stuff like:
> 
> 	if (gpio_cansleep(gpio))
> 		gpio_xxx_cansleep(gpio);
> 	else
> 		gpio_xxx(gpio);
> 
> > > If you read the gpiolib code and documentation, what you will realise is
> > > that the two calls are identical except for the "might_sleep_if()" in
> > > gpio_set_value_cansleep().  You will also note that gpiolib itself _only_
> > > calls gpio_set_value_cansleep() without checking gpio_cansleep() in
> > > contexts where sleeping is possible.  So if it's good enough for gpiolib,
> > > it should be good enough here.
> >
> > The documentation tells which calls to use when you don't need to sleep
> > and which calls to use when you might sleep. And here we have a case
> > where the same call to gpio_set_value might sleep or doesn't have to,
> > depending on the model.
> > In this case, i'd rather use gpio_cansleep check as Andrew proposed.
> >
> > I will also say that the distinction between gpio_set_value and
> > gpio_set_value_cansleep.
> > is rather confusing at this point. Is it really necessary to have both ?
> 
> No.  You can call gpio_set_value_cansleep() from task contexts for any
> GPIO just fine, but you can't call it from atomic contexts (it will
> complain).  It doesn't matter whether the GPIO can sleep or not.
> 
> You can call gpio_set_value() from any context without it complaining,
> however, gpio_set_value() can't be used with a GPIO which sleeps.
> 
> Look, when it comes down to it, in _task_ context, where sleeps are
> permissible:
> 
> 	gpio_set_value(gpio, xxx);
> and
> 	gpio_set_value_cansleep(gpio, xxx);
> 
> are exactly the same thing; they will both set the value of a GPIO
> output, whether it be an atomic or a sleeping gpio to the requested
> value.
> 
> The difference between the two becomes important if you're not in task
> context, where only the non-_cansleep() versions can be used.  This is
> enforced by the _cansleep() versions issuing a WARN_ON() if they're
> used in non-task contexts.  And conversely, the non-_cansleep() versions
> will warn (as you've found) if you use that call with a GPIO which will
> sleep.

The former one, the _cansleep() versions issuing a WARN_ON(), would be
better than the latter one, current scheme. 

Best regards,
Jingoo Han

> 
> There is another solution to this mess:
> 
> void __gpio_set_value(unsigned gpio, int value)
> {
> 	struct gpio_chip	*chip;
> 
> 	chip = gpio_to_chip(gpio);
> 	/* Should be using gpio_set_value_cansleep() */
> -	WARN_ON(chip->can_sleep);
> +	might_sleep_if(chip->can_sleep);
> 	trace_gpio_value(gpio, 0, value);
> 	if (test_bit(FLAG_OPEN_DRAIN,  &gpio_desc[gpio].flags))
> 		_gpio_set_open_drain_value(gpio, chip, value);
> 	else if (test_bit(FLAG_OPEN_SOURCE,  &gpio_desc[gpio].flags))
> 		_gpio_set_open_source_value(gpio, chip, value);
> 	else
> 		chip->set(chip, gpio - chip->base, value);
> }
> EXPORT_SYMBOL_GPL(__gpio_set_value);
> 
> With the above change (and an equivalent change everywhere else), it means
> gpio_set_value() is callable from task contexts on GPIOs which can sleep.
> 
> However, it loses us some checking - we no longer have the cross-platform
> checking that we get with the existing API, and that's why it's undesirable.
> 
> As I say above, IMHO it would've been much better to rename these functions
> to be the other way around but David was always very dismissive of any
> comments I had against any code he'd written.

Re: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

[Jingoo Han]

On Monday, December 10, 2012 5:18 PM, Jingoo Han wrote
> On Thursday, December 06, 2012 4:22 AM, Russell King - ARM Linux wrote
> > On Wed, Dec 05, 2012 at 07:20:00PM +0100, Marko Katić wrote:
> > > On Wed, Dec 5, 2012 at 10:30 AM, Russell King - ARM Linux
> > > <linux@arm.linux.org.uk> wrote:
> > > > On Wed, Dec 05, 2012 at 09:59:07AM +0900, Jingoo Han wrote:
...
> > Eric shares my opinion of the _cansleep() mess, but unfortunately it's
> > what we have and no one's come up with any better solutions to it.  (I
> > argued from the outset that the gpio_xxx_cansleep() should've been
> > gpio_xxx() and the non-cansleep() version should be called
> > gpio_xxx_atomic() so that by default people use the version which _can_
> > sleep, but have to think about it when they want to manipulate GPIOs in
> > non-task contexts.)
> 
> Hi Russell,
> 
> Thank you for your explanation. It is very helpful for getting hold of.
> I have been confused by the current function name such as gpio_xxx_cansleep().
> As you mentioned, gpio_xxx_cansleep()and gpio_xxx_atomic() would be better.

Oh, sorry. There is a mistake.

It should be as below:
'gpio_xxx()and gpio_xxx_atomic() would be better'.


Best regards,
Jingoo Han