Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Dec 8, 2023 at 9:38 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> > > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > > 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.
> > > >
> > >
> > > To be clear, that is an existing pinctrl mutex?
> >
> > Yes, I'm talking about pctldev->mutex. TBH set_config IMO should never
> > be called from atomic context but that's already there and will be
> > hard to change it now. :(
> >
> > >
> > > > 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()
> > > >
> > >
> > > Isn't using a mutex (the pinctrl one here) from atomic always a
> > > problem?  Shouldn't this flow be handed off to irq_thread()?
> > >
> >
> > This is a corner-case. Typically we won't be calling gpio_set_config()
> > from gpiod_direction_output(). This only happens if the line has
> > special config like open-source/open-drain. Any other places where we
> > may end up calling into pinctrl is request() and free() and those
> > already might sleep.
> >
>
> Why does it matter that it is a corner case?
> So it is currently possible for that corner case to hit a mutex within
> atomic context??
>
> > > > 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.
> > > >
> > >
> > > Makes sense to me, as it is basically the sub-state solution I suggested
> > > earlier for request/release, but applied to direction.  Not sure about
> > > the contention behaviour though, as that is something userspace will
> > > see and might not be expecting.
> > >
> >
> > User-space will never call from atomic context.
>
> Don't you need to do the test and set in either case?
>
> > We could potentially
> > use a work-queue here even for sleeping chips and serialize the
> > operations
> >
> > > OTOH I'm starting to think that serialising callbacks might be a good idea
> > > - unless it is crystal clear to the driver authors that the callbacks may
> > > be called concurrently.
> >
> > This was my initial idea: use mutex for sleeping chips, spinlock for
> > non-sleeping ones and make it possible for drivers to disable locking
> > entirely. Except that we cannot use spinlock for chips interacting
> > with pinctrl at all even if they can never sleep. :/
> >
> > >
> > > The debounce is really a cdev field.  Putting it in the desc made sense
> > > to me at the time as it is per-line, not per-request, but perhaps it
> > > should moved into the cdev linereq, even if that means having to alloc
> > > space for it, just to get it out of your hair.
> > >
> >
> > This sounds good actually.
> >
>
> Yeah, no need to risk other GPIO users messing with it if it is only there
> for cdev.
> Want me to take a look at it or are you happy to take care of it?
>

If you'll find the time to do it in the following days then sure, go
ahead, otherwise, I'll have some pare cycles today and next week to
spend on it.

Bart

> Cheers,
> Kent.