Message-ID: <CAMuHMdVNg5eAtKZ=X7qqnNybk1mS4-5HTVP_ut44D2qEpzkXPA@mail.gmail.com>
Date: Wed, 12 Feb 2025 16:36:13 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Koichiro Den <koichiro.den@...onical.com>
Cc: linux-gpio@...r.kernel.org, brgl@...ev.pl, linus.walleij@...aro.org, 
	maciej.borzecki@...onical.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 08/10] gpio: aggregator: expoose aggregator created via
 legacy sysfs to configfs

Hi Den-san,

Thanks for your patch!

On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@...onical.com> 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@...onical.com>

> @@ -73,6 +76,10 @@ struct gpio_aggregator_line {
>         enum gpio_lookup_flags flags;
>  };
>
> +struct gpio_aggregator_pdev_meta {
> +       bool init_via_sysfs;
> +};

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

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

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

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

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

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

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

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

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

Powered by blists - more mailing lists