Re: [PATCH 5/5] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson]

On Sat, Oct 05, 2024 at 08:45:17PM +0200, Bartosz Golaszewski wrote:
> On Sat, Oct 5, 2024 at 11:54 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Sat, Oct 05, 2024 at 11:42:34AM +0200, Bartosz Golaszewski wrote:
> > > On Sat, Oct 5, 2024 at 9:46 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Oct 04, 2024 at 04:43:26PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > > >
> > > > > There is a problem with gpiod_direction_output/input(), namely the fact
> > > > > that they can be called both from sleeping as well as atomic context. We
> > > > > cannot call the blocking notifier from atomic and we cannot switch to
> > > > > atomic notifier because the pinctrl functions we call higher up the stack
> > > > > take a mutex. Let's instead use a workqueue and schedule a task to emit
> > > > > the event from process context on the unbound system queue for minimal
> > > > > latencies.
> > > > >
> > > >
> > > > So now there is a race between the state of the desc changing and the
> > > > notified reading it?
> > > >
> > >
> > > Theoretically? Well, yes... In practice I don't think this would
> > > matter. But I understand the concern and won't insist if it's a
> > > deal-breaker for you.
> > >
> >
> > I don't like that correctness depends on timing, so this is a deal
> > breaker for me as it stands.  I would like to see the relevant state passed
> > via the notifier chain, rather than assuming it can be pulled from the desc
> > when the notifier is eventually called.
> >
>
> We could potentially still use the workqueue but atomically allocate
> the work_struct in any context, store the descriptor data, timestamp
> etc. (except the info from pinctrl which is rarely modified and would
> be retrieved just before emitting the event in process context) in it
> and pass it to the workqueue which would then put the data into the
> kfifo and free the work_struct. We can enforce ordering of work
> execution so we wouldn't mangle them, userspace would still see the
> events with correct timestamps and in the right order. Does this sound
> like something viable?
>

That is what I had in mind.  The passed/queued state only has to be the
fields subject to change, which isn't all that much.  You have to keep any
queues finite and deal with potential overflows, though that should be
unlikely if they are reasonably sized - and as you noted earlier this is not
a hot path so even "reasonably sized" is probably going to be small.

Cheers,
Kent.