| Message ID | 570294A4.2070801@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 4, 2016 at 6:21 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > 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. Thanks for looking into this. > 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. Yes, so it will need to be cross-coordinated with IRQ maintainers Marc and TGLX. > 3) [-] has_irq might need to be initialized by non-DT drivers Yes. Every driver in the kernel need to be audited. > 4) [-] irq_ready might need to be set manually by drivers which do not use GPIO irq > helpers (see change in gpio-mpc8xxx.c) Yes. That too. Every driver in the kernel need to be audited. > 4) irq_ready access synchronization on SMP? atomic? Uhhh.... I don't even understand the question. > job done with commit e6918cd 'gpiolib: handle probe deferrals better' > reverted. I have taken that out of my tree as well. My naive approach doesn't work. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/06/2016 04:39 PM, Linus Walleij wrote: > On Mon, Apr 4, 2016 at 6:21 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > >> 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. > > Thanks for looking into this. > >> 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. > > Yes, so it will need to be cross-coordinated with IRQ maintainers > Marc and TGLX. Seems, I have to study how to be more clear :( This +/- are all about my RFC patch. My patch limits the scope of problem to GPIO subsystem/drivers only. As opposite, if we will touch gpiod_to_irq() - it will affect on whole kernel. > >> 3) [-] has_irq might need to be initialized by non-DT drivers > > Yes. Every driver in the kernel need to be audited. With my patch only GPIO drivers need to be checked, especially GPIO drivers which: - are non-DT based; - do not use GPIO irq helpers - can make IRQ functionality optional using Kconfig/sysfs/module parameters > >> 4) [-] irq_ready might need to be set manually by drivers which do not use GPIO irq >> helpers (see change in gpio-mpc8xxx.c) > > Yes. That too. Every driver in the kernel need to be audited. > >> 4) irq_ready access synchronization on SMP? atomic? > > Uhhh.... I don't even understand the question. in my patch the irq_ready is set from _gpiochip_irqchip_add() and read from gpiod_request() without any kind of protection and those two functions can be executed in parallel. > >> job done with commit e6918cd 'gpiolib: handle probe deferrals better' >> reverted. > > I have taken that out of my tree as well. My naive approach > doesn't work. >
On Wed, Apr 6, 2016 at 5:42 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > On 04/06/2016 04:39 PM, Linus Walleij wrote: >>> 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. >> >> Yes, so it will need to be cross-coordinated with IRQ maintainers >> Marc and TGLX. > > Seems, I have to study how to be more clear :( > This +/- are all about my RFC patch. > My patch limits the scope of problem to GPIO subsystem/drivers only. > As opposite, if we will touch gpiod_to_irq() - it will affect on whole > kernel. OK I get it now, good. >> Yes. Every driver in the kernel need to be audited. > > With my patch only GPIO drivers need to be checked, especially GPIO > drivers which: > - are non-DT based; > - do not use GPIO irq helpers > - can make IRQ functionality optional using Kconfig/sysfs/module parameters Yeah. I mean you have to look at all of them, not patch all of them. It's a lot but not unberably much. < 300 files. >>> 4) irq_ready access synchronization on SMP? atomic? >> >> Uhhh.... I don't even understand the question. > > in my patch the irq_ready is set from _gpiochip_irqchip_add() and > read from gpiod_request() without any kind of protection and those > two functions can be executed in parallel. Aha. Well I don't know if that is really a big problem? Does that really happen in practice? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 07 April 2016 19:09:06, Linus Walleij wrote: > >>> 4) irq_ready access synchronization on SMP? atomic? > >> > >> Uhhh.... I don't even understand the question. > > > > in my patch the irq_ready is set from _gpiochip_irqchip_add() and > > read from gpiod_request() without any kind of protection and those > > two functions can be executed in parallel. > > Aha. Well I don't know if that is really a big problem? > Does that really happen in practice? I guess this is what actually happens in my case. The gpio controller has already been registred and the companion irq chip is about to be registered. Meanwhile gpio-keys requests a GPIO from that recently registred gpio controller and the following gpio_to_irq or irq_request returns 0 or fails as the irq chip has not been registred yet (without Grygorii's patch). So this calling situation might actually happen. Best regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);