Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932145Ab3ETSPZ (ORCPT ); Mon, 20 May 2013 14:15:25 -0400 Received: from mail-la0-f54.google.com ([209.85.215.54]:63401 "EHLO mail-la0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755997Ab3ETSPX (ORCPT ); Mon, 20 May 2013 14:15:23 -0400 MIME-Version: 1.0 In-Reply-To: <31802083.135751369010355609.JavaMail.weblogic@epml04> References: <31802083.135751369010355609.JavaMail.weblogic@epml04> From: Bryan Wu Date: Mon, 20 May 2013 11:15:01 -0700 Message-ID: Subject: Re: [PATCH] leds: leds-gpio: reserve gpio before using it To: Jingoo Han Cc: "Timo Ter?s" , Richard Purdie , "linux-leds@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Sachin Kamat , Raphael Assenat , Trent Piepho , Javier Martinez Canillas , Arnaud Patard , Ezequiel Garcia Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5476 Lines: 130 On Sun, May 19, 2013 at 5:39 PM, Jingoo Han wrote: > 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: [] __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:[] 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: >> [] gpio_led_probe+0xba/0x203 [leds_gpio] >> [] platform_drv_probe+0x26/0x48 >> [] driver_probe_device+0x75/0x15c >> [] __driver_attach+0x47/0x63 >> [] bus_for_each_dev+0x3c/0x66 >> [] driver_attach+0x14/0x16 >> [] ? driver_probe_device+0x15c/0x15c >> [] bus_add_driver+0xbd/0x1bc >> [] ? 0xd08b3fff >> [] ? 0xd08b3fff >> [] driver_register+0x74/0xec >> [] ? 0xd08b3fff >> [] platform_driver_register+0x38/0x3a >> [] gpio_led_driver_init+0xd/0x1000 [leds_gpio] >> [] do_one_initcall+0x6b/0x10f >> [] ? 0xd08b3fff >> [] load_module+0x1631/0x1907 >> [] ? insert_vmalloc_vmlist+0x14/0x43 >> [] ? __vmalloc_node_range+0x13e/0x15f >> [] sys_init_module+0x62/0x77 >> [] syscall_call+0x7/0xb >> EIP: [] __gpio_cansleep+0xe/0x1a SS:ESP 0068:cded9dbc >> CR2: 000000000000004c >> ---[ end trace 5308fb20d2514822 ]--- >> >> Signed-off-by: Timo Ter?s >> Cc: Jingoo Han > > Acked-by: Jingoo Han > > 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 Thanks for this fixing. I will add this into my leds-fixes-3.10 branch and send out for Linus. -Bryan >> Cc: Sachin Kamat >> Cc: Raphael Assenat >> Cc: Trent Piepho >> Cc: Javier Martinez Canillas >> Cc: Arnaud Patard >> Cc: Ezequiel Garcia >> --- >> 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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/