Re: GPIOLIB locking is broken and how to fix it

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:

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

OK I see the problem.

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

The only thing that really make sense to protect from here is
concurrent access to the same register (such as if a single
register contains multiple bits to set a number of GPIOs at
output or input).

The real usecases for gpiod_direction_* I know of are limited to:

1. Once when the GPIO is obtained.

2. In strict sequence switching back and forth as in
    drivers/i2c/busses/i2c-cbus-gpio.c
    cbus_transfer()

But *two* execution contexts contesting over *the same* GPIO?
I've never heard of that one. But I'm not sure that is what you mean
to address? Sounds like a theoretical problem to me.

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

Given that I think the situation is entirely theoretical I'm certainly
happy with that solution.

Whoever want to do this crazy thing can very well teach their code
to recover from errors as well...

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

Sounds fair to me, if it's even a problem. But I trust you, so if you
think this is needed, I'm game!

Yours,
Linus Walleij