| 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: <wyicl2dgxkwxzfwd37cmhgshnqb3phmpeboz3gwqqfmbabaegy@tkjx56nj423u>
Date: Thu, 20 Feb 2025 22:07:38 +0900
From: Koichiro Den <koichiro.den@...onical.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
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 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@...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.
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@...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.
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@...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