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

[Koichiro Den]

On Mon, Mar 10, 2025 at 11:19:40AM GMT, Bartosz Golaszewski wrote:
> On Mon, Feb 24, 2025 at 3:31 PM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> >
> > This patch series introduces a configfs-based interface to gpio-aggregator
> > to address limitations in the existing 'new_device' interface.
> >
> > The existing 'new_device' interface has several limitations:
> >
> >   Issue#1. No way to determine when GPIO aggregator creation is complete.
> >   Issue#2. No way to retrieve errors when creating a GPIO aggregator.
> >   Issue#3. No way to trace a GPIO line of an aggregator back to its
> >            corresponding physical device.
> >   Issue#4. The 'new_device' echo does not indicate which virtual
> >            gpiochip<N> was created.
> >   Issue#5. No way to assign names to GPIO lines exported through an
> >            aggregator.
> >
> > Although Issue#1 to #3 could technically be resolved easily without
> > configfs, using configfs offers a streamlined, modern, and extensible
> > approach, especially since gpio-sim and gpio-virtuser already utilize
> > configfs.
> >
> > This v5 patch series includes 9 patches:
> >
> >   Patch#1: Fix an issue that was spotted during v3 preparation.
> >   Patch#2: Reorder functions to prepare for configfs introduction.
> >   Patch#3: Add aggr_alloc() to reduce code duplication.
> >   Patch#4: Introduce basic configfs interface. Address Issue#1 to #4.
> >   Patch#5: Address Issue#5.
> >   Patch#6: Prepare for Patch#7.
> >   Patch#7: Expose devices created with sysfs to configfs.
> >   Patch#8: Suppress deferred probe for purely configfs-based aggregators.
> >   Patch#9: Documentation for the new configfs interface.
> >
> > N.B. This v5 is based on the latest gpio/for-next commit as of writing this:
> >      * 45af02f06f69 ("gpio: virtuser: convert to use dev-sync-probe utilities")
> >
> >
> > v4->v5 changes:
> >   - Rebased off of the latest gpio/for-next, that includes the patch series:
> >     "Add synchronous fake device creation utility for GPIO drivers"
> >     (https://lore.kernel.org/all/20250221133501.2203897-1-koichiro.den@xxxxxxxxxxxxx/)
> >
> > v3->v4 changes:
> >   - Splitted off the introduction of gpio-pseudo.[ch] and conversions.
> >   - Reordered commits to place a fix commit first.
> >   - Squashed the trivial update for gpio-aggregator's conversion to gpio-pseudo
> >     into the primary commit "gpio: aggregator: introduce basic configfs interface"
> >     as it is only meaningful when combined.
> >
> > v2->v3 changes:
> >   - Addressed feedback from Bartosz:
> >     * Factored out the common mechanism for synchronizing platform device
> >       probe by adding gpio-pseudo.[ch].
> >     * Renamed "_auto." prefix to "_sysfs." for auto-generated
> >       configfs entries corresponding to sysfs-created devices.
> >     * Squashed v2 Patch#3 into its predecessor.
> >   - Addressed feedback from Geert:
> >     * Factored out duplicate code in struct gpio_aggregator initialization
> >       by adding gpio_alloc()/gpio_free() functions. Note that v2 Patch#7
> >       was dropped for other reasons as mentioned below, so aggr_free() in
> >       v3 is unrelated to the same-named function in v2.
> >     * Removed redundant parsing of gpio-line-names and unnecessary
> >       chip->names assignments; squashed v2 Patch#4 + v2 Patch#5 into v3
> >       Patch#9.
> >     * Updated to use sysfs_emit().
> >     * Updated Kconfig (select CONFIGFS_FS).
> >     * Fixed typos, coding style issues, missing const qualifiers, and other
> >       minor issues.
> >   - Resolved an issue that was spotted during v3 preparation. See Patch#2.
> >   - Reordered resource initialization order in gpio_aggregator_init() to
> >     both eliminate a potential race condition (as noted in the source code
> >     comment) and simplify the code. See Patch#8. This enabled:
> >     * Removal of v2 Patch#7.
> >     * Merging of aggr_unregister_lines() and aggr_free_lines() into a
> >       unified function.
> >   - Disabled 'delete_device' functionality for devices created via configfs
> >     for simplicity. It was mistakenly allowed in v2 and proved buggy. See
> >     Patch #8.
> >
> > RFC->v2 changes:
> >   - Addressed feedback from Bartosz:
> >     * Expose devices created with sysfs to configfs.
> >     * Drop 'num_lines' attribute.
> >     * Fix bugs and crashes.
> >     * Organize internal symbol prefixes more cleanly.
> >   - Split diffs for improved reviewability.
> >   - Update kernel doc to reflect the changes.
> >
> > v4: https://lore.kernel.org/all/20250217143531.541185-1-koichiro.den@xxxxxxxxxxxxx/
> > v3: https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@xxxxxxxxxxxxx/
> > v2: https://lore.kernel.org/all/20250203031213.399914-1-koichiro.den@xxxxxxxxxxxxx/
> > RFC (v1): https://lore.kernel.org/linux-gpio/20250129155525.663780-1-koichiro.den@xxxxxxxxxxxxx/T/#u
> >
> >
> > *** BLURB HERE ***
> >
> > Koichiro Den (9):
> >   gpio: aggregator: protect driver attr handlers against module unload
> >   gpio: aggregator: reorder functions to prepare for configfs
> >     introduction
> >   gpio: aggregator: add aggr_alloc()/aggr_free()
> >   gpio: aggregator: introduce basic configfs interface
> >   gpio: aggregator: add 'name' attribute for custom GPIO line names
> >   gpio: aggregator: rename 'name' to 'key' in aggr_parse()
> >   gpio: aggregator: expose aggregator created via legacy sysfs to
> >     configfs
> >   gpio: aggregator: cancel deferred probe for devices created via
> >     configfs
> >   Documentation: gpio: document configfs interface for gpio-aggregator
> >
> >  .../admin-guide/gpio/gpio-aggregator.rst      |  107 ++
> >  drivers/gpio/Kconfig                          |    2 +
> >  drivers/gpio/gpio-aggregator.c                | 1129 ++++++++++++++---
> >  3 files changed, 1050 insertions(+), 188 deletions(-)
> >
> > --
> > 2.45.2
> >
> 
> I did some more testing as I want to pick it up for v6.15 but I now
> noticed that we're hitting the lockdep_assert_held(&aggr->lock) splat
> in aggr_line_add(). Could you please look into it?

Thanks. Could you share with me a sample splat?

Koichiro

> 
> Bartosz