[PATCH 0/2] gpio: mmio: read the line direction from pinctrl on chips without direction registers

[PATCH 0/2] gpio: mmio: read the line direction from pinctrl on chips without direction registers

[Mehmet Fide]

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

Hi Bartosz, Linus,

this is the series that replaces the gpiolib guard patch [1], along the
lines Bartosz suggested there: instead of teaching gpiod_get_direction()
to stay quiet when a chip has no get_direction(), give gpio-mmio one that
asks the pinctrl backend, and teach the pin controller to answer.

The user is the Vybrid GPIO block (gpio-vf610, a generic mmio chip with
GPIO_GENERIC_PINCTRL_BACKEND and no direction registers, the direction
lives in the iomuxc pad as the OBE bit). Today every
gpiod_get_direction() there trips the WARN in gpiolib, 21 backtraces per
boot on a Colibri VF61/VF50.

Patch 1 is the pinctrl-imx side. Bartosz asked whether the raw register
coming back from pin_config_get() is a bug in pinctrl-imx: it is, the
callback never looked at which parameter was requested. This patch makes
it parameter aware for PIN_CONFIG_OUTPUT_ENABLE and
PIN_CONFIG_INPUT_ENABLE on SoCs that say where those bits live (Vybrid:
OBE bit 1, IBE bit 0), and keeps the raw register as the fallback for
everything else, because the debugfs dump still relies on it. Converting
the driver fully to generic pinconf is a bigger job than this fix needs.

Patch 2 installs the get_direction() callback in gpio-mmio for the
"pinctrl backend, no direction registers" combination, mirroring how the
direction setters are already forwarded.

Tested on a Colibri VF61 (Iris carrier) on top of gpio/for-next: the 21
boot-time backtraces are gone, and /sys/kernel/debug/gpio now shows the
pad's real direction for every requested line (the hogs, the SD card
detect input, the USB VBUS regulator output). Pads with no pinctrl
configuration in the device tree cannot be queried and report -ENOTSUPP;
those pads cannot change direction through this chip either, as the
existing direction setters return -EINVAL for them, so nothing that
worked before is affected.

Patch 2 needs patch 1 to give correct answers: with the raw register
coming back, the direction would be read from the wrong bits. Taking
both through one tree, with an ack from the other side, avoids that
window.

[1] https://lore.kernel.org/linux-gpio/20260813193715.2346477-1-mehmet.fide@gmail.com/

Mehmet Fide (2):
  pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad
    register
  gpio: mmio: get the direction from pinctrl when there are no direction
    registers

 drivers/gpio/gpio-mmio.c                  | 27 ++++++++++++++++++++
 drivers/pinctrl/freescale/pinctrl-imx.c   | 30 +++++++++++++++++++++++
 drivers/pinctrl/freescale/pinctrl-imx.h   |  4 +++
 drivers/pinctrl/freescale/pinctrl-vf610.c |  2 ++
 4 files changed, 63 insertions(+)

-- 
2.54.0

[PATCH 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register

[Mehmet Fide]

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

The mmio pinconf get callback ignores which parameter was requested and
returns the raw conf register, so a generic query through
pinctrl_gpio_get_config() gets register bits back instead of the packed
parameter it asked for.

Decode the requested parameter and answer PIN_CONFIG_OUTPUT_ENABLE and
PIN_CONFIG_INPUT_ENABLE on SoCs that declare where those bits live in
the pad register; Vybrid has OBE at bit 1 and IBE at bit 0. The answer
is 0 with the bit value as the argument, which is what the
pinctrl_gpio_get_config() users (gpio-by-pinctrl, and gpio-mmio in the
next patch) expect. Pins the device tree never configured have no
register offset and get -ENOTSUPP, without the error message meant for
configuration attempts. Other parameters keep the historic raw-register
behaviour, which the debugfs dump still relies on.

This gives gpio-mmio a way to read back the line direction on chips
whose direction lives in the pin controller.

Suggested-by: Bartosz Golaszewski <brgl@kernel.org>
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
 drivers/pinctrl/freescale/pinctrl-imx.c   | 30 +++++++++++++++++++++++
 drivers/pinctrl/freescale/pinctrl-imx.h   |  4 +++
 drivers/pinctrl/freescale/pinctrl-vf610.c |  2 ++
 3 files changed, 36 insertions(+)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 9a45b376d36f..5da1a76b0141 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -21,6 +21,7 @@
 
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 
@@ -297,6 +298,35 @@ static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev, unsigned pin_id,
 	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	const struct imx_pinctrl_soc_info *info = ipctl->info;
 	const struct imx_pin_reg *pin_reg = &ipctl->pin_regs[pin_id];
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned int mask = 0;
+	u32 raw;
+
+	/*
+	 * Only OUTPUT_ENABLE/INPUT_ENABLE are decoded, and only when the SoC
+	 * declares the bits. Everything else still returns the raw conf
+	 * register, the debugfs dump depends on it.
+	 */
+	switch (param) {
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		mask = info->obe_mask;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		mask = info->ibe_mask;
+		break;
+	default:
+		break;
+	}
+
+	if (mask) {
+		/* pin not configured, nothing to report */
+		if (pin_reg->conf_reg == -1)
+			return -ENOTSUPP;
+
+		raw = readl(ipctl->base + pin_reg->conf_reg);
+		*config = pinconf_to_config_packed(param, !!(raw & mask));
+		return 0;
+	}
 
 	if (pin_reg->conf_reg == -1) {
 		dev_err(ipctl->dev, "Pin(%s) does not support config function\n",
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index f65ff45b4003..8fa7e1e2521d 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -91,6 +91,10 @@ struct imx_pinctrl_soc_info {
 	unsigned int mux_mask;
 	u8 mux_shift;
 
+	/* OBE/IBE bits in the conf register, 0 if the pad does not have them */
+	unsigned int obe_mask;
+	unsigned int ibe_mask;
+
 	int (*gpio_set_direction)(struct pinctrl_dev *pctldev,
 				  struct pinctrl_gpio_range *range,
 				  unsigned offset,
diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c b/drivers/pinctrl/freescale/pinctrl-vf610.c
index 76a4bc0181a0..77d077618782 100644
--- a/drivers/pinctrl/freescale/pinctrl-vf610.c
+++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
@@ -319,6 +319,8 @@ static const struct imx_pinctrl_soc_info vf610_pinctrl_info = {
 	.gpio_set_direction = vf610_pmx_gpio_set_direction,
 	.mux_mask = 0x700000,
 	.mux_shift = 20,
+	.obe_mask = 0x2,
+	.ibe_mask = 0x1,
 };
 
 static const struct of_device_id vf610_pinctrl_of_match[] = {
-- 
2.54.0

[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 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The patch introduces a read of an uninitialized stack variable, causing non-deterministic behavior and a cascading state trap that corrupts debugfs output.

Pre-existing issues:
- [High] Generic queries for unsupported SoCs fall through to the legacy path and return raw register bits instead of -ENOTSUPP, violating the generic API contract.
--

commit b3256a64d0ea28cb67caa6371dfdc4e8b8b48f42
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register
    
The mmio pinconf get callback ignores which parameter was requested and
returns the raw conf register, so a generic query through
pinctrl_gpio_get_config() gets register bits back instead of the packed
parameter it asked for.

> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 9a45b376d36f9..5da1a76b01412 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c

[ ... ]

> @@ -297,6 +298,35 @@ static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev, unsigned pin_id,
>  	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
>  	const struct imx_pinctrl_soc_info *info = ipctl->info;
>  	const struct imx_pin_reg *pin_reg = &ipctl->pin_regs[pin_id];
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	unsigned int mask = 0;
> +	u32 raw;
> +
> +	/*
> +	 * Only OUTPUT_ENABLE/INPUT_ENABLE are decoded, and only when the SoC
> +	 * declares the bits. Everything else still returns the raw conf
> +	 * register, the debugfs dump depends on it.
> +	 */
> +	switch (param) {
> +	case PIN_CONFIG_OUTPUT_ENABLE:
> +		mask = info->obe_mask;
> +		break;
> +	case PIN_CONFIG_INPUT_ENABLE:
> +		mask = info->ibe_mask;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (mask) {
> +		/* pin not configured, nothing to report */
> +		if (pin_reg->conf_reg == -1)
> +			return -ENOTSUPP;
> +
> +		raw = readl(ipctl->base + pin_reg->conf_reg);
> +		*config = pinconf_to_config_packed(param, !!(raw & mask));

[Severity: Medium]
Is it safe to extract a parameter from the incoming *config pointer?

Looking at imx_pinconf_group_dbg_show() in the same file, config is passed
to imx_pinconf_get() uninitialized:

drivers/pinctrl/freescale/pinctrl-imx.c:imx_pinconf_group_dbg_show() {
	...
	unsigned long config;
	...
	for (i = 0; i < grp->grp.npins; i++) {
		...
		ret = imx_pinconf_get(pctldev, pin->pin, &config);
	...
}

Since config contains stack garbage on the first iteration, and its
value is retained across loop iterations, could pinconf_to_config_param()
randomly decode into PIN_CONFIG_OUTPUT_ENABLE or PIN_CONFIG_INPUT_ENABLE?

If it does, or if a previous pin's raw register ends in the same byte,
won't this path embed the parameter back into the lowest 8 bits via
pinconf_to_config_packed()?

This appears to permanently trap all subsequent loop iterations into falsely
triggering the generic decoding path instead of returning raw registers.

> +		return 0;
> +	}
>  
>  	if (pin_reg->conf_reg == -1) {
>  		dev_err(ipctl->dev, "Pin(%s) does not support config function\n",

[Severity: High]
This is a pre-existing issue, but does this correctly handle generic queries
on SoCs that lack the requested masks?

If a generic caller requests PIN_CONFIG_OUTPUT_ENABLE via
pinctrl_gpio_get_config(), and the SoC does not define obe_mask, the
mask remains 0 and bypasses the if (mask) decoding block.

Does this cause the query to fall through to the default path:

drivers/pinctrl/freescale/pinctrl-imx.c:imx_pinconf_get_mmio() {
	...
	*config = readl(ipctl->base + pin_reg->conf_reg);

	if (info->flags & SHARE_MUX_CONF_REG)
		*config &= ~info->mux_mask;

	return 0;
}

and succeed, returning raw pad register bits to a generic caller instead of
rejecting the unsupported generic parameter with -ENOTSUPP?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902062352.3600368-1-mehmet.fide@gmail.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `gpiochip_lock_as_irq()` breaks for valid GPIOs that lack a pinctrl mapping or are fixed inputs, returning `-EPROBE_DEFER` or `-ENOTSUPP`.
- [High] The newly added `get_direction` callback for generic MMIO GPIO chips acquires a mutex via the pinctrl subsystem, but these chips are marked as non-sleeping (`can_sleep = false`). This leads to a scheduling-while-atomic bug when called from atomic contexts.
- [Medium] The patch fails to achieve its stated goal of preventing the initial line direction from being guessed, because `get_direction` is queried before pinctrl ranges are populated.
--

commit 58325f6a7a3ffea5ecb0fe02708d9e2517757ca9
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

gpio: mmio: get the direction from pinctrl when there are no direction registers
    
A generic chip with GPIO_GENERIC_PINCTRL_BACKEND and no direction
registers is given a get_direction callback to prevent gpiolib from guessing
the initial line state. The callback reads the direction from the pinctrl
backend.

> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index e9c531eef4521..25c8faa41749e 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.c
[ ... ]
> @@ -360,6 +361,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;

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

During chip initialization in gpiolib, the initial line state is queried by
gpiochip_add_data_with_key() calling gc->get_direction(). However, this
happens before gpiochip_add_pin_ranges() populates the pin ranges.

Since the pin ranges list is empty during this setup phase, this function will
always return -EOPNOTSUPP, which causes gpiolib to fall back to guessing the
state anyway.

> +#endif
> +
> +	config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
> +	ret = pinctrl_gpio_get_config(gc, gpio, &config);
> +	if (ret)
> +		return ret;

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

When an IRQ is requested, gpiochip_lock_as_irq() checks the line direction. If
the pin lacks a pinctrl mapping or is a fixed input, pinctrl_gpio_get_config()
returns an error such as -ENOTSUPP or -EPROBE_DEFER.

Because this error is now propagated back to gpiochip_lock_as_irq(), wouldn't
this cause the IRQ setup to fail for hardware configurations that were working
previously when get_direction was unset?

> +
> +	return config ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> +}
[ ... ]
> @@ -596,6 +619,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;

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

gpiochip_lock_as_irq()
  if (!gc->can_sleep && gc->get_direction)
    gpiod_get_direction()

However, the new callback delegates to pinctrl_gpio_get_config(), which
unconditionally acquires a sleepable mutex. Doesn't this result in taking a
mutex while local interrupts are disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902062352.3600368-1-mehmet.fide@gmail.com?part=2

Re: [PATCH 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register

[Mehmet Fide]

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

> [Severity: Medium]
> Is it safe to extract a parameter from the incoming *config pointer?
>
> Looking at imx_pinconf_group_dbg_show() in the same file, config is passed
> to imx_pinconf_get() uninitialized:
[...]
> This appears to permanently trap all subsequent loop iterations into falsely
> triggering the generic decoding path instead of returning raw registers.

Correct, the group dump reuses an uninitialized config across the loop
and my decode turns that into a sticky misread. In v2 the debugfs dumps
will read the raw register through their own helper, so they never go
through pin_config_get() and never depend on *config coming in.

> [Severity: High]
> This is a pre-existing issue, but does this correctly handle generic queries
> on SoCs that lack the requested masks?

Also correct. Once the dumps no longer use pin_config_get(), nothing
needs the raw fallback there anymore, so v2 makes the callback answer
only the parameters the SoC declares and return -ENOTSUPP for anything
else, instead of handing raw register bits to a generic caller.

Thanks,
Mehmet

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