Re: [PATCH v4 1/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Dec 18, 2023 at 4:40 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Mon, Dec 18, 2023 at 04:24:50PM +0100, Bartosz Golaszewski wrote:
> > On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> > > 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>
> > > ---
> > >  drivers/gpio/gpiolib-cdev.c | 154 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 132 insertions(+), 22 deletions(-)
> > >
> > > +static inline bool line_is_supplemental(struct line *line)
> >
> > Under v2 I suggested naming this line_has_suppinfo(). Any reason not
> > to do it? I think it's more logical than saying "line is
> > supplemental". The latter makes it seem as if certain line objects
> > would "supplement" some third party with something. What this really
> > checks is: does this line contain additional information.
> >
>
>
> My bad - responded to your first comment and then missed the rest.
>
> Agreed - the naming could be better. Will fix for v5.
>
> > > +{
> > > +       return READ_ONCE(line->debounce_period_us);
> > > +}
> > > +
> > > +static void line_set_debounce_period(struct line *line,
> > > +                                    unsigned int debounce_period_us)
> > > +{
> > > +       bool was_suppl = line_is_supplemental(line);
> > > +
> > > +       WRITE_ONCE(line->debounce_period_us, debounce_period_us);
> > > +
> > > +       if (line_is_supplemental(line) == was_suppl)
> > > +               return;
> > > +
> > > +       if (was_suppl)
> > > +               supinfo_erase(line);
> > > +       else
> > > +               supinfo_insert(line);
> >
> > Could you add a comment here saying it's called with the config mutex
> > taken as at first glance it looks racy but actually isn't?
> >
>
> Sure.  Though it is also covered by the gdev->sem you added, right?
> So the config_mutex is now redundant?
> Should I document it is covered by both?
> Or drop the config_mutex entirely?
>

No! The semaphore is a read-write semaphore and we can have multiple
readers at once. This semaphore only protects us from the chip being
set to NULL during a system call. It will also go away once I get the
descriptor access serialized (or not - I'm figuring out a lockless
approach) and finally use SRCU to protect gdev->chip.

> And you wanted some comments to explain the logic?
> I thought this is a common "has it changed" pattern, and so didn't
> require additional explanation, but I guess not as common as I thought.
>

If line_set_debounce_period() could be called concurrently for the
same line, this approach would be racy. It cannot but I want a comment
here as I fear that if in the future we add some more attributes that
constitute "supplemental info" and which may be changed outside of the
config lock then this would in fact become racy.

Bart

> Cheers,
> Kent.