Re: [PATCH] gpio: sim: allocate IDA numbers earlier

[Andy Shevchenko]

On Fri, Apr 23, 2021 at 6:24 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>
> Instead of allocating the device ID number for gpio-sim platform devices
> when the associated configfs item is committed, do it already when the
> item is created. This way we can display the device name even when the
> chip is still pending. Once it's committed the user can easily identify
> the chip by its real device name. This will allow launching concurrent
> user-space test suites with gpio-sim.

Thanks!
With or without below comment addressed:
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> ---
> Hi all! This is a late one for which I'm sorry but I realized that this
> change will allow us to launch test-suites concurrently if we allow the
> user-space to read the device name before the device is created and then
> wait for this specific name to appear in a udev add event.
>
>  drivers/gpio/gpio-sim.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index 92493b98c51b..2e2e6399e453 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -409,6 +409,7 @@ struct gpio_sim_chip_config {
>          * item is 'live'.
>          */
>         struct platform_device *pdev;
> +       int id;
>
>         /*
>          * Each configfs filesystem operation is protected with the subsystem
> @@ -442,7 +443,7 @@ static ssize_t gpio_sim_config_dev_name_show(struct config_item *item,
>         if (pdev)
>                 ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
>         else
> -               ret = sprintf(page, "none\n");
> +               ret = sprintf(page, "gpio-sim.%d\n", config->id);

Wondering if you need to have one place of definition, i.e. "gpio-sim" part.

>         mutex_unlock(&config->lock);
>
>         return ret;
> @@ -724,6 +725,7 @@ static void gpio_sim_chip_config_release(struct config_item *item)
>         struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
>
>         mutex_destroy(&config->lock);
> +       ida_free(&gpio_sim_ida, config->id);
>         kfree_strarray(config->line_names, config->num_line_names);
>         kfree(config);
>  }
> @@ -747,6 +749,12 @@ gpio_sim_config_make_item(struct config_group *group, const char *name)
>         if (!config)
>                 return ERR_PTR(-ENOMEM);
>
> +       config->id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);
> +       if (config->id < 0) {
> +               kfree(config);
> +               return ERR_PTR(config->id);
> +       }
> +
>         config_item_init_type_name(&config->item, name,
>                                    &gpio_sim_chip_config_type);
>         config->num_lines = 1;
> @@ -781,18 +789,12 @@ static int gpio_sim_config_commit_item(struct config_item *item)
>                                                 config->line_names,
>                                                 config->num_line_names);
>
> -       pdevinfo.id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);
> -       if (pdevinfo.id < 0) {
> -               mutex_unlock(&config->lock);
> -               return pdevinfo.id;
> -       }
> -
>         pdevinfo.name = "gpio-sim";
>         pdevinfo.properties = properties;
> +       pdevinfo.id = config->id;
>
>         pdev = platform_device_register_full(&pdevinfo);
>         if (IS_ERR(pdev)) {
> -               ida_free(&gpio_sim_ida, pdevinfo.id);
>                 mutex_unlock(&config->lock);
>                 return PTR_ERR(pdev);
>         }
> @@ -806,15 +808,12 @@ static int gpio_sim_config_commit_item(struct config_item *item)
>  static int gpio_sim_config_uncommit_item(struct config_item *item)
>  {
>         struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> -       int id;
>
>         mutex_lock(&config->lock);
> -       id = config->pdev->id;
>         platform_device_unregister(config->pdev);
>         config->pdev = NULL;
>         mutex_unlock(&config->lock);
>
> -       ida_free(&gpio_sim_ida, id);
>         return 0;
>  }
>
> --
> 2.30.1
>


-- 
With Best Regards,
Andy Shevchenko