mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH] leds: leds-gpio: reserve gpio before using it
@ 2013-05-20  0:39 Jingoo Han
  2013-05-20 18:15 ` Bryan Wu
  0 siblings, 1 reply; 3+ messages in thread
From: Jingoo Han @ 2013-05-20  0:39 UTC (permalink / raw)
  To: Timo Ter?s, Bryan Wu, Richard Purdie, linux-leds, linux-kernel
  Cc: Timo Ter?s, Jingoo Han, Sachin Kamat, Raphael Assenat,
	Trent Piepho, Javier Martinez Canillas, Arnaud Patard,
	Ezequiel Garcia

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 4940 bytes --]

Friday, May 17, 2013 4:49 PM, Timo Teras wrote:
> 
> This reverts commit a99d76f (leds: leds-gpio: use gpio_request_one)
> and commit 2d7c22f (leds: leds-gpio: set devm_gpio_request_one()
> flags param correctly) which was a fix of the first one.
> 
> The conversion to devm_gpio_request in commit e3b1d44c (leds:
> leds-gpio: use devm_gpio_request_one) is not reverted.
> 
> The problem is that gpio_cansleep() and gpio_get_value_cansleep()
> calls can crash if the gpio is not first reserved. Incidentally this
> same bug existed earlier and was fixed similarly in commit d95cbe61
> (leds: Fix potential leds-gpio oops). But the OOPS is real. It happens
> when GPIOs are provided by module which is not yet loaded.
> 
> So this fixes the following BUG during my ALIX boot (3.9.2-vanilla):
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000004c
> IP: [<c11287d6>] __gpio_cansleep+0xe/0x1a
> *pde = 00000000
> Oops: 0000 [#1] SMP
> Modules linked in: leds_gpio(+) via_rhine mii cs5535_mfd mfd_core
> geode_rng rng_core geode_aes isofs nls_utf8 nls_cp437 vfat fat
> ata_generic pata_amd pata_cs5536 pata_acpi libata ehci_pci ehci_hcd
> ohci_hcd usb_storage usbcore usb_common sd_mod scsi_mod squashfs loop
> Pid: 881, comm: modprobe Not tainted 3.9.2 #1-Alpine
> EIP: 0060:[<c11287d6>] EFLAGS: 00010282 CPU: 0
> EIP is at __gpio_cansleep+0xe/0x1a
> EAX: 00000000 EBX: cf364018 ECX: c132b8b9 EDX: 00000000
> ESI: c13993a4 EDI: c1399370 EBP: cded9dbc ESP: cded9dbc
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> CR0: 8005003b CR2: 0000004c CR3: 0f0c4000 CR4: 00000090
> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> DR6: ffff0ff0 DR7: 00000400
> Process modprobe (pid: 881, ti=cded8000 task=cf094aa0 task.ti=cded8000)
> Stack:
>  cded9de0 d09471cb 00000000 c1399260 cf364014 00000000 c1399260 c1399254
>  d0949014 cded9df4 c118cd59 c1399260 d0949014 d0949014 cded9e08 c118ba47
>  c1399260 d0949014 c1399294 cded9e1c c118bb75 cded9e24 d0949014 00000000
> Call Trace:
>  [<d09471cb>] gpio_led_probe+0xba/0x203 [leds_gpio]
>  [<c118cd59>] platform_drv_probe+0x26/0x48
>  [<c118ba47>] driver_probe_device+0x75/0x15c
>  [<c118bb75>] __driver_attach+0x47/0x63
>  [<c118a727>] bus_for_each_dev+0x3c/0x66
>  [<c118b6f9>] driver_attach+0x14/0x16
>  [<c118bb2e>] ? driver_probe_device+0x15c/0x15c
>  [<c118b3d5>] bus_add_driver+0xbd/0x1bc
>  [<d08b4000>] ? 0xd08b3fff
>  [<d08b4000>] ? 0xd08b3fff
>  [<c118bffc>] driver_register+0x74/0xec
>  [<d08b4000>] ? 0xd08b3fff
>  [<c118c8e8>] platform_driver_register+0x38/0x3a
>  [<d08b400d>] gpio_led_driver_init+0xd/0x1000 [leds_gpio]
>  [<c100116c>] do_one_initcall+0x6b/0x10f
>  [<d08b4000>] ? 0xd08b3fff
>  [<c105e918>] load_module+0x1631/0x1907
>  [<c10975d6>] ? insert_vmalloc_vmlist+0x14/0x43
>  [<c1098d5b>] ? __vmalloc_node_range+0x13e/0x15f
>  [<c105ec50>] sys_init_module+0x62/0x77
>  [<c1257888>] syscall_call+0x7/0xb
> EIP: [<c11287d6>] __gpio_cansleep+0xe/0x1a SS:ESP 0068:cded9dbc
> CR2: 000000000000004c
>  ---[ end trace 5308fb20d2514822 ]---
> 
> Signed-off-by: Timo Ter?s <timo.teras@iki.f>
> Cc: Jingoo Han <jg1.han@samsung.com>

Acked-by: Jingoo Han <jg1.han@samsung.com>

I look at what happens in leds-gpio.
As you mentioned, devm_gpio_request() should be called before
gpio_cansleep() and gpio_get_value_cansleep are called.
Thank you for sending the patch.

Best regards,
Jingoo Han

> Cc: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Raphael Assenat <raph@8d.com>
> Cc: Trent Piepho <tpiepho@freescale.com>
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Arnaud Patard <arnaud.patard@rtp-net.org>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/leds/leds-gpio.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index a0d931b..b02b679 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -107,6 +107,10 @@ static int create_gpio_led(const struct gpio_led *template,
>  		return 0;
>  	}
> 
> +	ret = devm_gpio_request(parent, template->gpio, template->name);
> +	if (ret < 0)
> +		return ret;
> +
>  	led_dat->cdev.name = template->name;
>  	led_dat->cdev.default_trigger = template->default_trigger;
>  	led_dat->gpio = template->gpio;
> @@ -126,10 +130,7 @@ static int create_gpio_led(const struct gpio_led *template,
>  	if (!template->retain_state_suspended)
>  		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
> 
> -	ret = devm_gpio_request_one(parent, template->gpio,
> -				    (led_dat->active_low ^ state) ?
> -				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> -				    template->name);
> +	ret = gpio_direction_output(led_dat->gpio, led_dat->active_low ^ state);
>  	if (ret < 0)
>  		return ret;
> 
> --
> 1.8.2.3
> 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [Regression] Boot failure on ALIX during LED probe
@ 2013-05-15  7:24 Timo Teras
  2013-05-17  7:48 ` [PATCH] leds: leds-gpio: reserve gpio before using it Timo Teräs
  0 siblings, 1 reply; 3+ messages in thread
From: Timo Teras @ 2013-05-15  7:24 UTC (permalink / raw)
  To: Grant Likely, Linus Walleij, Alexandre Courbot, linux-kernel, linux-leds

I got the below with Linux 3.8.7-grsec and 3.9.2-grsec. Where as
3.6.11-grsec is a known good. I believe the same would happen on on
vanilla kernels too.

[ 15.709955] general protection fault: 0000 [#1] SMP
[ 15.712287] Modules linked in: leds_gpio(+) via_rhine mii cs5535_mfd mfd_core geode_rng rng_core geode_aes isofs nls_utf8 nls_cp437 vfat fat pata_acpi pata_cs5536 pata_amd ata_generic libata ehci_pci ehci_hcd ohci_hcd usbcore usb_common sd_mod scsi_mod squashfs loop
[ 15.712287] Pid: 782, comm: modprobe Not tainted 3.8.7-0-grsec #1-Alpine
[ 15.712287] EIP: 0060:[] EFLAGS: 00010212 CPU: 0
[ 15.712287] EIP is at __gpio_cansleep+0xd/0x14
[ 15.712287] EAX: 00000000 EBX: ce3c2818 ECX: c1379bf0 EDX: 00000000
[ 15.712287] ESI: c142f7a4 EDI: c142f770 EBP: cdd67ddc ESP: cdd67dbc
[ 15.712287] DS: 0068 ES: 0068 FS: 00d8 GS: 007b SS: 0068
[ 15.712287] CR0: 8005003b CR2: aaa409b0 CR3: 0dd16000 CR4: 00000090
[ 15.712287] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 15.712287] DR6: ffff0ff0 DR7: 00000400
[ 15.712287] Process modprobe (pid: 782, ti=ce6d6a90 task=ce6d6700 task.ti=ce6d6a90)
[ 15.712287] Stack:
[ 15.712287] d0aa91cb 00000000 c142f660 ce3c2814 00000000 c142f660 c142f654 d0aa7014
[ 15.712287] cdd67df0 c11a5efd c142f660 d0aa7014 d0aa7014 cdd67e04 c11a4bf2 c142f660
[ 15.712287] d0aa7014 c142f694 cdd67e18 c11a4d2b cdd67e20 d0aa7014 00000000 cdd67e34
[ 15.712287] Call Trace:
[ 15.712287] [] ? gpio_led_probe+0xba/0x203 [leds_gpio]
[ 15.712287] [] platform_drv_probe+0x26/0x48
[ 15.712287] [] driver_probe_device+0x80/0x172
[ 15.712287] [] __driver_attach+0x47/0x63
[ 15.712287] [] bus_for_each_dev+0x3c/0x66
[ 15.712287] [] driver_attach+0x14/0x16
[ 15.712287] [] ? driver_probe_device+0x172/0x172
[ 15.712287] [] bus_add_driver+0xbd/0x1bc
[ 15.712287] [] ? 0xd0aaafff
[ 15.712287] [] ? 0xd0aaafff
[ 15.712287] [] driver_register+0x74/0xec
[ 15.712287] [] ? notifier_call_chain+0x25/0x46
[ 15.712287] [] ? 0xd0aaafff
[ 15.712287] [] platform_driver_register+0x38/0x3a
[ 15.712287] [] gpio_led_driver_init+0xd/0x5b5 [leds_gpio]
[ 15.712287] [] do_one_initcall+0x6a/0xff
[ 15.712287] [] load_module+0x15b2/0x18de
[ 15.712287] [] ? insert_vmalloc_vmlist+0x14/0x52
[ 15.712287] [] ? __vmalloc_node_range+0x13e/0x15f
[ 15.712287] [] sys_init_module+0x113/0x129
[ 15.712287] [] syscall_call+0x7/0xb
[ 15.712287] Code: 00 5c 50 c1 75 14 8b 82 04 5c 50 c1 a8 01 74 0a 8b 8a 08 5c 50 c1 eb 02 31 c9 89 c8 5d c3 6b c0 0c 55 89 e5 5d 8b 80 00 5c 50 c1 40 40 83 e0 01 c3 6b d0 0c 55 89 e5 53 8b 8a 00 5c 50 c1 8b
[ 15.712287] EIP: [] __gpio_cansleep+0xd/0x14 SS:ESP 0068:cdd67dbc
[ 15.714343] ---[ end trace d6fdd52620ab8376 ]---
[ 16.491213] cs5535-gpio cs5535-gpio: reserved resource region [io 0x6100-0x61ff]
[ 16.604968] cs5535-mfgpt cs5535-mfgpt: reserved resource region [io 0x6200-0x623f]
[ 16.606766] cs5535-mfgpt cs5535-mfgpt: 8 MFGPT timers available

This goes away if gpio_cs5535 is downloaded by hand before leds_gpio.
To me it seems that recent gpiolib changes do not handle the case that
the gpio chip driver is loaded after leds-gpio. Or perhaps leds-gpio
should request that platform specific module first if needed.

The relevant config is:
CONFIG_ALIX=y
CONFIG_LEDS_GPIO=m
CONFIG_GPIO_CS5535=m

Full config available at:
http://git.alpinelinux.org/cgit/aports/tree/main/linux-grsec/kernelconfig.x86



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2013-05-20 18:15 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-20  0:39 [PATCH] leds: leds-gpio: reserve gpio before using it Jingoo Han
2013-05-20 18:15 ` Bryan Wu
  -- strict thread matches above, loose matches on Subject: below --
2013-05-15  7:24 [Regression] Boot failure on ALIX during LED probe Timo Teras
2013-05-17  7:48 ` [PATCH] leds: leds-gpio: reserve gpio before using it Timo Teräs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome