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

Andy Shevchenko
2021-04-23T15:40:38Z

On Fri, Apr 23, 2021 at 6:24 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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@gmail.com>

> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
> 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