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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, Oct 16, 2024 at 12:12:10PM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 16, 2024 at 11:43 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Wed, Oct 16, 2024 at 11:22:07AM +0200, Bartosz Golaszewski wrote:
> > > On Wed, Oct 16, 2024 at 11:17 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > > > > >
> > > > >
> > > > > You mean, you get a CHANGED_CONFIG event but the debounce value is not
> > > > > in the associated line info?
> > > > >
> > > >
> > > > Correct.
> > > >
> > >
> > > Ok, let me see.
> > >
> >
> > When setting from userspace the issue is that linereq_set_config() setting the
> > direction will emit, quite possibly before the debounce has been set.  The
> > edge_detector_setup() that does set it can also emit, though only if the
> > hardware supports debounce.  And then there could be a race between the
> > notifier being called and the period being set in the supinfo.
> > (the set will probably win that one)
> >
> > Debounce set from the kernel side is going to be an issue as cdev
> > catches and stores the value from userspace to report in the supinfo - that
> > isn't the case for kernel calls to gpiod_set_config().
> >
> > Seems moving the debounce value out of the desc and into cdev, which seemed a
> > good idea at the time, might come back and bite now if it is no longer
> > restricted to being cdev specific.  Now there is an actual reason to
> > store it in the desc :(.
> >
>
> I'm seeing commit:
>
> commit 9344e34e7992fec95ce6210d95ac01437dd327ab
> Author: Kent Gibson <warthog618@xxxxxxxxx>
> Date:   Tue Dec 19 08:41:54 2023 +0800
>
>     gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
>
>     Store the debounce period for a requested line locally, rather than in
>     the debounce_period_us field in the gpiolib struct gpio_desc.
>
>     Add a global tree of lines containing supplemental line information
>     to make the debounce period available to be reported by the
>     GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.
>
>     Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
>     Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx>
>     Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> But it doesn't explain *why* we did this and I don't remember the
> story behind this change.
>
> How bad would it be to go back to storing the debounce setting in the
> descriptor?
>

At the time it was only being used in cdev, and moving it into cdev was
just about not exporting cdev specific stuff to the rest of the kernel.
So it was just tidying up.
But if cdev is now reporting the configuration of the line independent
of whether it was set from userspace or the kernel then it actually
makes more sense for that state to be stored in the desc.

I don't have any objections to that commit being reverted.

Cheers,
Kent.