Re: [PATCH v2 08/10] gpio: aggregator: expoose aggregator created via legacy sysfs to configfs

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Wed, Feb 12, 2025 at 04:36:13PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> Thanks for your patch!
> 
> On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> > Expose settings for aggregators created using the sysfs 'new_device'
> > interface to configfs. Once written to 'new_device', an "_auto.<N>" path
> > appears in the configfs regardless of whether the probe succeeds.
> > Consequently, users can no longer use that prefix for custom GPIO
> > aggregator names. The 'live' attribute changes to 1 when the probe
> > succeeds and the GPIO forwarder is instantiated.
> >
> > Note that the aggregator device created via sysfs is asynchrnous, i.e.
> 
> asynchronous
> 
> > writing into 'new_device' returns without waiting for probe completion,
> > and the probe may succeed, fail, or eventually succeed via deferred
> > probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
> > without notice. So, editting key/offset/name while it's waiting for
> 
> editing
> 
> > deferred probe is prohibited.
> >
> > The configfs auto-generation relies on create_default_group(), which
> > inherently prohibits rmdir(2). To align with the limitation, this commit
> > also prohibits mkdir(2) for them. When users want to change the number
> > of lines for an aggregator initialized via 'new_device', they need to
> > tear down the device using 'delete_device' and reconfigure it from
> > scratch. This does not break previous behaviour; users of legacy sysfs
> > interface simply gain additional almost read-only configfs exposure.
> >
> > Still, users can write into 'live' attribute to toggle the device unless
> 
> write to the
> 
> > it's waiting for deferred probe. So once probe succeeds, they can
> > deactivate it in the same manner as the devices initialized via
> > configfs.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@xxxxxxxxxxxxx>
> 
> > @@ -73,6 +76,10 @@ struct gpio_aggregator_line {
> >         enum gpio_lookup_flags flags;
> >  };
> >
> > +struct gpio_aggregator_pdev_meta {
> > +       bool init_via_sysfs;
> > +};

Thanks for pointing out all of them above. Will fix them all in v3.

> 
> The use of this structure to indicate the instantiation method looks
> a bit hacky to me, but I don't see a better way...

I agree.

> 
> > +
> >  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
> >  static DEFINE_IDR(gpio_aggregator_idr);
> >
> > @@ -127,6 +134,14 @@ static bool aggr_is_active(struct gpio_aggregator *aggr)
> >         return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
> >  }
> >
> > +/* Only aggregators created via legacy sysfs can be "activating". */
> > +static bool aggr_is_activating(struct gpio_aggregator *aggr)
> > +{
> > +       lockdep_assert_held(&aggr->lock);
> > +
> > +       return !!aggr->pdev && !platform_get_drvdata(aggr->pdev);
> 
> No need for "!!".

Will fix in v3.

> 
> > +}
> > +
> >  static size_t aggr_count_lines(struct gpio_aggregator *aggr)
> >  {
> >         lockdep_assert_held(&aggr->lock);
> 
> > @@ -1002,6 +1048,14 @@ gpio_aggr_make_group(struct config_group *group, const char *name)
> >         if (!aggr)
> >                 return ERR_PTR(-ENOMEM);
> >
> > +       /*
> > +        * "_auto" prefix is reserved for auto-generated config group
> > +        * for devices create via legacy sysfs interface.
> > +        */
> > +       if (strncmp(name, AGGREGATOR_LEGACY_PREFIX,
> > +                   sizeof(AGGREGATOR_LEGACY_PREFIX)) == 0)
> > +               return ERR_PTR(-EINVAL);
> 
> Missing kfree(aggr) in case of failure.

My bad. Will fix in v3.

> 
> > +
> >         mutex_lock(&gpio_aggregator_lock);
> >         aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> >         mutex_unlock(&gpio_aggregator_lock);
> > @@ -1044,6 +1098,8 @@ static struct configfs_subsystem gpio_aggr_subsys = {
> >  static int aggr_parse(struct gpio_aggregator *aggr)
> >  {
> >         char *args = skip_spaces(aggr->args);
> > +       struct gpio_aggregator_line *line;
> > +       char name[CONFIGFS_ITEM_NAME_LEN];
> >         char *key, *offsets, *p;
> >         unsigned int i, n = 0;
> >         int error = 0;
> > @@ -1055,14 +1111,29 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> >
> >         args = next_arg(args, &key, &p);
> >         while (*args) {
> > +               scnprintf(name, CONFIGFS_ITEM_NAME_LEN, "line%u", n);
> 
> sizeof(name), to protect against future changes of name[].

Right, will fix it in v3.

> 
> >  static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >                                 size_t count)
> >  {
> > +       struct gpio_aggregator_pdev_meta meta;
> 
> You might as well pre-initialize this:
> 
>    = { .init_via_sysfs = true };
>

Will do so in v3.

> > +       char name[CONFIGFS_ITEM_NAME_LEN];
> >         struct gpio_aggregator *aggr;
> >         struct platform_device *pdev;
> >         int res, id;
> > @@ -1112,6 +1210,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >
> >         memcpy(aggr->args, buf, count + 1);
> >
> > +       aggr->init_via_sysfs = true;
> >         aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
> >                                 GFP_KERNEL);
> >         if (!aggr->lookups) {
> > @@ -1128,10 +1227,22 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >                 goto free_table;
> >         }
> >
> > +       scnprintf(name, CONFIGFS_ITEM_NAME_LEN,
> 
> sizeof(name)

Will fix in v3.

> 
> > +                 "%s.%d", AGGREGATOR_LEGACY_PREFIX, id);
> > +       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);
> 
> The code above is now almost identical to gpio_aggr_make_group().

(As I replied to your feedback to [PATCH v2 02/10]) I'll factor out the
common part and add new funcs, then use them in each.

> 
> > +
> > +       /* Expose to configfs */
> > +       res = configfs_register_group(&gpio_aggr_subsys.su_group, &aggr->group);
> > +       if (res)
> > +               goto remove_idr;
> > +
> >         aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> >         if (!aggr->lookups->dev_id) {
> >                 res = -ENOMEM;
> > -               goto remove_idr;
> > +               goto unregister_group;
> >         }
> >
> >         res = aggr_parse(aggr);
> > @@ -1140,7 +1251,9 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >
> >         gpiod_add_lookup_table(aggr->lookups);
> >
> > -       pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
> > +       meta.init_via_sysfs = true;
> > +
> > +       pdev = platform_device_register_data(NULL, DRV_NAME, id, &meta, sizeof(meta));
> >         if (IS_ERR(pdev)) {
> >                 res = PTR_ERR(pdev);
> >                 goto remove_table;
> 
> > @@ -1258,7 +1379,26 @@ static struct platform_driver gpio_aggregator_driver = {
> >
> >  static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
> >  {
> > -       aggr_free(p);
> > +       /*
> > +        * There should be no aggregator created via configfs, as their
> > +        * presence would prevent module unloading.
> > +        */
> > +       struct gpio_aggregator *aggr = (struct gpio_aggregator *)p;
> 
> The cast is not needed.

Will fix in v3.

Thanks!

Koichiro

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