Re: [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Mon, Mar 10, 2025 at 06:46:25PM GMT, Bartosz Golaszewski wrote:
> On Mon, Mar 10, 2025 at 5:28 PM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> >
> 
> [snip!]
> 
> Please remove unnecessary context from responses. You attached
> hundreds of lines of stack traces here. :(

Right, this will never happen. Sorry for inconvenience.

> 
> >
> > Thanks, I've confirmed it. It seems I overlooked it because somehow
> > lockdep and kasan were not enabled for a while.
> >
> > Assuming the v5 patch series rebased onto the latest gpio/for-next
> > 21c853ad9309 ("gpio: adnp: use new line value setter callbacks"),
> > the following follow-up patch should suffice.
> >
> > ------------8<--------------8<---------------
> > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> > index df34d8fcb79a..56f0fde8c843 100644
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -207,7 +207,18 @@ static void aggr_free_lines(struct gpio_aggregator *aggr)
> >
> >         list_for_each_entry_safe(line, tmp, &aggr->list_head, entry) {
> >                 configfs_unregister_group(&line->group);
> > -               aggr_line_del(aggr, line);
> > +               /*
> > +                * Normally, we acquire aggr->lock within the configfs
> > +                * callback. However, in the legacy sysfs interface case,
> > +                * calling configfs_(un)register_group while holding
> > +                * aggr->lock could cause a deadlock. Fortunately, this is
> > +                * unnecessary because the new_device/delete_device path
> > +                * and the module unload path are mutually exclusive,
> > +                * thanks to an explicit try_module_get. That's why this
> > +                * minimal scoped_guard suffices here.
> > +                */
> > +               scoped_guard(mutex, &aggr->lock)
> > +                       aggr_line_del(aggr, line);
> >                 kfree(line->key);
> >                 kfree(line);
> >         }
> > @@ -926,8 +937,6 @@ static void gpio_aggr_device_release(struct config_item *item)
> >  {
> >         struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> >
> > -       guard(mutex)(&aggr->lock);
> > -
> >         /*
> >          * At this point, aggr is neither active nor activating,
> >          * so calling aggr_deactivate() is always unnecessary.
> > @@ -1072,7 +1081,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> >                                                         &line->group);
> >                         if (error)
> >                                 goto err;
> > -                       aggr_line_add(aggr, line);
> > +                       scoped_guard(mutex, &aggr->lock)
> > +                               aggr_line_add(aggr, line);
> >
> >                         error = aggr_add_gpio(aggr, key, U16_MAX, &n);
> >                         if (error)
> > @@ -1101,7 +1111,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> >                                                         &line->group);
> >                         if (error)
> >                                 goto err;
> > -                       aggr_line_add(aggr, line);
> > +                       scoped_guard(mutex, &aggr->lock)
> > +                               aggr_line_add(aggr, line);
> >
> >                         error = aggr_add_gpio(aggr, key, i, &n);
> >                         if (error)
> > @@ -1205,8 +1216,10 @@ static DRIVER_ATTR_WO(new_device);
> >
> >  static void gpio_aggregator_free(struct gpio_aggregator *aggr)
> >  {
> > -       if (aggr_is_activating(aggr) || aggr_is_active(aggr))
> > -               aggr_deactivate(aggr);
> > +       scoped_guard(mutex, &aggr->lock) {
> > +               if (aggr_is_activating(aggr) || aggr_is_active(aggr))
> > +                       aggr_deactivate(aggr);
> > +       }
> >         aggr_free_lines(aggr);
> >         configfs_unregister_group(&aggr->group);
> >         kfree(aggr);
> > ------------8<--------------8<---------------
> >
> >
> > * The second hunk should be squashed into
> >   [PATCH v5 4/9] gpio: aggregator: introduce basic configfs interface
> >
> > * The rest of the hunks should be squashed into
> >   [PATCH v5 8/9] gpio: aggregator: expose aggregator created via legacy sysfs to configfs
> >
> > If you agree with the above approach, I'll send out v6,
> > while also addressing your feedback here:
> > https://lore.kernel.org/all/CAMRc=MdoMKdqyzGMFDa3aMz3h=vfZ0OtwARxY7FdsPKcBu9HQA@xxxxxxxxxxxxxx/
> >
> > Koichiro
> >
> 
> I won't be testing in-line diff chunks. Please, just fix these issues
> and send a v6. Also: please do write some sort of a script to automate
> the testing of this driver if possible. Ideally: add test script to
> selftests.

Sorry for the delayed response, I've been so tied up with other tasks this
week. Ok, I'll introduce a kselftest for gpio-aggregator. Actually I've
wanted that from the beginning.. I believe it should rely on gpio-sim for
convenience, but please let me know if you don't think so.

Thanks.

> 
> Bart