Message-ID: <CAMRc=Me=um+=uAKxP8enVPQ4gZvKgtsmaT5m0tM_=7-xGRVY3w@mail.gmail.com>
Date: Thu, 20 Mar 2025 16:56:20 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Koichiro Den <koichiro.den@...onical.com>
Cc: linux-gpio@...r.kernel.org, geert+renesas@...der.be, 
	linus.walleij@...aro.org, maciej.borzecki@...onical.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 0/9] Introduce configfs-based interface for gpio-aggregator

On Sat, Mar 15, 2025 at 5:41 PM Koichiro Den <koichiro.den@...onical.com> 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 v6 patch series includes 9 patches:
>
>   Patch#1: Fix an issue that was spotted during v3 preparation.
>            (Not present in gpio/for-next, so retained in v6.)
>   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 #5.
>   Patch#5: Prepare for Patch#6.
>   Patch#6: Expose devices created with sysfs to configfs.
>   Patch#7: Suppress deferred probe for purely configfs-based aggregators.
>   Patch#8: Documentation for the new configfs interface.
>   Patch#9: Selftest for gpio-aggregator.
>
> N.B. This submission is targeting at gpio/for-next and is based on:
>      commit 21c853ad9309 ("gpio: adnp: use new line value setter callbacks")
>
> v5->v6 changes:
>   - Addressed feedback from Bartosz:
>     * Resolved issues spotted with lockdep and kasan.
>     * Added kselftest for gpio-aggregator.
>   - Fixed a memory leak in aggr_free_line() (missing kfree(line->name)).
>   - Fixed a bug I mistakenly added in aggr_parse() (misplaced scnprintf()).
>   - Eliminated a potential lock inversion deadlock by removing
>     gpio_aggregator_lock acquisition in gpio_aggregator_remove_all(), which
>     became unnecessary after the upstream commit 12f65d120350 ("gpio:
>     aggregator: protect driver attr handlers against module unload").
>
> 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@canonical.com/)
>
> v3->v4 changes:
>   - Split 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.
>
> v5: https://lore.kernel.org/all/20250224143134.3024598-1-koichiro.den@canonical.com/
> v4: https://lore.kernel.org/all/20250217143531.541185-1-koichiro.den@canonical.com/
> v3: https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@canonical.com/
> v2: https://lore.kernel.org/all/20250203031213.399914-1-koichiro.den@canonical.com/
> RFC (v1): https://lore.kernel.org/linux-gpio/20250129155525.663780-1-koichiro.den@canonical.com/T/#u
>
>
> 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: 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
>   selftests: gpio: add test cases for gpio-aggregator
>
>  .../admin-guide/gpio/gpio-aggregator.rst      |  107 ++
>  drivers/gpio/Kconfig                          |    2 +
>  drivers/gpio/gpio-aggregator.c                | 1155 ++++++++++++++---
>  tools/testing/selftests/gpio/Makefile         |    2 +-
>  tools/testing/selftests/gpio/config           |    1 +
>  .../testing/selftests/gpio/gpio-aggregator.sh |  723 +++++++++++
>  6 files changed, 1800 insertions(+), 190 deletions(-)
>  create mode 100755 tools/testing/selftests/gpio/gpio-aggregator.sh
>
> --
> 2.45.2
>

It's too late in the cycle for this to make v6.15, let's try again
once v6.15-rc1 is tagged. I've tested it and it now seems to work
well, I've been unable to trigger any issues even with corner cases.

I still see some nits I'd like to see fixed but I think we're close to
getting it into the tree.

Bart

Powered by blists - more mailing lists