Re: [libgpiod v2][PATCH] line-config: rework the internal implementation

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Oct 5, 2021 at 6:20 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Wed, Sep 22, 2021 at 10:11:00AM +0200, Bartosz Golaszewski wrote:
> > This reworks how the line config objects work internally. In order to reduce
> > the size of the gpiod_line_config objects, we switch from using a set number
> > of override config structures with each storing a list of line offsets to
> > storing a smaller override object for each of the maximum of 64 lines.
> > Additionally these internal config structures are now packed and only occupy
> > the minimum required amount of memory.
> >
> > The processing of these new overrides has become a bit more complicated but
> > should actually be more robust wrt corner cases.
> >
> > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
>
> Given the level of rework the diff is harder work than looking at the
> resulting code, but it all looks good to me.
> A few minor comments scattered below.
>

[snip]

> >
> >  GPIOD_API struct gpiod_line_config *gpiod_line_config_new(void)
>
> You don't have any change to gpiod_line_config_free() here, but
> isn't free() guaranteed to be NULL aware, so you can drop the NULL
> check?
> If not, couldn't it be flipped to:
>
>         if (config)
>                 free(config);
>
> Similarly gpiod_line_config_new() could have
>
>         if (config)
>                 gpiod_line_config_reset(config);
>
> and that "if" could be dropped if gpiod_line_config_reset() were NULL
> aware.
>

No, you're right, as long as we don't dereference the pointer, we can
drop the check. Here and elsewhere.

> But the general policy is that gpiod functions are not NULL aware?
>

In general yes, but various free functions are NULL-aware.

[snip]

> >  static struct base_config *
> > -get_base_config_for_offset(struct gpiod_line_config *config,
> > -                        unsigned int offset)
> > +get_base_config_for_reading(struct gpiod_line_config *config,
> > +                        unsigned int offset, unsigned int flag)
> >  {
> > -     struct secondary_config *secondary;
> > -     unsigned int i, j;
> > -
> > -     /*
> > -      * We're looking backwards as the settings get overwritten if set
> > -      * multiple times.
> > -      */
> > -     for (i = config->num_secondary; i > 0; i--) {
> > -             secondary = &config->secondary[i - 1];
> > +     struct override_config *override;
> >
> > -             for (j = 0; j < secondary->num_offsets; j++) {
> > -                     if (secondary->offsets[j] == offset)
> > -                             return &secondary->config;
> > -             }
> > -     }
> > +     override = get_override_by_offset(config, offset);
> > +     if (!override || !(override->override_flags & flag))
> > +             return &config->defaults;
> >
> > -     return NULL;
> > +     return &override->base;
> >  }
> >
>
> Maybe flip the logic around to make it easier to read:
>
>         if (override && (override->override_flags & flag))
>                 return &override->base;
>
>     return &config->defaults;
>
> Similarly in gpiod_line_config_get_output_value().
>

Done.

> >  static void set_direction(struct base_config *config, int direction)
>
> In set_direction() you have a specific case (GPIOD_LINE_DIRECTION_AS_IS)
> that matches the default behaviour.  I generally drop that case and let
> the default handle it, but that is just personal preference.
>
> Similarly in other set_XXX switches.
>

My personal preference is to be explicit and include those default
cases even if only to tell my future self what's happening here.

> > @@ -209,7 +170,7 @@ static void set_direction(struct base_config *config, int direction)
> >  GPIOD_API void
> >  gpiod_line_config_set_direction(struct gpiod_line_config *config, int direction)
> >  {
> > -     set_direction(&config->primary, direction);
> > +     set_direction(&config->defaults, direction);
> >  }
> >
> >  GPIOD_API void
> > @@ -224,13 +185,19 @@ gpiod_line_config_set_direction_subset(struct gpiod_line_config *config,
> >                                      int direction, unsigned int num_offsets,
> >                                      const unsigned int *offsets)
> >  {
> > -     struct secondary_config *secondary;
> > +     struct override_config *override;
> > +     unsigned int i, offset;
> >
> > -     secondary = get_secondary_config(config, num_offsets, offsets);
> > -     if (!secondary)
> > -             return;
> > +     for (i = 0; i < num_offsets; i++) {
> > +             offset = offsets[i];
> >
>
> Worth the effort given it is only used once?
>

Must have been a leftover, thanks.

[snip]

> >
> >  GPIOD_API void
> > @@ -464,20 +448,26 @@ gpiod_line_config_set_active_high_subset(struct gpiod_line_config *config,
> >                                        unsigned int num_offsets,
> >                                        const unsigned int *offsets)
> >  {
> > -     struct secondary_config *secondary;
> > +     struct override_config *override;
> > +     unsigned int i, offset;
> >
> > -     secondary = get_secondary_config(config, num_offsets, offsets);
> > -     if (!secondary)
> > -             return;
> > +     for (i = 0; i < num_offsets; i++) {
> > +             offset = offsets[i];
> > +
> > +             override = get_override_config_for_writing(config, offset);
> > +             if (!override)
> > +                     return;
> >
> > -     secondary->config.active_low = false;
> > +             override->base.active_low = false;
> > +             override->override_flags |= OVERRIDE_FLAG_ACTIVE_LOW;
> > +     }
> >  }
> >
>
> gpiod_line_config_set_active_low_subset() and
> gpiod_line_config_set_active_high_subset() could call a common helper
> that accepts the active_low as a parameter?
>

Makes sense.

[snip]

> > +
> > +static bool override_config_debounce_period_is_equal(struct override_config *a,
> > +                                                  struct override_config *b)
> > +{
> > +     if (base_debounce_period_is_equal(&a->base, b) &&
> > +         ((a->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD) ==
> > +          (b->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD)))
> > +             return true;
> > +
> > +     return false;
> > +}
> > +
>
> To improve readability, flip the order here to test the flag equivalence
> first.
> Particularly wrt the "a" doesn't have debounce overridden case.
>

Done.

[snip]

>
> Don't see anything wrong here, but I'd like to see a bunch of tests to
> cover the corner cases you mentioned, as the bulk of the module complexity
> is here.
> Not that I expect that now.
>

Yes, definitely. Trying to get the gpio-sim module upstream for this
exact reason. Please take a look at this series if you can.

[snip]

>
> Overall it looks really good to me, so no problem with applying it,
> with or without my suggestions.

I've applied most of your suggestions and will squash this into the
big patch, thanks as usual!

Bart