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

[Kent Gibson <warthog618@gmail.com>]

On Thu, Oct 17, 2024 at 05:04:13PM +0200, Bartosz Golaszewski wrote:
> On Thu, Oct 17, 2024 at 5:02 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 04:59:46PM +0200, Bartosz Golaszewski wrote:
> > > On Thu, Oct 17, 2024 at 4:20 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Thu, Oct 17, 2024 at 04:14:24PM +0200, Bartosz Golaszewski wrote:
> > > > > On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > > >
> > > > > > > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > > >               }
> > > > > > >
> > > > > > >               WRITE_ONCE(line->edflags, edflags);
> > > > > > > -
> > > > > > > -             gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > > >       }
> > > > > > >       return 0;
> > > > > > >  }
> > > > > >
> > > > > > I still get errors from this when reconfiguring lines with debounce.
> > > > > > You should leave this notify in place and use _nonotify when setting the
> > > > > > direction.
> > > > > > i.e.
> > > > > >
> > > > > > @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > >                         int val = gpio_v2_line_config_output_value(&lc, i);
> > > > > >
> > > > > >                         edge_detector_stop(line);
> > > > > > -                       ret = gpiod_direction_output(desc, val);
> > > > > > +                       ret = gpiod_direction_output_nonotify(desc, val);
> > > > > >                         if (ret)
> > > > > >                                 return ret;
> > > > > >                 } else {
> > > > > > -                       ret = gpiod_direction_input(desc);
> > > > > > +                       ret = gpiod_direction_input_nonotify(desc);
> > > > > >                         if (ret)
> > > > > >                                 return ret;
> > > > > >
> > > > > > @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > >                 }
> > > > > >
> > > > > >                 WRITE_ONCE(line->edflags, edflags);
> > > > > > +
> > > > > > +               gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > >         }
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > Given that, all my current tests are passing.
> > > > > >
> > > > >
> > > > > That looks good - after all we no longer notify from any place in
> > > > > gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
> > > > > Are you getting more events with debounce? Are you not getting any?
> > > > >
> > > >
> > > > In linereq_set_config(), the notify comes from the gpiod_direction_input() -
> > > > before the edge_detector_setup() is called (not visible in the patch) and that
> > > > sets the debounce value in the desc.
> > > > So you get an event without the debounce set, or with a stale value.
> > > > Keeping the gpiod_line_state_notify() and using the _nonotify()
> > > > functions means the notify comes after the debounce has been set.
> > > >
> > >
> > > I see. I guess I should do the same both for linehandle_set_config()
> > > and linereq_set_config()?
> > >
> >
> > linehandles don't support debounce, so it's good as is.
> >
>
> Right, but I'm wondering if it isn't better for consistency.
> Otherwise, someone may ask themselves why there's no event being
> emitted if they're not familiar with the gpiod_direction_*()
> internals.
>

I prefer the more succinct form myself, and if you are working in the
kernel you should be capable of determining what the functions do, but
whichever works for you.

Cheers,
Kent.