Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>

[snip]

>
> Because years of technical debt, that's why. :)
>

Speaking of technical debt: you may have noticed that despite stating
I'm almost done last week, I still haven't submitted my locking
rework.

The reason for that is that I'm stuck on some corner-cases related to
the GPIO <-> pinctrl interaction. Specifically the fact that we have
GPIOLIB API functions that may be called from atomic context which may
end up calling into pinctrl where a mutex will be acquired.

An example of that is any of the GPIO chips that don't set the
can_sleep field in struct gpio_chip but still use
gpiochip_generic_config() (e.g. tegra186). We can then encounter the
following situation:

irq_handler() // in atomic context
  gpiod_direction_output() // line is open-drain
    gpio_set_config()
      gpiochip_generic_config()
        pinctrl_gpio_set_config()
          mutex_lock()

Currently we don't take any locks nor synchronize in any other way
(which is wrong as concurrent gpiod_direction_output() and
gpiod_direction_input() will get in each other's way). Using a mutex
will be fine but for non-sleeping chips if we use a spinlock here (no
other choice really) we'll set off fireworks.

One of the ideas I have is using the fact that we already use atomic
bitops in most places. Let's not take locks but add a new flag:
FLAG_SETTING_DIRECTION. Now when we go into
gpiod_direction_output/input(), we test and set it. A subsequent call
will fail with EBUSY or EAGAIN as long as it's set. It will have no
effect on set/get() - any synchronization will be left to the driver.
When we're done, we clear it after setting the relevant direction
flag.

Does this make any sense? There's still the label pointer and debounce
period stored in the descriptor but these are not accessed in atomic
context AFAICT.

Bart