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

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

[Linus Walleij]

Hi Runyu,

thanks for your report!

On Thu, Jun 18, 2026 at 5:11 AM Runyu Xiao <runyu.xiao@seu.edu.cn> wrote:

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

Again that is mutex_lock(&pinctrldev_list_mutex); is it not?

If we go with my suggestion in the previous report to just
replace this mutex with a spinlock, I think this issue will
also be solved.

Am I right?

Yours,
Linus Walleij

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

[Runyu Xiao]

Hi,

Thanks for checking.

Thierry, thanks for confirming that Tegra does not currently provide a
gpio_set_direction callback.  I agree with your concern: even if removing
the call is technically safe for the current Tegra pinctrl implementation,
it may be conceptually cleaner for the GPIO driver to keep delegating the
direction request to pinctrl.

Linus, yes, the mutex I am hitting is the pinctrl device list mutex taken
while resolving the GPIO range.  If that lock can be made non-sleeping as
you suggested in the other thread, then this class of direction-path
warnings should be better handled in the pinctrl core instead of by
removing pinctrl_gpio_direction_*() calls from individual GPIO drivers.

I will hold back the PXA/Tegra driver-local patches for now and first
look at whether the pinctrl core locking change covers this direction
callback case as well.

Thanks,
Runyu

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

[Thierry Reding]

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

On Thu, Jun 18, 2026 at 03:25:49PM +0200, Linus Walleij wrote:
> Hi Runyu,
> 
> thanks for your report!
> 
> On Thu, Jun 18, 2026 at 5:11 AM Runyu Xiao <runyu.xiao@seu.edu.cn> wrote:
> 
> > 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)
> 
> Again that is mutex_lock(&pinctrldev_list_mutex); is it not?
> 
> If we go with my suggestion in the previous report to just
> replace this mutex with a spinlock, I think this issue will
> also be solved.
> 
> Am I right?

I'm not sure it's that simple to convert this to a spinlock. One one
hand this lock is taken around calls to mutex_lock(&pctldev->mutex), so
those could sleep and a spinlock would be wrong.

There's one other case where in addition to the nested pctldev->mutex,
we call some of the generic cleanup functions under the lock. Luckily it
looks like all of those should be safe to call under a spinlock since
they don't sleep themselves from what I can tell.

Maybe the locking order of pinctrldev_list_mutex vs. pctldev->mutex can
be changed to avoid that first issue?

Thierry

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