Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Dec 9, 2023 at 2:56 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Dec 08, 2023 at 07:54:40PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Dec 8, 2023 at 11:27 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> > > On Fri, Dec 08, 2023 at 10:52:09AM +0100, Bartosz Golaszewski wrote:
> > > > 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]
> > > > > >
> > > > >
> > > > > 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.
> > > >
> > >
> > > It would probably take me longer than that to context switch, so go for
> > > it.
> > >
> >
> > Well I went for it and it turns out to be quite tricky. We have
> > linereq and gpio_chardev_data that have independent lifetimes and the
> > only resource they share is the gpio_device and - by extension - gpio
> > descriptors . If we want to store some additional data locally within
> > the context of gpiolib-cdev.c then I see the following options:
> >
>
> Well that probably explains why putting it in the desc made so much
> sense at the time.
>
> Lets take a look at the code...
>
> I had thought it could be moved to struct line (contained within
> struct linereq), so basically replacing line->desc->debounce_period_us
> with line->debounce_period_us.  That almost works, but the problem there
> is that gpio_desc_to_lineinfo() returns the debounce period in the line
> info - and there is no way to access the linereq/line from the desc...
>
> Ah, so the lineinfo_get/_v1() functions that call
> gpio_desc_to_lineinfo() also have the gpio_chardev_data to work with -
> now I see where you are coming from.
> (Debounce is not relevant for v1, so that reduces the problem to
> lineinfo_get().)
>
> > 1. Global radix_tree containing additional configuration
> > (debounce_period_us for now) looked up by the descriptor's address.
> > Lookup can be done locklessly using RCU and from atomic context
> > (interrupt handlers in cdev code).
> >
>
> The irq handlers are already protected from changes to debounce period.
> It is either set before the irqs are enabled (in the request), or the
> irq is disabled, debounce updated, and irq re-enabled (in set_config).
>
> > 2. Reference counted wrapper around descriptors. It would look something like:
> >
> > struct gpio_cdev_desc {
> >     struct kref ref;
> >     struct gpio_desc *desc;
> >     unsigned int debounce_period_us;
> > };
> >
> > And then struct gpio_chardev_data would store an array of pointers to
> > this wrapper while struct line would store a pointer to it instead of
> > directly referencing struct gpio_desc.
> >
> > Any other ideas?
> >
>
> I still think the primary location for any additional line config is in
> struct line - that makes it clear and simple for the majority of cdev and
> matches the lifetimes of the accessors (other than lineinfo_get()).
>
> The only issue being how to access that info from lineinfo_get().
> I guess we can't stop reporting the debounce in the line info :-(.
>
> Rather than worry about an linkage between gpio_chardev_data and the
> linereq/line, I would consider a separate copy of this supplemental line
> info for the chardev, possibly using a radix_tree as you suggest.
> That would be updated by the linereq to match its value in struct line.
> So basically Option 1 but for a shadow copy of the additional info.
>

We still need to connect linereq with its "parent" gpio_chardev_data
somehow and make this link weak so that it can survive one or the
other being destroyed. Maybe a notifier in linereq to which
gpio_chardev_data would subscribe? It would send out notifications on
changes to debounce_period which gpio_chardev_data could store. When
linereq goes out of scope it sends a corresponding notification
allowing gpio_chardev_data to unsubscribe before linereq is freed,
while when gpio_chardev_data goes out of scope first, it unsubscribes
when being released.

Bart

> I'm not a fan of globals, so would go per chip and indexed by line
> offset rather than desc. But then, given the lifetime issues at play, that
> would have to be in a reference counted object that both gpio_chardev_data
> and linereq would reference.  So swings and roundabouts.
>
> Does that make sense? Anyway, that's my $/50.
>
> Cheers,
> Kent.
>