Re: GPIOLIB locking is broken and how to fix it

[Thierry Reding <thierry.reding@gmail.com>]

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

On Fri, Dec 08, 2023 at 02:12:45PM +0100, Linus Walleij wrote:
> On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> 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()

Isn't this a very special case already? cbus_transfer() holds the spin
lock across the entire function, so it will only work for a very small
set of GPIO providers anyway, right? Anything that's sleepable just is
not going to work. I suspect that direction configuration is then also
not going to sleep, so this should be fine.

Thierry

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