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