gpiolib: handle probe deferrals better

Message ID 1459511048-24084-1-git-send-email-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij April 1, 2016, 11:44 a.m.
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(-)

-- 
2.4.3

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

Comments

Linus Walleij April 1, 2016, 1:03 p.m. | #1
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
Bjorn Andersson April 1, 2016, 5:52 p.m. | #2
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

Patch hide | download patch | download mbox

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)