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

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Wed, Feb 12, 2025 at 02:48:12PM GMT, Geert Uytterhoeven wrote:
> 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.

Indeed. I had mentioned this in the response to Bartosz here:
https://lore.kernel.org/all/dmy4mvxut3l5kqds2b2fnnes5ukr73spddwgrbkeoqrb5p5wir@hkq6ltr7d6dt/

I agree with the former.

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

Thanks. I'll fix this in v3.

> 
> > +
> > +       /* 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 "!!".

I'll fix this in v3.

> 
> > +}
> 
> > +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?

I don't think so. It was just a safeguard for the future to help us notice
when something very bad would happen, caused by changes on codebase. So let
me drop it in v3.

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

I don't think so. This was just a safeguard for the future.

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

Thanks for pointing it out. I'll fix all of them in v3.

> 
> > +}
> 
> > +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/?

I must admit that I couldn't find a best suitable antonym to 'depend'.
IMO Lock-up sounds a bit misleading. How about 'Unlock'?

> 
> > +        * 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().

I'll factor out the common part and add new funcs gpio_alloc()/gpio_free().
Please let me know if any objections.

> 
> > +
> > +       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,

Thanks for the review.

Koichiro

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