Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Dec 11, 2023 at 11:55 AM Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
>
> 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:
> > >

[snip]

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

No harm but it turns out that gpiod_direction_input() may also end up
calling into .set_config() (via gpio_set_bias()) so we have the same
problem without the _raw variant to bail us out. I don't want to
introduce it for a single legacy driver that is causing trouble.

Adding Aaro to Cc.

Aaro: do you still have the HW to test this driver? I understand the
need to disable interrupts during writing or reading data - when we
are driving the clock line. But do we also absolutely need to hold the
spinlock when setting the direction of the data line to input? Because
if we don't then we could modify the last remaining offender to not
set GPIO direction with a spinlock held and finally make
gpiod_direction_*() functions sleepable.

Bart