Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson <warthog618@gmail.com>]

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@gmail.com> 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@bgdev.pl> 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?

Cheers,
Kent.