On Fri, Dec 08, 2023 at 07:30:36PM +0100, Bartosz Golaszewski wrote: > On Fri, Dec 8, 2023 at 5:41 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > > > > On Fri, Dec 08, 2023 at 03:47:00PM +0100, Bartosz Golaszewski wrote: > > > On Fri, Dec 8, 2023 at 2:56 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > > > > > > > > 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@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() > > > > > > > > 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. > > > > > > > > > > Maybe we could switch to using gpiod_direction_*_raw() here and then > > > mark regular gpiod_direction_input/output() as might_sleep() and be > > > done with it? Treat this one as a special-case and then not accept > > > anyone new calling these from atomic context? > > > > Yeah, I2C CBUS already uses gpiod_set_value() in the same context as > > gpiod_direction_output()/gpiod_direction_input(), so it would've already > > warned about a mismatch anyway. Doing a test-run with the regular > > direction accessors marked as might_sleep() should flush out any other > > abusers. > > > > Thierry > > We cannot possibly test all drivers out there but we can start out by > adding: `WARN_ON(in_atomic())` to the direction setters and getters > for the next release and then possibly switch to a full might_sleep() > if nobody complains? What's the harm in using might_sleep() right away? If there's a lot of problems we need to back out the change anyway, so whether we back out the WARN_ON() or the might_sleep() doesn't really make a difference. Thierry
Attachment:
signature.asc
Description: PGP signature
