Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson <warthog618@xxxxxxxxx>]

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.

Cheers,
Kent.