| Message ID | 20250218160333.605829-2-koichiro.den@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add synchronous fake device creation utility for GPIO drivers | expand |
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. > +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 >
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 2:07 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > 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. [snip] > > > +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; > No, I was thinking about a local buffer in the notifier handler, like what we do currently in gpio-sim, but no worries, you can keep it this way. > > > > > + 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). > Not sure I understand this. Would you mind rephrasing? Bart
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
On Thu, Feb 20, 2025 at 2:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > 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 > I disagree, this will get plenty of coverage being used by three virtual drivers. > 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. > Indeed. And eventually I'd like it moved to lib/ but that's for another time. Bart > > > + 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
On Thu, Feb 20, 2025 at 02:13:42PM GMT, Bartosz Golaszewski wrote: > On Thu, Feb 20, 2025 at 2:07 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > 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. > > [snip] > > > > > +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; > > > > No, I was thinking about a local buffer in the notifier handler, like > what we do currently in gpio-sim, but no worries, you can keep it this > way. > > > > > > > > + 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). > > > > Not sure I understand this. Would you mind rephrasing? dev_sync_probe_notifier_call() references dev_sync_probe_data's 'name' field. In dev_sync_probe_register(), platform_device_register_full() invocation can possibly succeed in the initial probe, meaning that dev_sync_probe_notifier_call() can be invoked before platform_device_register_full() returns. So, 'name' field must be set beforehand, and I located 'data->name = name' as shown above; If I used __free(kfree), the number of which I needed to write 'no_free_ptr(data->name);' would be the same (= 2 times). So I thought that calling kfree(data->name) without __free(kfree) was simpler and better. > > Bart
On Thu, Feb 20, 2025 at 02:27:41PM GMT, Bartosz Golaszewski wrote: > On Thu, Feb 20, 2025 at 2:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > 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 > > > > I disagree, this will get plenty of coverage being used by three > virtual drivers. > > > 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. > > > > Indeed. And eventually I'd like it moved to lib/ but that's for another time. I'll move it outside the big "if GPIOLIB ... endif" block. Thanks for pointing that out. Koichiro > > Bart > > > > > + 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
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 +config DEV_SYNC_PROBE + tristate "Utilities for synchronous fake device creation" + 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+ +/* + * 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> + */ + +#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) +{ + 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); + 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); + 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 */
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