Re: [PATCH v2 02/10] gpio: aggregator: introduce basic configfs interface

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Den-san,

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> The existing sysfs 'new_device' interface has several limitations:
> * No way to determine when GPIO aggregator creation is complete.
> * No way to retrieve errors when creating a GPIO aggregator.
> * No way to trace a GPIO line of an aggregator back to its
>   corresponding physical device.
> * The 'new_device' echo does not indicate which virtual gpiochip<N>
>   was created.
> * No way to assign names to GPIO lines exported through an aggregator.
>
> Introduce the new configfs interface for gpio-aggregator to address
> these limitations. It provides a more streamlined, modern, and
> extensible configuration method. For backward compatibility, the
> 'new_device' interface and its behaviour is retained for now.
>
> This commit implements minimal functionalities:
>
>   /config/gpio-aggregator/<name-of-your-choice>/
>   /config/gpio-aggregator/<name-of-your-choice>/live
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
>
> Basic setup flow is:
> 1. Create a directory for a GPIO aggregator.
> 2. Create subdirectories for each line you want to instantiate.
> 3. In each line directory, configure the key and offset.
>    The key/offset semantics are as follows:
>    * If offset is >= 0:
>      - key specifies the name of the chip this GPIO belongs to
>      - offset specifies the line offset within that chip.
>    * If offset is <0:
>      - key needs to specify the GPIO line name.
> 4. Return to the aggregator's root directory and write '1' to the live
>    attribute.
>
> For example, the command in the existing kernel doc:
>
>   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
>
> is equivalent to:
>
>   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
>   # Change <custom-name> to name of your choice (e.g. "aggr0")
>   cd /sys/kernel/config/gpio-aggregator/<custom-name>
>   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
>   echo e6052000.gpio > line0/key
>   echo 19            > line0/offset
>   echo e6050000.gpio > line1/key
>   echo 20            > line1/offset
>   echo e6050000.gpio > line2/key
>   echo 21            > line2/offset
>   echo 1             > live
>
> Signed-off-by: Koichiro Den <koichiro.den@xxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -9,10 +9,14 @@
>
>  #include <linux/bitmap.h>
>  #include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/configfs.h>

Using configfs requires CONFIG_CONFIGFS_FS.
So either GPIO_AGGREGATOR should select CONFIGFS_FS, or
all configfs-related code should be protected by checks for
CONFIG_CONFIGFS_FS.  Since we want to encourage people to use the new
API, I think the former is preferred.

>  #include <linux/ctype.h>
>  #include <linux/delay.h>
>  #include <linux/idr.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/lockdep.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -34,11 +38,39 @@
>   */
>
>  struct gpio_aggregator {
> +       struct config_group group;
>         struct gpiod_lookup_table *lookups;
>         struct platform_device *pdev;
> +       struct mutex lock;
> +       int id;
> +
> +       /* Synchronize with probe */
> +       struct notifier_block bus_notifier;
> +       struct completion probe_completion;
> +       bool driver_bound;
> +
> +       /* List of gpio_aggregator_line. Always added in order */
> +       struct list_head list_head;
> +
>         char args[];
>  };
>
> +struct gpio_aggregator_line {
> +       struct config_group group;
> +       struct gpio_aggregator *parent;
> +       struct list_head entry;
> +
> +       /* Line index within the aggregator device */
> +       int idx;

unsigned int

> +
> +       /* GPIO chip label or line name */
> +       char *key;
> +       /* Can be negative to indicate lookup by line name */
> +       int offset;
> +
> +       enum gpio_lookup_flags flags;
> +};
> +
>  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
>  static DEFINE_IDR(gpio_aggregator_idr);

> +static bool aggr_is_active(struct gpio_aggregator *aggr)
> +{
> +       lockdep_assert_held(&aggr->lock);
> +
> +       return !!aggr->pdev && platform_get_drvdata(aggr->pdev);

No need for "!!".

> +}

> +static void aggr_line_add(struct gpio_aggregator *aggr,
> +                         struct gpio_aggregator_line *line)
> +{
> +       struct gpio_aggregator_line *tmp;
> +
> +       lockdep_assert_held(&aggr->lock);
> +
> +       list_for_each_entry(tmp, &aggr->list_head, entry) {
> +               if (WARN_ON(tmp->idx == line->idx))

Can this really happen?

> +                       return;
> +               if (tmp->idx > line->idx) {
> +                       list_add_tail(&line->entry, &tmp->entry);
> +                       return;
> +               }
> +       }
> +       list_add_tail(&line->entry, &aggr->list_head);
> +}

> +static void aggr_lockup_configfs(struct gpio_aggregator *aggr, bool lock)
> +{
> +       struct configfs_subsystem *subsys = aggr->group.cg_subsys;
> +       struct gpio_aggregator_line *line;
> +
> +       /*
> +        * The device only needs to depend on leaf lines. This is
> +        * sufficient to lock up all the configfs entries that the
> +        * instantiated, alive device depends on.
> +        */
> +       list_for_each_entry(line, &aggr->list_head, entry) {
> +               if (lock)
> +                       WARN_ON(configfs_depend_item_unlocked(
> +                                       subsys, &line->group.cg_item));

Can this happen?
I am also worried if this can be triggered by the user, combined
with panic_on_warn.

> +               else
> +                       configfs_undepend_item_unlocked(
> +                                       &line->group.cg_item);
> +       }
> +}
> +
> +static ssize_t
> +gpio_aggr_line_key_show(struct config_item *item, char *page)
> +{
> +       struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
> +       struct gpio_aggregator *aggr = line->parent;
> +
> +       guard(mutex)(&aggr->lock);
> +
> +       return sprintf(page, "%s\n", line->key ?: "");

Please use sysfs_emit() instead (everywhere).


> +}

> +static ssize_t
> +gpio_aggr_device_live_store(struct config_item *item, const char *page,
> +                           size_t count)
> +{
> +       struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> +       int ret = 0;
> +       bool live;
> +
> +       ret = kstrtobool(page, &live);
> +       if (ret)
> +               return ret;
> +
> +       if (live)
> +               aggr_lockup_configfs(aggr, true);
> +
> +       scoped_guard(mutex, &aggr->lock) {
> +               if (live == aggr_is_active(aggr))
> +                       ret = -EPERM;
> +               else if (live)
> +                       ret = aggr_activate(aggr);
> +               else
> +                       aggr_deactivate(aggr);
> +       }
> +
> +       /*
> +        * Undepend is required only if device disablement (live == 0)

s/Undepend/Lock-up/?

> +        * succeeds or if device enablement (live == 1) fails.
> +        */
> +       if (live == !!ret)
> +               aggr_lockup_configfs(aggr, false);
> +
> +       return ret ?: count;
> +}

> +static struct config_group *
> +gpio_aggr_make_group(struct config_group *group, const char *name)
> +{
> +       /* arg space is unneeded */
> +       struct gpio_aggregator *aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
> +       if (!aggr)
> +               return ERR_PTR(-ENOMEM);
> +
> +       mutex_lock(&gpio_aggregator_lock);
> +       aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> +       mutex_unlock(&gpio_aggregator_lock);
> +
> +       if (aggr->id < 0) {
> +               kfree(aggr);
> +               return ERR_PTR(aggr->id);
> +       }

The above more or less duplicates the existing code in
new_device_store().

> +
> +       INIT_LIST_HEAD(&aggr->list_head);
> +       mutex_init(&aggr->lock);
> +       config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
> +       init_completion(&aggr->probe_completion);
> +
> +       return &aggr->group;
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds