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

Bartosz Golaszewski
2021-04-26T07:17:15Z

On Fri, Apr 23, 2021 at 5:40 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> 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.
>

I applied it as is for 5.13. Feel free to submit a patch if you think
this is useful.

Bart

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