From patchwork Mon Apr 4 16:21:56 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Grygorii Strashko X-Patchwork-Id: 8742181 Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 145659F39A for ; Mon, 4 Apr 2016 16:22:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E8003201F2 for ; Mon, 4 Apr 2016 16:22:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 70F1320125 for ; Mon, 4 Apr 2016 16:22:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753828AbcDDQWu (ORCPT ); Mon, 4 Apr 2016 12:22:50 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:53165 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751902AbcDDQWs (ORCPT ); Mon, 4 Apr 2016 12:22:48 -0400 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id u34GLx2J031840; Mon, 4 Apr 2016 11:21:59 -0500 Received: from DLEE70.ent.ti.com (dlemailx.itg.ti.com [157.170.170.113]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id u34GLxi4025906; Mon, 4 Apr 2016 11:21:59 -0500 Received: from dlep33.itg.ti.com (157.170.170.75) by DLEE70.ent.ti.com (157.170.170.113) with Microsoft SMTP Server id 14.3.224.2; Mon, 4 Apr 2016 11:21:59 -0500 Received: from [172.22.232.237] (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep33.itg.ti.com (8.14.3/8.13.8) with ESMTP id u34GLuln025142; Mon, 4 Apr 2016 11:21:57 -0500 Subject: Re: [PATCH] gpiolib: handle probe deferrals better To: Linus Walleij , , Alexandre Courbot , Alexander Stein , Alexander Stein References: <1459511048-24084-1-git-send-email-linus.walleij@linaro.org> <56FE7785.1010507@ti.com> CC: , Tomeu Vizoso , Guenter Roeck , From: Grygorii Strashko Message-ID: <570294A4.2070801@ti.com> Date: Mon, 4 Apr 2016 19:21:56 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <56FE7785.1010507@ti.com> Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Linus, On 04/01/2016 04:28 PM, Grygorii Strashko wrote: > On 04/01/2016 02:44 PM, Linus Walleij wrote: >> The gpiolib does not currently return probe deferrals from the >> .to_irq() hook while the GPIO drivers are being initialized. >> Further: it keeps returning -EPROBE_DEFER for gpio[d]_get() >> even after all builtin drivers have initialized. >> >> Fix this thusly: >> >> - Move the assignment of .to_irq() to the last step when >> using gpiolib_irqchip_add() so we can't get spurious calls >> into the .to_irq() function until all set-up is finished. >> >> - Put in a late_initcall_sync() to set a boolean state variable to >> indicate that we're not gonna defer any longer. Since deferred >> probe happens at late_initcall() time, using late_initcall_sync() >> should be fine. >> >> - After this point, return hard errors (-ENXIO) from both >> gpio[d]_get() and .to_irq(). >> >> This way we should (at least for all drivers using GPIOLIB_IRQCHIP) >> be getting proper deferrals from both gpio[d]_get() and .to_irq() >> until the irqchip side is properly set up, and then proper >> errors after all drivers should have been probed. >> >> This problem was first seen with gpio-keys. >> >> Cc: linux-input@vger.kernel.org >> Cc: Tomeu Vizoso >> Cc: Guenter Roeck >> Reported-by: Alexander Stein >> Signed-off-by: Linus Walleij >> --- >> Alexander: please test this, you need Guether's patches too, >> I have it all on my "fixes" branch in the GPIO git: >> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/log/?h=fixes >> >> Tomeu: I think you're the authority on deferred probe these days. >> Is this the right way for a subsystem to switch from returning >> -EPROBE_DEFER to instead returning an unrecoverable error? >> >> Guenther: I applied this on top of your patches, please check it >> if you can, you're smarter than me with this stuff. >> --- >> drivers/gpio/gpiolib.c | 47 ++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 40 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index b747c76fd2b1..426a93f9d79e 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -68,7 +68,9 @@ LIST_HEAD(gpio_devices); >> static void gpiochip_free_hogs(struct gpio_chip *chip); >> static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); >> >> +/* These keep the state of the library */ >> static bool gpiolib_initialized; >> +static bool gpiolib_builtin_ready; >> [..] >> >> int gpiod_request(struct gpio_desc *desc, const char *label) >> { >> - int status = -EPROBE_DEFER; >> + int status; >> struct gpio_device *gdev; >> >> VALIDATE_DESC(desc); >> gdev = desc->gdev; >> >> + /* >> + * Defer requests until all built-in drivers have had a chance >> + * to probe, then give up and return a hard error. >> + */ >> + if (!gpiolib_builtin_ready) >> + status = -EPROBE_DEFER; >> + else >> + status = -ENXIO; > > Probably It will work right if we will let gpiochip know that > it has irqchip companion. > With above knowledge the gpiod_request() can return -EPROBE_DEFER while > irqchip is not initialized. In other words: > - driver will set HAS_IRQ flag > - gpiolib will set gpio_chip->irqs_ready = 0 in gpiochip_add_data() > - gpiolib will set gpio_chip->irqs_ready = 1 in gpiochip_irqchip_add() > - gpiod_request() will do at the beginning: > if (HAS_IRQ && !gpio_chip->irqs_ready) > return -EPROBE_DEFER > > it might also required to combine > gpiochip_irqchip_add and gpiochip_set_chained_irqchip. > Below is RFC patch to prove above consent. I've had offlist debugging session with Alexander and He mentioned that this change fixes boot issue for him. Of course, there are some question to discuss: 1) [+] It should sync initialization of GPIOchip and GPIOirqchip 2) [+] This approach requires changes in gpiolib/gpio drivers only, from other side It will require to add fixes all over the Kernel if gpiod_to_irq() will start returning -EPROBE_DEFER. 3) [-] has_irq might need to be initialized by non-DT drivers 4) [-] irq_ready might need to be set manually by drivers which do not use GPIO irq helpers (see change in gpio-mpc8xxx.c) 4) irq_ready access synchronization on SMP? atomic? job done with commit e6918cd 'gpiolib: handle probe deferrals better' reverted. ----- From d3d486700351d55fd242c7d9494740b0534a2081 Mon Sep 17 00:00:00 2001 From: Grygorii Strashko Date: Sat, 2 Apr 2016 14:55:31 +0300 Subject: [RFC PATCH] gpiolib: sync gpiochip and irqchip initializtion TBD. Signed-off-by: Grygorii Strashko --- drivers/gpio/gpio-mpc8xxx.c | 2 ++ drivers/gpio/gpiolib.c | 13 ++++++++++++- include/linux/gpio/driver.h | 2 ++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c index 425501c..3652b44 100644 --- a/drivers/gpio/gpio-mpc8xxx.c +++ b/drivers/gpio/gpio-mpc8xxx.c @@ -368,6 +368,8 @@ static int mpc8xxx_probe(struct platform_device *pdev) irq_set_chained_handler_and_data(mpc8xxx_gc->irqn, mpc8xxx_gpio_irq_cascade, mpc8xxx_gc); + + gc->irq_ready = true; return 0; err: iounmap(mpc8xxx_gc->regs); diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 7206553..9d24196 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -488,6 +488,11 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data) gdev->dev.of_node = chip->of_node; #endif } + + chip->has_irq = of_get_property(gdev->dev.of_node, + "interrupt-controller", NULL) ? + true : false; + gdev->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL); if (gdev->id < 0) { status = gdev->id; @@ -1092,6 +1097,9 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, acpi_gpiochip_request_interrupts(gpiochip); + if (gpiochip->has_irq) + gpiochip->irq_ready = true; + return 0; } EXPORT_SYMBOL_GPL(_gpiochip_irqchip_add); @@ -1335,7 +1343,10 @@ int gpiod_request(struct gpio_desc *desc, const char *label) gdev = desc->gdev; if (try_module_get(gdev->owner)) { - status = __gpiod_request(desc, label); + if (!gdev->chip->has_irq) + status = __gpiod_request(desc, label); + else if (gdev->chip->has_irq && gdev->chip->irq_ready) + status = __gpiod_request(desc, label); if (status < 0) module_put(gdev->owner); else diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index bee976f..134be32 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -141,6 +141,8 @@ struct gpio_chip { const char *const *names; bool can_sleep; bool irq_not_threaded; + bool has_irq; + bool irq_ready; #if IS_ENABLED(CONFIG_GPIO_GENERIC) unsigned long (*read_reg)(void __iomem *reg);