Question: GPIO direction callbacks calling pinctrl in atomic paths

Question: GPIO direction callbacks calling pinctrl in atomic paths

[Runyu Xiao]

Hi,

While auditing GPIO direction callbacks, our static analysis tool flagged
several drivers whose direction_input/direction_output paths call into
the pinctrl core even though the GPIO chip is registered as non-sleeping.
We then manually reviewed the findings against the current tree.

The class of path we looked at is:

  gpiod_direction_output_raw_commit()
    -> <driver>_gpio_direction_output()
       -> pinctrl_gpio_direction_output()
       -> pinctrl_get_device_gpio_range()
       -> mutex_lock(&pctldev->mutex)

That can be reached from shared GPIO users while a per-line spinlock is
still held.  A minimal Lockdep reproducer preserving this direction path
reports:

  BUG: sleeping function called from invalid context
  #0: ... (&global_shared_desc.spinlock) ...
  pinctrl_get_device_gpio_range
  <driver>_gpio_direction_output
  [ BUG: Invalid wait context ]

My first draft for this class was to mark the affected gpio_chip as
can_sleep, but that looks like the wrong contract.  gpio_chip::can_sleep
describes whether get()/set() may sleep, while the problematic operation
here is not MMIO value access but an extra pinctrl direction round-trip.
Rockchip history seems to support that concern: after the controller was
marked sleeping, a follow-up change stopped calling pinctrl for
set_direction because whole-chip can_sleep caused atomic get/set
warnings.

For PXA and Tegra, I am considering a small series that removes the
pinctrl_gpio_direction_input/output() calls from the GPIO direction
callbacks and leaves direction programming on the drivers' existing MMIO
paths.

For PXA, the driver already updates GPDR directly in
pxa_gpio_direction_input/output().  The proposed change would drop the
additional pinctrl direction call on variants where pxa_gpio_has_pinctrl()
currently returns true.

For Tegra, the GPIO driver already programs the GPIO controller direction
registers directly.  The Tegra pinmux ops appear to provide GPIO
request/free handling, but no gpio_set_direction hook, so the
pinctrl_gpio_direction_input/output() call seems to enter the pinctrl core
without adding a Tegra-specific direction operation.  The proposed change
would keep pinctrl involvement in request/free but not in GPIO direction.

I am not proposing the same change for MVEBU in this question.  Its
pinctrl direction hook appears to enforce GPI/GPO capability checks, so
simply removing the pinctrl direction call there would remove real
driver-specific validation and needs a different design.

Does removing the pinctrl direction calls for PXA and Tegra sound like
the right direction, or would you prefer that this be handled differently
in gpiolib/pinctrl?

The local draft subjects are:

  gpio: pxa: stop routing direction changes through pinctrl
  gpio: tegra: do not call pinctrl for GPIO direction

Thanks,
Runyu

Re: Question: GPIO direction callbacks calling pinctrl in atomic paths

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2837 bytes --]

On Thu, Jun 18, 2026 at 11:06:09AM +0800, Runyu Xiao wrote:
> Hi,
> 
> While auditing GPIO direction callbacks, our static analysis tool flagged
> several drivers whose direction_input/direction_output paths call into
> the pinctrl core even though the GPIO chip is registered as non-sleeping.
> We then manually reviewed the findings against the current tree.
> 
> The class of path we looked at is:
> 
>   gpiod_direction_output_raw_commit()
>     -> <driver>_gpio_direction_output()
>        -> pinctrl_gpio_direction_output()
>        -> pinctrl_get_device_gpio_range()
>        -> mutex_lock(&pctldev->mutex)
> 
> That can be reached from shared GPIO users while a per-line spinlock is
> still held.  A minimal Lockdep reproducer preserving this direction path
> reports:
> 
>   BUG: sleeping function called from invalid context
>   #0: ... (&global_shared_desc.spinlock) ...
>   pinctrl_get_device_gpio_range
>   <driver>_gpio_direction_output
>   [ BUG: Invalid wait context ]
> 
> My first draft for this class was to mark the affected gpio_chip as
> can_sleep, but that looks like the wrong contract.  gpio_chip::can_sleep
> describes whether get()/set() may sleep, while the problematic operation
> here is not MMIO value access but an extra pinctrl direction round-trip.
> Rockchip history seems to support that concern: after the controller was
> marked sleeping, a follow-up change stopped calling pinctrl for
> set_direction because whole-chip can_sleep caused atomic get/set
> warnings.
> 
> For PXA and Tegra, I am considering a small series that removes the
> pinctrl_gpio_direction_input/output() calls from the GPIO direction
> callbacks and leaves direction programming on the drivers' existing MMIO
> paths.
> 
> For PXA, the driver already updates GPDR directly in
> pxa_gpio_direction_input/output().  The proposed change would drop the
> additional pinctrl direction call on variants where pxa_gpio_has_pinctrl()
> currently returns true.
> 
> For Tegra, the GPIO driver already programs the GPIO controller direction
> registers directly.  The Tegra pinmux ops appear to provide GPIO
> request/free handling, but no gpio_set_direction hook, so the
> pinctrl_gpio_direction_input/output() call seems to enter the pinctrl core
> without adding a Tegra-specific direction operation.  The proposed change
> would keep pinctrl involvement in request/free but not in GPIO direction.

I looked into this, and yes, we don't provide gpio_set_direction
callbacks for the Tegra pinctrl driver, so what you're proposing looks
fine.

However, I'm on the fence about this because I think conceptually it is
correct to call into the pinctrl subsystem to set the direction. The
GPIO driver should be oblivious to the fact that it isn't strictly
necessary.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]