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

[Bartosz Golaszewski]

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

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