[PATCH] gpio: sim: allocate IDA numbers earlier

[PATCH] gpio: sim: allocate IDA numbers earlier

[Bartosz Golaszewski]

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.

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

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

[Andy Shevchenko]

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

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

[Bartosz Golaszewski]

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