| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMRc=MfmG0okVjV1nH78Aw18dFcoOAZ-UwU-iFc1VKb-BVcTxQ@mail.gmail.com>
Date: Thu, 20 Feb 2025 12:06:33 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Koichiro Den <koichiro.den@...onical.com>
Cc: linux-gpio@...r.kernel.org, geert+renesas@...der.be,
linus.walleij@...aro.org, maciej.borzecki@...onical.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] gpio: introduce utilities for synchronous fake
device creation
On Tue, Feb 18, 2025 at 5:04 PM Koichiro Den <koichiro.den@...onical.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@...onical.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@...ev.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@...ev.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
>
Powered by blists - more mailing lists