| 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: <d2qdoq3f3jk6gzgsjeqszgaqk7z523r7cfnopxfq4ghsbsqgp3@zjw67ewqzi5u>
Date: Tue, 18 Feb 2025 01:12:14 +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 1/3] gpio: pseudo: common helper functions for pseudo
gpio devices
On Mon, Feb 17, 2025 at 04:46:30PM GMT, Bartosz Golaszewski wrote:
> On Mon, Feb 17, 2025 at 3:28 PM Koichiro Den <koichiro.den@...onical.com> wrote:
> >
> > Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
> > platform device and wait synchronously for probe completion.
> > With gpio-aggregator adopting the same approach in a later commit for
> > its configfs interface, it's time to factor out the common code.
> >
> > Add gpio-pseudo.[ch] to house helper functions used by all the pseudo
> > GPIO device implementations.
> >
> > No functional change.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@...onical.com>
> > ---
>
Thanks for the review.
> Looking at this patch now, I've realized that there is nothing
> GPIO-specific here. It's a mechanism for synchronous platform device
> probing. I don't think its place is in drivers/gpio/ if we're making
> it a set of library functions. Can I suggest moving it to lib/ and
> renaming the module as pdev_sync_probe or something else that's
> expressive enough to tell users what it does? You can make me the
> maintainer of that module if you wish (feel free to add yourself
> too!).
I had vaguely envisioned that this might eventually contain some
GPIO-specific code for some reason, and also it's just a tiny utility to
reduce code duplication, which is why I placed it in the neighborhood,
drivers/gpio/. However, of course you’re right, there’s nothing
GPIO-specific here, so moving it to lib/ makes sense.
I'm not really sure if this method for synchronous platform device probing
can be broadly accepted as a general solution, but I have no objections to
making the change. I'll move it as you suggested and send v2, setting you
as its maintainer.
>
> > drivers/gpio/Kconfig | 4 ++
> > drivers/gpio/Makefile | 1 +
> > drivers/gpio/gpio-pseudo.c | 86 ++++++++++++++++++++++++++++++++++++++
> > drivers/gpio/gpio-pseudo.h | 24 +++++++++++
> > 4 files changed, 115 insertions(+)
> > create mode 100644 drivers/gpio/gpio-pseudo.c
> > create mode 100644 drivers/gpio/gpio-pseudo.h
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 56c1f30ac195..1e2c95e03a95 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1863,6 +1863,10 @@ config GPIO_MPSSE
> >
> > endmenu
> >
> > +# This symbol is selected by some pseudo gpio device implementations
> > +config GPIO_PSEUDO
> > + bool
>
> Please make it tristate - modules that use it are already tristate.
Thanks for pointing that out. I'll fix it.
>
> > +
> > menu "Virtual GPIO drivers"
> >
> > config GPIO_AGGREGATOR
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index af3ba4d81b58..5eb54147a1ab 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -136,6 +136,7 @@ obj-$(CONFIG_GPIO_PISOSR) += gpio-pisosr.o
> > obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o
> > obj-$(CONFIG_GPIO_PMIC_EIC_SPRD) += gpio-pmic-eic-sprd.o
> > obj-$(CONFIG_GPIO_POLARFIRE_SOC) += gpio-mpfs.o
> > +obj-$(CONFIG_GPIO_PSEUDO) += gpio-pseudo.o
> > obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o
> > obj-$(CONFIG_GPIO_RASPBERRYPI_EXP) += gpio-raspberrypi-exp.o
> > obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o
> > diff --git a/drivers/gpio/gpio-pseudo.c b/drivers/gpio/gpio-pseudo.c
> > new file mode 100644
> > index 000000000000..6e3da05440d8
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-pseudo.c
> > @@ -0,0 +1,86 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Helper functions for Pseudo GPIOs
> > + *
> > + * Copyright 2025 Canonical Ltd.
>
> Well, I'd say that most of the code here is still mine, so I'll go
> ahead and shamelessly claim some credit: it would make sense to keep
> my copyright.
Sure, sorry about that. I'll fix it.
>
> > + */
> > +
> > +#include "gpio-pseudo.h"
> > +
> > +static int pseudo_gpio_notifier_call(struct notifier_block *nb,
> > + unsigned long action,
> > + void *data)
> > +{
> > + struct pseudo_gpio_common *common;
> > + struct device *dev = data;
> > +
> > + common = container_of(nb, struct pseudo_gpio_common, bus_notifier);
> > + if (!device_match_name(dev, common->name))
> > + return NOTIFY_DONE;
> > +
> > + switch (action) {
> > + case BUS_NOTIFY_BOUND_DRIVER:
> > + common->driver_bound = true;
> > + break;
> > + case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > + common->driver_bound = false;
> > + break;
> > + default:
> > + return NOTIFY_DONE;
> > + }
> > +
> > + complete(&common->probe_completion);
> > + return NOTIFY_OK;
> > +}
> > +
> > +void pseudo_gpio_init(struct pseudo_gpio_common *common)
> > +{
> > + memset(common, 0, sizeof(*common));
> > + init_completion(&common->probe_completion);
> > + common->bus_notifier.notifier_call = pseudo_gpio_notifier_call;
> > +}
> > +EXPORT_SYMBOL_GPL(pseudo_gpio_init);
> > +
> > +int pseudo_gpio_register(struct pseudo_gpio_common *common,
> > + 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;
> > +
> > + common->driver_bound = false;
> > + common->name = name;
> > + reinit_completion(&common->probe_completion);
> > + bus_register_notifier(&platform_bus_type, &common->bus_notifier);
> > +
> > + pdev = platform_device_register_full(pdevinfo);
> > + if (IS_ERR(pdev)) {
> > + bus_unregister_notifier(&platform_bus_type, &common->bus_notifier);
> > + kfree(common->name);
> > + return PTR_ERR(pdev);
> > + }
> > +
> > + wait_for_completion(&common->probe_completion);
> > + bus_unregister_notifier(&platform_bus_type, &common->bus_notifier);
> > +
> > + if (!common->driver_bound) {
> > + platform_device_unregister(pdev);
> > + kfree(common->name);
> > + return -ENXIO;
> > + }
> > +
> > + common->pdev = pdev;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pseudo_gpio_register);
> > +
> > +void pseudo_gpio_unregister(struct pseudo_gpio_common *common)
> > +{
> > + platform_device_unregister(common->pdev);
> > + kfree(common->name);
> > + common->pdev = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(pseudo_gpio_unregister);
> > diff --git a/drivers/gpio/gpio-pseudo.h b/drivers/gpio/gpio-pseudo.h
> > new file mode 100644
> > index 000000000000..093112b6cce5
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-pseudo.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef GPIO_PSEUDO_H
> > +#define GPIO_PSEUDO_H
> > +
> > +#include <linux/completion.h>
> > +#include <linux/platform_device.h>
> > +
> > +struct pseudo_gpio_common {
>
> After moving to lib/ this could be named "pdev_sync_probe_data"?
Hm, I can't think of anything else right now. I'll use the name. Thanks!
>
> > + struct platform_device *pdev;
> > + const char *name;
> > +
> > + /* Synchronize with probe */
> > + struct notifier_block bus_notifier;
> > + struct completion probe_completion;
> > + bool driver_bound;
> > +};
> > +
> > +void pseudo_gpio_init(struct pseudo_gpio_common *common);
> > +int pseudo_gpio_register(struct pseudo_gpio_common *common,
> > + struct platform_device_info *pdevinfo);
> > +void pseudo_gpio_unregister(struct pseudo_gpio_common *common);
> > +
> > +#endif /* GPIO_PSEUDO_H */
> > --
> > 2.45.2
> >
>
> Bartosz
Powered by blists - more mailing lists