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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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. :(

>
> 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.

Bart