Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sun, Dec 10, 2023 at 2:28 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Sun, Dec 10, 2023 at 10:34:47AM +0800, Kent Gibson wrote:
> > > >
> > >
> > > 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.
> > >
> >
> > No, there has to be a link between both and the supplemental info.
> > For gpio_chardev_data that is to create lineinfo, and for the linereq it
> > is to keep the value reported in lineinfo mirroring the current value.
> > Below I suggested making the supplemental info a reference counted
> > object, with chip scope, referenced by both gpio_chardev_data and the
> > linereq. So last one out turns off the lights.
> >
> > Having the shadow copy allows most usage to avoid the tree lookup and any
> > associated locking (assuming the tree isn't inherently thread safe and
> > requires a spinlock to prevent modification during a lookup).
> > It is only populating the lineinfo or updating the value that would
> > require the lookup, and neither are a hot path (if there is such a thing
> > in cdev).
> >
> > Hmmm, the radix_tree allocates a page of entries at a time, which might
> > be a bit of overkill per-chip, so maybe a global is the way to go?
> > Or something other than a radix_tree, say a rbtree?
> >
> > All this is getting rather complicated just to relocate one field, so I'm
> > starting to reconsider whether the desc was the right place for it after
> > all. ¯\_(ツ)_/¯
> >
> > OTOH, I've partially coded my suggestion, to the point of skeletons for
> > the supplemental info, so if you like I'm happy to finish that off and
> > provide patches.  Though what remains is probably 90% of the work...
> >
>
> Bah, just ignore me wrt the supplemental info per chip.
> That solution only works for the chip fd used to request the lines.
> If you close the chip and re-open it there will be no connection between
> the new gpio_chardev_data and the existing line requests or the
> supplemental info.
>
> So it would have to be a global indexed by desc as you suggested.
> Well that sucks.
>

Yeah, I don't see any other way if we want to contain this within
gpiolib-cdev.c. I tried fiddling with notifiers but it went nowhere.
:(

Bart

> Cheers,
> Kent.
>