| Message ID | 1459511048-24084-1-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Friday 01 April 2016 13:44:08, 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 <tomeu.vizoso@collabora.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Reported-by: Alexander Stein <alexander.stein@systec-electronic.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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=fi > xes I cherry-picked the top most 3 patches: 7f41f039a096a957eaa3e0ef0b040d96e7ad200d 2f126cda38e0614b3e8d211047b18e831b110f82 e6918cd02fd84402311f3fab4b218d9c911e70d6 Unfortunately it does not fix it. At least I do not get an IRQ of 0. After 10 boots: 4x gpio-keys user_sw: Unable to get irq number for GPIO 376, error -6 1x gpio-keys dip: Unable to get irq number for GPIO 352, error -6 5x ok user_sw and dip are essientially the same, just different devices in DT with different gpiochip parents (MCP23S18 and PCA9555). I noticed you fiddle with late_initcall_sync. Sorry, I did not mention it: gpio_mcp23s08 as well as gpio_keys are loaded as modules, so late_initcall_* should not affect it. Best regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/01/2016 04:44 AM, 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 <tomeu.vizoso@collabora.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Reported-by: Alexander Stein <alexander.stein@systec-electronic.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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. Not sure about that ;-). I'll run it through my test suite. Guenter > --- > 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; > > static inline void desc_set_label(struct gpio_desc *d, const char *label) > { > @@ -1093,7 +1095,6 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, > gpiochip->irqchip = irqchip; > gpiochip->irq_handler = handler; > gpiochip->irq_default_type = type; > - gpiochip->to_irq = gpiochip_to_irq; > gpiochip->lock_key = lock_key; > gpiochip->irqdomain = irq_domain_add_simple(of_node, > gpiochip->ngpio, first_irq, > @@ -1129,6 +1130,12 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, > } > > acpi_gpiochip_request_interrupts(gpiochip); > + /* > + * Wait with this until last, as someone may be asynchronously > + * calling .to_irq() and needs to be getting probe deferrals until > + * this point. > + */ > + gpiochip->to_irq = gpiochip_to_irq; > > return 0; > } > @@ -1366,12 +1373,21 @@ done: > > 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; > + > if (try_module_get(gdev->owner)) { > status = __gpiod_request(desc, label); > if (status < 0) > @@ -1993,18 +2009,27 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep); > * gpiod_to_irq() - return the IRQ corresponding to a GPIO > * @desc: gpio whose IRQ will be returned (already requested) > * > - * Return the IRQ corresponding to the passed GPIO, or an error code in case of > - * error. > + * Return the IRQ corresponding to the passed GPIO, or an error code. > */ > int gpiod_to_irq(const struct gpio_desc *desc) > { > - struct gpio_chip *chip; > - int offset; > + struct gpio_chip *chip; > + int offset; > > VALIDATE_DESC(desc); > chip = desc->gdev->chip; > offset = gpio_chip_hwgpio(desc); > - return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO; > + > + if (chip->to_irq) > + return chip->to_irq(chip, offset); > + /* > + * We will wait for new GPIO drivers to arrive until the > + * late initcalls. After that we stop deferring and return > + * a hard error. > + */ > + if (!gpiolib_builtin_ready) > + return -EPROBE_DEFER; > + return -ENXIO; > } > EXPORT_SYMBOL_GPL(gpiod_to_irq); > > @@ -2883,6 +2908,14 @@ static int __init gpiolib_dev_init(void) > } > core_initcall(gpiolib_dev_init); > > +static int __init gpiolib_late_done(void) > +{ > + /* Flag that we're not deferring probes anymore */ > + gpiolib_builtin_ready = true; > + return 0; > +} > +late_initcall_sync(gpiolib_late_done); > + > #ifdef CONFIG_DEBUG_FS > > static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev) > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 1, 2016 at 2:16 PM, Alexander Stein <alexander.stein@systec-electronic.com> wrote: > I noticed you fiddle with late_initcall_sync. Sorry, I did not mention it: > gpio_mcp23s08 as well as gpio_keys are loaded as modules, so late_initcall_* > should not affect it. I don't get this. I think probe deferral is only used to defer initcalls used for built-in drivers. If there are dependencies among things compiled as modules, doesn't depmod/modprobe make sure that they are probed in the right order? Could it be that some module alias thingofabob is missing? Or is modprobe failing because it (correctly) see that there is no symbol dependencies between the gpio_mcp23s08 and gpio-keys modules? (Not that I know how depmod works...) If nothing else works, I guess marking mcp23s08 as bool and building it into the kernel will work, right? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <tomeu.vizoso@collabora.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Reported-by: Alexander Stein <alexander.stein@systec-electronic.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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; > > static inline void desc_set_label(struct gpio_desc *d, const char *label) > { > @@ -1093,7 +1095,6 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, > gpiochip->irqchip = irqchip; > gpiochip->irq_handler = handler; > gpiochip->irq_default_type = type; > - gpiochip->to_irq = gpiochip_to_irq; > gpiochip->lock_key = lock_key; > gpiochip->irqdomain = irq_domain_add_simple(of_node, > gpiochip->ngpio, first_irq, > @@ -1129,6 +1130,12 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, > } > > acpi_gpiochip_request_interrupts(gpiochip); > + /* > + * Wait with this until last, as someone may be asynchronously > + * calling .to_irq() and needs to be getting probe deferrals until > + * this point. > + */ > + gpiochip->to_irq = gpiochip_to_irq; > > return 0; > } > @@ -1366,12 +1373,21 @@ done: > > 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. > + > if (try_module_get(gdev->owner)) { > status = __gpiod_request(desc, label); > if (status < 0) > @@ -1993,18 +2009,27 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep); > * gpiod_to_irq() - return the IRQ corresponding to a GPIO > * @desc: gpio whose IRQ will be returned (already requested) > * > - * Return the IRQ corresponding to the passed GPIO, or an error code in case of > - * error. > + * Return the IRQ corresponding to the passed GPIO, or an error code. > */ > int gpiod_to_irq(const struct gpio_desc *desc) > { > - struct gpio_chip *chip; > - int offset; > + struct gpio_chip *chip; > + int offset; > > VALIDATE_DESC(desc); > chip = desc->gdev->chip; > offset = gpio_chip_hwgpio(desc); > - return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO; > + > + if (chip->to_irq) > + return chip->to_irq(chip, offset); > + /* > + * We will wait for new GPIO drivers to arrive until the > + * late initcalls. After that we stop deferring and return > + * a hard error. > + */ > + if (!gpiolib_builtin_ready) > + return -EPROBE_DEFER; yah. This might not help with modules :( > + return -ENXIO; > } > EXPORT_SYMBOL_GPL(gpiod_to_irq); > > @@ -2883,6 +2908,14 @@ static int __init gpiolib_dev_init(void) > } > core_initcall(gpiolib_dev_init); > > +static int __init gpiolib_late_done(void) > +{ > + /* Flag that we're not deferring probes anymore */ > + gpiolib_builtin_ready = true; > + return 0; > +} > +late_initcall_sync(gpiolib_late_done);
On Friday 01 April 2016 15:03:40, Linus Walleij wrote: > On Fri, Apr 1, 2016 at 2:16 PM, Alexander Stein > > <alexander.stein@systec-electronic.com> wrote: > > I noticed you fiddle with late_initcall_sync. Sorry, I did not mention it: > > gpio_mcp23s08 as well as gpio_keys are loaded as modules, so > > late_initcall_* should not affect it. > > I don't get this. I think probe deferral is only used to defer > initcalls used for built-in drivers. To my understanding probe deferral is for the case when the parent or some other required resource like DT nodes (e.g. panel node for graphics driver) is not available yet. IIRC all deffered device are probed once a device was probed sucessfully, no? So in my case, I would expect that gpio-keys probe fails due to missing IRQ parent and gets deferred. Once the IRQ parent happen to be probed successfully at some point gpio-keys is probed again and succeeds. > If there are dependencies among things compiled as modules, > doesn't depmod/modprobe make sure that they are probed in > the right order? Could it be that some module alias thingofabob > is missing? > > Or is modprobe failing because it (correctly) see that there is > no symbol dependencies between the gpio_mcp23s08 and > gpio-keys modules? (Not that I know how depmod works...) I don't think modprobe can or even should do any dependencies here. Consider the following cascading GPIO chips: /-------\ /---------\ /--------\ | | | | | | |MCP23S17 + Pin1 <--> IRQ-+ PCA9555 + Pin 1 <--> IRQ + MCP23S17 + | | | | | | \-------/ \-------/ \--------/ Probing should still work for this scenario. This is something modprobe cannot fix. > If nothing else works, I guess marking mcp23s08 as bool > and building it into the kernel will work, right? This would merely be a workaround and I guess there might be scenarios and/or random device probing where even this fails. Best regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/01/2016 04:42 PM, Alexander Stein wrote: > On Friday 01 April 2016 15:03:40, Linus Walleij wrote: >> On Fri, Apr 1, 2016 at 2:16 PM, Alexander Stein >> >> <alexander.stein@systec-electronic.com> wrote: >>> I noticed you fiddle with late_initcall_sync. Sorry, I did not mention it: >>> gpio_mcp23s08 as well as gpio_keys are loaded as modules, so >>> late_initcall_* should not affect it. >> >> I don't get this. I think probe deferral is only used to defer >> initcalls used for built-in drivers. It doesn't. Deferred probing expected to work for modules also. > > To my understanding probe deferral is for the case when the parent or some > other required resource like DT nodes (e.g. panel node for graphics driver) is > not available yet. IIRC all deffered device are probed once a device was > probed sucessfully, no? true. > So in my case, I would expect that gpio-keys probe fails due to missing IRQ > parent and gets deferred. Once the IRQ parent happen to be probed successfully > at some point gpio-keys is probed again and succeeds. I assume in your case It's devm_gpio_request_one() first of all, which follows by gpio_to_irq. > >> If there are dependencies among things compiled as modules, >> doesn't depmod/modprobe make sure that they are probed in >> the right order? Could it be that some module alias thingofabob >> is missing? >> >> Or is modprobe failing because it (correctly) see that there is >> no symbol dependencies between the gpio_mcp23s08 and >> gpio-keys modules? (Not that I know how depmod works...) > > I don't think modprobe can or even should do any dependencies here. > Consider the following cascading GPIO chips: > > /-------\ /---------\ /--------\ > | | | | | | > |MCP23S17 + Pin1 <--> IRQ-+ PCA9555 + Pin 1 <--> IRQ + MCP23S17 + > | | | | | | > \-------/ \-------/ \--------/ > > Probing should still work for this scenario. This is something modprobe cannot > fix. What I can see from my boot log is that at module loading time log output from many drivers is mixed which makes me think that this parallel process (udev) - and no I've not hit this issue..yet. > >> If nothing else works, I guess marking mcp23s08 as bool >> and building it into the kernel will work, right? > > This would merely be a workaround and I guess there might be scenarios and/or > random device probing where even this fails. >
On Fri, Apr 01, 2016 at 03:03:40PM +0200, Linus Walleij wrote: > On Fri, Apr 1, 2016 at 2:16 PM, Alexander Stein > <alexander.stein@systec-electronic.com> wrote: > > > I noticed you fiddle with late_initcall_sync. Sorry, I did not mention it: > > gpio_mcp23s08 as well as gpio_keys are loaded as modules, so late_initcall_* > > should not affect it. > > I don't get this. I think probe deferral is only used to defer > initcalls used for built-in drivers. > Maybe late_initcall() and driver_probe_done() are not synchronous ? In other words, is it guaranteed that driver_probe_done() returns true when late_initcall() is executed ? Thanks, Guenter > If there are dependencies among things compiled as modules, > doesn't depmod/modprobe make sure that they are probed in > the right order? Could it be that some module alias thingofabob > is missing? > > Or is modprobe failing because it (correctly) see that there is > no symbol dependencies between the gpio_mcp23s08 and > gpio-keys modules? (Not that I know how depmod works...) > > If nothing else works, I guess marking mcp23s08 as bool > and building it into the kernel will work, right? > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 01 April 2016 17:03:08, Grygorii Strashko wrote: > > So in my case, I would expect that gpio-keys probe fails due to missing > > IRQ > > parent and gets deferred. Once the IRQ parent happen to be probed > > successfully at some point gpio-keys is probed again and succeeds. > > I assume in your case It's devm_gpio_request_one() first of all, which > follows by gpio_to_irq. I think nothing is wrong with that. The problem is that the former succeeds while the latter doesn't although from a device view point the GPIO chip is already there. > > Consider the following cascading GPIO chips: > > /-------\ /---------\ /--------\ > > | > > |MCP23S17 + Pin1 <--> IRQ-+ PCA9555 + Pin 1 <--> IRQ + MCP23S17 + > > | > > \-------/ \-------/ \--------/ > > > > Probing should still work for this scenario. This is something modprobe > > cannot fix. > > What I can see from my boot log is that at module loading time log output > from many drivers is mixed which makes me think that this parallel process > (udev) - and no I've not hit this issue..yet. I think parallel device probing is expectable and reasonable. I guess to hit this issue you need a device which requires GPIOs and their corresponding IRQ and probing of that must be done while GPIO chip is attached but not it's IRQ chip. Best regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 01 Apr 06:03 PDT 2016, Linus Walleij wrote: > On Fri, Apr 1, 2016 at 2:16 PM, Alexander Stein > <alexander.stein@systec-electronic.com> wrote: > > > I noticed you fiddle with late_initcall_sync. Sorry, I did not mention it: > > gpio_mcp23s08 as well as gpio_keys are loaded as modules, so late_initcall_* > > should not affect it. > > I don't get this. I think probe deferral is only used to defer > initcalls used for built-in drivers. > FWIW, upon matching a device to a driver you will end up in really_probe(), which if the called probe function returns -EPROBE_DEFER will put the device on a deferred-probe-list. A worker is triggered upon subsequent binds that runs through this list and retries the probes, but only once something else successfully probes, potentially providing the missing resources. So it does not treat builtins and modules differently. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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/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; static inline void desc_set_label(struct gpio_desc *d, const char *label) { @@ -1093,7 +1095,6 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, gpiochip->irqchip = irqchip; gpiochip->irq_handler = handler; gpiochip->irq_default_type = type; - gpiochip->to_irq = gpiochip_to_irq; gpiochip->lock_key = lock_key; gpiochip->irqdomain = irq_domain_add_simple(of_node, gpiochip->ngpio, first_irq, @@ -1129,6 +1130,12 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, } acpi_gpiochip_request_interrupts(gpiochip); + /* + * Wait with this until last, as someone may be asynchronously + * calling .to_irq() and needs to be getting probe deferrals until + * this point. + */ + gpiochip->to_irq = gpiochip_to_irq; return 0; } @@ -1366,12 +1373,21 @@ done: 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; + if (try_module_get(gdev->owner)) { status = __gpiod_request(desc, label); if (status < 0) @@ -1993,18 +2009,27 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep); * gpiod_to_irq() - return the IRQ corresponding to a GPIO * @desc: gpio whose IRQ will be returned (already requested) * - * Return the IRQ corresponding to the passed GPIO, or an error code in case of - * error. + * Return the IRQ corresponding to the passed GPIO, or an error code. */ int gpiod_to_irq(const struct gpio_desc *desc) { - struct gpio_chip *chip; - int offset; + struct gpio_chip *chip; + int offset; VALIDATE_DESC(desc); chip = desc->gdev->chip; offset = gpio_chip_hwgpio(desc); - return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO; + + if (chip->to_irq) + return chip->to_irq(chip, offset); + /* + * We will wait for new GPIO drivers to arrive until the + * late initcalls. After that we stop deferring and return + * a hard error. + */ + if (!gpiolib_builtin_ready) + return -EPROBE_DEFER; + return -ENXIO; } EXPORT_SYMBOL_GPL(gpiod_to_irq); @@ -2883,6 +2908,14 @@ static int __init gpiolib_dev_init(void) } core_initcall(gpiolib_dev_init); +static int __init gpiolib_late_done(void) +{ + /* Flag that we're not deferring probes anymore */ + gpiolib_builtin_ready = true; + return 0; +} +late_initcall_sync(gpiolib_late_done); + #ifdef CONFIG_DEBUG_FS static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
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 <tomeu.vizoso@collabora.com> Cc: Guenter Roeck <linux@roeck-us.net> Reported-by: Alexander Stein <alexander.stein@systec-electronic.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- 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(-)