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