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

[sashiko-bot@kernel.org]

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