[PATCH 2/2] gpio: mmio: get the direction from pinctrl when there are no direction registers

[PATCH 2/2] gpio: mmio: get the direction from pinctrl when there are no direction registers

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

A generic chip with GPIO_GENERIC_PINCTRL_BACKEND and no direction
registers already sets the direction through pinctrl, but leaves
gc->get_direction unset, so every gpiod_get_direction() call trips the
WARN in gpiolib and the initial line state is guessed. On a Vybrid
Colibri module that is 21 backtraces per boot.

Install a get_direction callback for that combination which asks the
pinctrl backend for PIN_CONFIG_OUTPUT_ENABLE, the same way
gpio-by-pinctrl does it and mirroring how the direction setters are
forwarded. Pins the pin controller cannot answer for report -ENOTSUPP;
gpiolib ignores that when requesting a line and in the debugfs dump, and
such pins cannot change direction through this chip either, as the
setters fail for them too. The one caller that does not ignore it is
gpiochip_lock_as_irq(), which now refuses such a pin as an interrupt
where it previously had nothing to check against.

Suggested-by: Bartosz Golaszewski <brgl@kernel.org>
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
 drivers/gpio/gpio-mmio.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 7e4b3e8d609f..b2aa74a734e4 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -49,6 +49,7 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/spinlock.h>
@@ -365,6 +366,28 @@ static int gpio_mmio_dir_return(struct gpio_chip *gc, unsigned int gpio,
 		return pinctrl_gpio_direction_input(gc, gpio);
 }
 
+/*
+ * Without direction registers the direction lives in the pin controller
+ * (Vybrid: the OBE bit in the iomuxc pad), so ask pinctrl.
+ */
+static int gpio_mmio_pinctrl_get_dir(struct gpio_chip *gc, unsigned int gpio)
+{
+	unsigned long config;
+	int ret;
+
+#ifdef CONFIG_PINCTRL
+	if (list_empty(&gc->gpiodev->pin_ranges))
+		return -EOPNOTSUPP;
+#endif
+
+	config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
+	ret = pinctrl_gpio_get_config(gc, gpio, &config);
+	if (ret)
+		return ret;
+
+	return config ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
 static int gpio_mmio_dir_in_err(struct gpio_chip *gc, unsigned int gpio)
 {
 	return -EINVAL;
@@ -601,6 +624,10 @@ static int gpio_mmio_setup_direction(struct gpio_generic_chip *chip,
 			gc->direction_input = gpio_mmio_dir_in_err;
 		else
 			gc->direction_input = gpio_mmio_simple_dir_in;
+
+		if (IS_ENABLED(CONFIG_PINCTRL) &&
+		    cfg->flags & GPIO_GENERIC_PINCTRL_BACKEND)
+			gc->get_direction = gpio_mmio_pinctrl_get_dir;
 	}
 
 	return 0;
-- 
2.54.0

Re: [PATCH 2/2] gpio: mmio: get the direction from pinctrl when there are no direction registers

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

> [Severity: High]
> Can assigning this callback cause a scheduling-while-atomic bug?
>
> Generic MMIO GPIO chips are initialized with gc->can_sleep set to false.
>
> When an interrupt is activated, gpiochip_lock_as_irq() executes with the
> IRQ descriptor's raw spinlock held. Since it sees the chip is non-sleeping,
> it invokes this new get_direction callback directly in atomic context:

This one is real and the most important of the three: irq_activate()
runs inside the desc->lock region of __setup_irq(), reaches
gpiochip_lock_as_irq() through gpiochip_irq_domain_activate(), and for a
!can_sleep chip that calls get_direction(), so the pinctrl mutex would be
taken with interrupts off. My test kernel had DEBUG_ATOMIC_SLEEP off, the
uncontended mutex fastpath hid it.

So the callback cannot ask pinctrl. v2 changes the approach: the chip
keeps the direction in its existing shadow (sdir, under the chip's raw
spinlock), the direction setters update it, and get_direction() only
reads the shadow. The pad is asked once, in process context, from the
request() callback: gpiolib calls request() right before it calls
get_direction() for a newly requested line, so the shadow is seeded from
PIN_CONFIG_OUTPUT_ENABLE there and the line reports what the pad says.

> [Severity: High]
> Does returning these pinctrl errors break interrupt setup for valid GPIOs?

With the shadow, get_direction() never fails: a line pinctrl cannot
answer for keeps the input default, which is exactly what gpiolib
assumed before this patch, so gpiochip_lock_as_irq() behaves as it did.

> [Severity: Medium]
> Will this early exit prevent the patch from determining the initial line state?

Yes, the initial scan in gpiochip_add_data_with_key() runs before the
pin ranges exist, so nothing can be read from pinctrl at that point.
The v2 commit message will drop that claim; what the series fixes is the
WARN and the direction reported for requested lines.

Thanks,
Mehmet