[PATCH] gpiolib: handle probe deferrals better

[PATCH] gpiolib: handle probe deferrals better

[Linus Walleij]

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;
+
 	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)
-- 
2.4.3

Re: [PATCH] gpiolib: handle probe deferrals better

[Alexander Stein]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Guenter Roeck]

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)
>

Re: [PATCH] gpiolib: handle probe deferrals better

[Linus Walleij]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Grygorii Strashko]

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);


-- 
regards,
-grygorii

Re: [PATCH] gpiolib: handle probe deferrals better

[Alexander Stein]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Grygorii Strashko]

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.
> 


-- 
regards,
-grygorii

Re: [PATCH] gpiolib: handle probe deferrals better

[Guenter Roeck]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Alexander Stein]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Bjorn Andersson]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Grygorii Strashko]

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 <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;
>>    

[..]

>>    
>>    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 <grygorii.strashko@ti.com>
Date: Sat, 2 Apr 2016 14:55:31 +0300
Subject: [RFC PATCH] gpiolib: sync gpiochip and irqchip initializtion

TBD.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 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);
-- 
2.8.0



-- 
regards,
-grygorii

Re: [PATCH] gpiolib: handle probe deferrals better

[Linus Walleij]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Grygorii Strashko]

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.
> 


-- 
regards,
-grygorii

Re: [PATCH] gpiolib: handle probe deferrals better

[Linus Walleij]

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

Re: [PATCH] gpiolib: handle probe deferrals better

[Alexander Stein]

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