[v2,1/3] gpio: introduce utilities for synchronous fake device creation

Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
platform device, wait for probe completion, and retrieve the probe
success or error status synchronously. With gpio-aggregator planned to
adopt this approach for its configfs interface, it's time to factor
out the common code.

Add dev-sync-probe.[ch] to house helper functions used by all such
implementations.

No functional change.

Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/Kconfig          |  7 +++
 drivers/gpio/Makefile         |  3 ++
 drivers/gpio/dev-sync-probe.c | 96 +++++++++++++++++++++++++++++++++++
 drivers/gpio/dev-sync-probe.h | 25 +++++++++
 4 files changed, 131 insertions(+)
 create mode 100644 drivers/gpio/dev-sync-probe.c
 create mode 100644 drivers/gpio/dev-sync-probe.h

On Thu, Feb 20, 2025 at 12:06:33PM GMT, Bartosz Golaszewski wrote:
> On Tue, Feb 18, 2025 at 5:04 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
> > platform device, wait for probe completion, and retrieve the probe
> > success or error status synchronously. With gpio-aggregator planned to
> > adopt this approach for its configfs interface, it's time to factor
> > out the common code.
> >
> > Add dev-sync-probe.[ch] to house helper functions used by all such
> > implementations.
> >
> > No functional change.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> 
> This is looking good now. A couple more nits for this series and the
> next iteration should be good to go.
> 
> A note on patch versioning: when you split an existing series into
> smaller, please keep the existing patch versioning. So if you had a
> series that went up to v3 and you split it into two smaller ones, the
> next time you submit it, it should be v4.
> 
> >  drivers/gpio/Kconfig          |  7 +++
> >  drivers/gpio/Makefile         |  3 ++
> >  drivers/gpio/dev-sync-probe.c | 96 +++++++++++++++++++++++++++++++++++
> >  drivers/gpio/dev-sync-probe.h | 25 +++++++++
> >  4 files changed, 131 insertions(+)
> >  create mode 100644 drivers/gpio/dev-sync-probe.c
> >  create mode 100644 drivers/gpio/dev-sync-probe.h
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 56c1f30ac195..2e4c5f0a94f7 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1863,6 +1863,13 @@ config GPIO_MPSSE
> >
> >  endmenu
> >
> > +# This symbol is selected by drivers that need synchronous fake device creation
> 
> This comment is unnecessary, please drop it.

Will fix it, thanks.

> 
> > +config DEV_SYNC_PROBE
> > +       tristate "Utilities for synchronous fake device creation"
> 
> Please don't make this available for users to select, this should be a
> hidden symbol only to be selected by its users.

Will fix it, thanks.

> 
> > +       help
> > +         Common helper functions for drivers that need synchronous fake
> > +         device creation.
> > +
> >  menu "Virtual GPIO drivers"
> >
> >  config GPIO_AGGREGATOR
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index af3ba4d81b58..af130882ffee 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -19,6 +19,9 @@ obj-$(CONFIG_GPIO_GENERIC)    += gpio-generic.o
> >  # directly supported by gpio-generic
> >  gpio-generic-$(CONFIG_GPIO_GENERIC)    += gpio-mmio.o
> >
> > +# Utilities for drivers that need synchronous fake device creation
> > +obj-$(CONFIG_DEV_SYNC_PROBE)           += dev-sync-probe.o
> > +
> >  obj-$(CONFIG_GPIO_104_DIO_48E)         += gpio-104-dio-48e.o
> >  obj-$(CONFIG_GPIO_104_IDI_48)          += gpio-104-idi-48.o
> >  obj-$(CONFIG_GPIO_104_IDIO_16)         += gpio-104-idio-16.o
> > diff --git a/drivers/gpio/dev-sync-probe.c b/drivers/gpio/dev-sync-probe.c
> > new file mode 100644
> > index 000000000000..82c8d7ae9fa7
> > --- /dev/null
> > +++ b/drivers/gpio/dev-sync-probe.c
> > @@ -0,0 +1,96 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> Use GPL-2.0-or-later, same elsewhere.

Will fix it, thanks.

> 
> > +/*
> > + * Common code for drivers creating fake platform devices.
> > + *
> > + * Provides synchronous device creation: waits for probe completion and
> > + * returns the probe success or error status to the device creator.
> > + *
> > + * Copyright (C) 2025 Bartosz Golaszewski <brgl@bgdev.pl>
> 
> Please copy my copyright entry from the gpio-sim with the right date
> and add yours too, you did spend some time on this after all. Same for
> MODULE_AUTHOR(), feel free to add yourself too.

Will do so (i.e. change 2025 -> 2021 and add mine), thanks.

> 
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/slab.h>
> > +
> > +#include "dev-sync-probe.h"
> > +
> > +static int dev_sync_probe_notifier_call(struct notifier_block *nb,
> > +                                       unsigned long action,
> > +                                       void *data)
> 
> No need for this last line break.

Will fix it. Thanks, and sorry about silly mistakes.

> 
> > +{
> > +       struct dev_sync_probe_data *pdata;
> > +       struct device *dev = data;
> > +
> > +       pdata = container_of(nb, struct dev_sync_probe_data, bus_notifier);
> > +       if (!device_match_name(dev, pdata->name))
> > +               return NOTIFY_DONE;
> > +
> > +       switch (action) {
> > +       case BUS_NOTIFY_BOUND_DRIVER:
> > +               pdata->driver_bound = true;
> > +               break;
> > +       case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > +               pdata->driver_bound = false;
> > +               break;
> > +       default:
> > +               return NOTIFY_DONE;
> > +       }
> > +
> > +       complete(&pdata->probe_completion);
> > +       return NOTIFY_OK;
> > +}
> > +
> > +void dev_sync_probe_init(struct dev_sync_probe_data *data)
> > +{
> > +       memset(data, 0, sizeof(*data));
> > +       init_completion(&data->probe_completion);
> > +       data->bus_notifier.notifier_call = dev_sync_probe_notifier_call;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_sync_probe_init);
> > +
> > +int dev_sync_probe_register(struct dev_sync_probe_data *data,
> > +                           struct platform_device_info *pdevinfo)
> > +{
> > +       struct platform_device *pdev;
> > +       char *name;
> > +
> > +       name = kasprintf(GFP_KERNEL, "%s.%u", pdevinfo->name, pdevinfo->id);
> 
> pdevinfo->id is a signed integer
> 
> I'm also wondering if we could avoid the allocation here and keep on
> using snprintf() like in the existing drivers? On the other hand,
> memory is cheap so no big deal.

Are you assuming the following change?

   struct dev_sync_probe_data {
          struct platform_device *pdev;
  -       const char *name;
  +       char name[32];

          /* Synchronize with probe */
          struct notifier_block bus_notifier;

> 
> > +       if (!name)
> > +               return -ENOMEM;
> > +
> > +       data->driver_bound = false;
> > +       data->name = name;
> > +       reinit_completion(&data->probe_completion);
> > +       bus_register_notifier(&platform_bus_type, &data->bus_notifier);
> > +
> > +       pdev = platform_device_register_full(pdevinfo);
> > +       if (IS_ERR(pdev)) {
> > +               bus_unregister_notifier(&platform_bus_type, &data->bus_notifier);
> > +               kfree(data->name);
> 
> We could probably simplify it by using __free(kfree) with the name
> variable and just setting it at the end with no_free_ptr().

platform_device_register_full() call path might finish probe so before
calling it, we need to make sure the 'name' is filled in. That's why I
didn't used __free(kfree).

> 
> Bart
> 
> > +               return PTR_ERR(pdev);
> > +       }
> > +
> > +       wait_for_completion(&data->probe_completion);
> > +       bus_unregister_notifier(&platform_bus_type, &data->bus_notifier);
> > +
> > +       if (!data->driver_bound) {
> > +               platform_device_unregister(pdev);
> > +               kfree(data->name);
> > +               return -ENXIO;
> > +       }
> > +
> > +       data->pdev = pdev;
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_sync_probe_register);
> > +
> > +void dev_sync_probe_unregister(struct dev_sync_probe_data *data)
> > +{
> > +       platform_device_unregister(data->pdev);
> > +       kfree(data->name);
> > +       data->pdev = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_sync_probe_unregister);
> > +
> > +MODULE_AUTHOR("Bartosz Golaszewski <brgl@bgdev.pl>");
> > +MODULE_DESCRIPTION("Utilities for synchronous fake device creation");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpio/dev-sync-probe.h b/drivers/gpio/dev-sync-probe.h
> > new file mode 100644
> > index 000000000000..4b3d52b70519
> > --- /dev/null
> > +++ b/drivers/gpio/dev-sync-probe.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef DEV_SYNC_PROBE_H
> > +#define DEV_SYNC_PROBE_H
> > +
> > +#include <linux/completion.h>
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +
> > +struct dev_sync_probe_data {
> > +       struct platform_device *pdev;
> > +       const char *name;
> > +
> > +       /* Synchronize with probe */
> > +       struct notifier_block bus_notifier;
> > +       struct completion probe_completion;
> > +       bool driver_bound;
> > +};
> > +
> > +void dev_sync_probe_init(struct dev_sync_probe_data *data);
> > +int dev_sync_probe_register(struct dev_sync_probe_data *data,
> > +                           struct platform_device_info *pdevinfo);
> > +void dev_sync_probe_unregister(struct dev_sync_probe_data *data);
> > +
> > +#endif /* DEV_SYNC_PROBE_H */
> > --
> > 2.45.2
> >

On Thu, Feb 20, 2025 at 12:06:33PM GMT, Bartosz Golaszewski wrote:
> On Tue, Feb 18, 2025 at 5:04 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
> > platform device, wait for probe completion, and retrieve the probe
> > success or error status synchronously. With gpio-aggregator planned to
> > adopt this approach for its configfs interface, it's time to factor
> > out the common code.
> >
> > Add dev-sync-probe.[ch] to house helper functions used by all such
> > implementations.
> >
> > No functional change.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> 
> This is looking good now. A couple more nits for this series and the
> next iteration should be good to go.
> 
> A note on patch versioning: when you split an existing series into
> smaller, please keep the existing patch versioning. So if you had a
> series that went up to v3 and you split it into two smaller ones, the
> next time you submit it, it should be v4.

Somehow I forgot to reply here.
I got it, thank you. I'll be careful not to make the same mistake next time.

> 
> >  drivers/gpio/Kconfig          |  7 +++
> >  drivers/gpio/Makefile         |  3 ++
> >  drivers/gpio/dev-sync-probe.c | 96 +++++++++++++++++++++++++++++++++++
> >  drivers/gpio/dev-sync-probe.h | 25 +++++++++
> >  4 files changed, 131 insertions(+)
> >  create mode 100644 drivers/gpio/dev-sync-probe.c
> >  create mode 100644 drivers/gpio/dev-sync-probe.h
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 56c1f30ac195..2e4c5f0a94f7 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1863,6 +1863,13 @@ config GPIO_MPSSE
> >
> >  endmenu
> >
> > +# This symbol is selected by drivers that need synchronous fake device creation
> 
> This comment is unnecessary, please drop it.
> 
> > +config DEV_SYNC_PROBE
> > +       tristate "Utilities for synchronous fake device creation"
> 
> Please don't make this available for users to select, this should be a
> hidden symbol only to be selected by its users.
> 
> > +       help
> > +         Common helper functions for drivers that need synchronous fake
> > +         device creation.
> > +
> >  menu "Virtual GPIO drivers"
> >
> >  config GPIO_AGGREGATOR
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index af3ba4d81b58..af130882ffee 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -19,6 +19,9 @@ obj-$(CONFIG_GPIO_GENERIC)    += gpio-generic.o
> >  # directly supported by gpio-generic
> >  gpio-generic-$(CONFIG_GPIO_GENERIC)    += gpio-mmio.o
> >
> > +# Utilities for drivers that need synchronous fake device creation
> > +obj-$(CONFIG_DEV_SYNC_PROBE)           += dev-sync-probe.o
> > +
> >  obj-$(CONFIG_GPIO_104_DIO_48E)         += gpio-104-dio-48e.o
> >  obj-$(CONFIG_GPIO_104_IDI_48)          += gpio-104-idi-48.o
> >  obj-$(CONFIG_GPIO_104_IDIO_16)         += gpio-104-idio-16.o
> > diff --git a/drivers/gpio/dev-sync-probe.c b/drivers/gpio/dev-sync-probe.c
> > new file mode 100644
> > index 000000000000..82c8d7ae9fa7
> > --- /dev/null
> > +++ b/drivers/gpio/dev-sync-probe.c
> > @@ -0,0 +1,96 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> Use GPL-2.0-or-later, same elsewhere.
> 
> > +/*
> > + * Common code for drivers creating fake platform devices.
> > + *
> > + * Provides synchronous device creation: waits for probe completion and
> > + * returns the probe success or error status to the device creator.
> > + *
> > + * Copyright (C) 2025 Bartosz Golaszewski <brgl@bgdev.pl>
> 
> Please copy my copyright entry from the gpio-sim with the right date
> and add yours too, you did spend some time on this after all. Same for
> MODULE_AUTHOR(), feel free to add yourself too.
> 
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/slab.h>
> > +
> > +#include "dev-sync-probe.h"
> > +
> > +static int dev_sync_probe_notifier_call(struct notifier_block *nb,
> > +                                       unsigned long action,
> > +                                       void *data)
> 
> No need for this last line break.
> 
> > +{
> > +       struct dev_sync_probe_data *pdata;
> > +       struct device *dev = data;
> > +
> > +       pdata = container_of(nb, struct dev_sync_probe_data, bus_notifier);
> > +       if (!device_match_name(dev, pdata->name))
> > +               return NOTIFY_DONE;
> > +
> > +       switch (action) {
> > +       case BUS_NOTIFY_BOUND_DRIVER:
> > +               pdata->driver_bound = true;
> > +               break;
> > +       case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > +               pdata->driver_bound = false;
> > +               break;
> > +       default:
> > +               return NOTIFY_DONE;
> > +       }
> > +
> > +       complete(&pdata->probe_completion);
> > +       return NOTIFY_OK;
> > +}
> > +
> > +void dev_sync_probe_init(struct dev_sync_probe_data *data)
> > +{
> > +       memset(data, 0, sizeof(*data));
> > +       init_completion(&data->probe_completion);
> > +       data->bus_notifier.notifier_call = dev_sync_probe_notifier_call;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_sync_probe_init);
> > +
> > +int dev_sync_probe_register(struct dev_sync_probe_data *data,
> > +                           struct platform_device_info *pdevinfo)
> > +{
> > +       struct platform_device *pdev;
> > +       char *name;
> > +
> > +       name = kasprintf(GFP_KERNEL, "%s.%u", pdevinfo->name, pdevinfo->id);
> 
> pdevinfo->id is a signed integer
> 
> I'm also wondering if we could avoid the allocation here and keep on
> using snprintf() like in the existing drivers? On the other hand,
> memory is cheap so no big deal.
> 
> > +       if (!name)
> > +               return -ENOMEM;
> > +
> > +       data->driver_bound = false;
> > +       data->name = name;
> > +       reinit_completion(&data->probe_completion);
> > +       bus_register_notifier(&platform_bus_type, &data->bus_notifier);
> > +
> > +       pdev = platform_device_register_full(pdevinfo);
> > +       if (IS_ERR(pdev)) {
> > +               bus_unregister_notifier(&platform_bus_type, &data->bus_notifier);
> > +               kfree(data->name);
> 
> We could probably simplify it by using __free(kfree) with the name
> variable and just setting it at the end with no_free_ptr().
> 
> Bart
> 
> > +               return PTR_ERR(pdev);
> > +       }
> > +
> > +       wait_for_completion(&data->probe_completion);
> > +       bus_unregister_notifier(&platform_bus_type, &data->bus_notifier);
> > +
> > +       if (!data->driver_bound) {
> > +               platform_device_unregister(pdev);
> > +               kfree(data->name);
> > +               return -ENXIO;
> > +       }
> > +
> > +       data->pdev = pdev;
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_sync_probe_register);
> > +
> > +void dev_sync_probe_unregister(struct dev_sync_probe_data *data)
> > +{
> > +       platform_device_unregister(data->pdev);
> > +       kfree(data->name);
> > +       data->pdev = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_sync_probe_unregister);
> > +
> > +MODULE_AUTHOR("Bartosz Golaszewski <brgl@bgdev.pl>");
> > +MODULE_DESCRIPTION("Utilities for synchronous fake device creation");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpio/dev-sync-probe.h b/drivers/gpio/dev-sync-probe.h
> > new file mode 100644
> > index 000000000000..4b3d52b70519
> > --- /dev/null
> > +++ b/drivers/gpio/dev-sync-probe.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef DEV_SYNC_PROBE_H
> > +#define DEV_SYNC_PROBE_H
> > +
> > +#include <linux/completion.h>
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +
> > +struct dev_sync_probe_data {
> > +       struct platform_device *pdev;
> > +       const char *name;
> > +
> > +       /* Synchronize with probe */
> > +       struct notifier_block bus_notifier;
> > +       struct completion probe_completion;
> > +       bool driver_bound;
> > +};
> > +
> > +void dev_sync_probe_init(struct dev_sync_probe_data *data);
> > +int dev_sync_probe_register(struct dev_sync_probe_data *data,
> > +                           struct platform_device_info *pdevinfo);
> > +void dev_sync_probe_unregister(struct dev_sync_probe_data *data);
> > +
> > +#endif /* DEV_SYNC_PROBE_H */
> > --
> > 2.45.2
> >

Hi Bartosz,

On Thu, 20 Feb 2025 at 12:06, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Tue, Feb 18, 2025 at 5:04 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> > Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
> > platform device, wait for probe completion, and retrieve the probe
> > success or error status synchronously. With gpio-aggregator planned to
> > adopt this approach for its configfs interface, it's time to factor
> > out the common code.
> >
> > Add dev-sync-probe.[ch] to house helper functions used by all such
> > implementations.
> >
> > No functional change.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1863,6 +1863,13 @@ config GPIO_MPSSE
> >
> >  endmenu
> >
> > +# This symbol is selected by drivers that need synchronous fake device creation
>
> This comment is unnecessary, please drop it.
>
> > +config DEV_SYNC_PROBE
> > +       tristate "Utilities for synchronous fake device creation"
>
> Please don't make this available for users to select, this should be a
> hidden symbol only to be selected by its users.

It is still useful to make it visible for compile-testing, i.e.

    tristate "Utilities for synchronous fake device creation" if COMPILE_TEST

As it does not depend on GPIO at all, I think it should be moved
to the end of the file, outside the big "if GPIOLIB ... endif" block.

> > +       help
> > +         Common helper functions for drivers that need synchronous fake
> > +         device creation.
> > +
> >  menu "Virtual GPIO drivers"

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds