From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
With the list of GPIO devices now protected with SRCU we can use
gpio_device_find() to traverse it from sysfs.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++--------------------
drivers/gpio/gpiolib.c | 2 +-
drivers/gpio/gpiolib.h | 1 -
3 files changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 6bf5332136e5..3c3b8559cff5 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -790,11 +790,24 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
}
}
+static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
+{
+ struct gpio_device *gdev = gc->gpiodev;
+ int ret;
+
+ if (gdev->mockdev)
+ return 0;
+
+ ret = gpiochip_sysfs_register(gdev);
+ if (ret)
+ chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
+
+ return 0;
+}
+
static int __init gpiolib_sysfs_init(void)
{
- int status;
- unsigned long flags;
- struct gpio_device *gdev;
+ int status;
status = class_register(&gpio_class);
if (status < 0)
@@ -806,26 +819,8 @@ static int __init gpiolib_sysfs_init(void)
* We run before arch_initcall() so chip->dev nodes can have
* registered, and so arch_initcall() can always gpiod_export().
*/
- spin_lock_irqsave(&gpio_lock, flags);
- list_for_each_entry(gdev, &gpio_devices, list) {
- if (gdev->mockdev)
- continue;
+ gpio_device_find(NULL, gpiofind_sysfs_register);
- /*
- * TODO we yield gpio_lock here because
- * gpiochip_sysfs_register() acquires a mutex. This is unsafe
- * and needs to be fixed.
- *
- * Also it would be nice to use gpio_device_find() here so we
- * can keep gpio_chips local to gpiolib.c, but the yield of
- * gpio_lock prevents us from doing this.
- */
- spin_unlock_irqrestore(&gpio_lock, flags);
- status = gpiochip_sysfs_register(gdev);
- spin_lock_irqsave(&gpio_lock, flags);
- }
- spin_unlock_irqrestore(&gpio_lock, flags);
-
- return status;
+ return 0;
}
postcore_initcall(gpiolib_sysfs_init);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f425e0264b7e..6cfb75ee739d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -85,7 +85,7 @@ DEFINE_SPINLOCK(gpio_lock);
static DEFINE_MUTEX(gpio_lookup_lock);
static LIST_HEAD(gpio_lookup_list);
-LIST_HEAD(gpio_devices);
+static LIST_HEAD(gpio_devices);
/* Protects the GPIO device list against concurrent modifications. */
static DEFINE_MUTEX(gpio_devices_lock);
/* Ensures coherence during read-only accesses to the list of GPIO devices. */
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index d2e73eea9e92..2bf3f9e13ae4 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -136,7 +136,6 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
extern spinlock_t gpio_lock;
-extern struct list_head gpio_devices;
void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
--
2.40.1On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> With the list of GPIO devices now protected with SRCU we can use
> gpio_device_find() to traverse it from sysfs.
...
> +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> +{
> + struct gpio_device *gdev = gc->gpiodev;
> + int ret;
> +
> + if (gdev->mockdev)
> + return 0;
> +
> + ret = gpiochip_sysfs_register(gdev);
> + if (ret)
> + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
> + return 0;
???
> +}
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 5, 2024 at 1:36 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > With the list of GPIO devices now protected with SRCU we can use
> > gpio_device_find() to traverse it from sysfs.
>
> ...
>
> > +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> > +{
> > + struct gpio_device *gdev = gc->gpiodev;
> > + int ret;
> > +
> > + if (gdev->mockdev)
> > + return 0;
> > +
> > + ret = gpiochip_sysfs_register(gdev);
> > + if (ret)
> > + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
>
> > + return 0;
>
> ???
>
Not sure what the ... and ??? mean? The commit message should have
read "... traverse it from gpiofind_sysfs_register()" I agree but the
latter?
Bart
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Feb 05, 2024 at 02:19:10PM +0100, Bartosz Golaszewski wrote:
> On Mon, Feb 5, 2024 at 1:36 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > With the list of GPIO devices now protected with SRCU we can use
> > > gpio_device_find() to traverse it from sysfs.
...
> > > +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> > > +{
> > > + struct gpio_device *gdev = gc->gpiodev;
> > > + int ret;
> > > +
> > > + if (gdev->mockdev)
> > > + return 0;
> > > +
> > > + ret = gpiochip_sysfs_register(gdev);
> > > + if (ret)
> > > + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
> >
> > > + return 0;
> >
> > ???
What the point of function to be int if you effectively ignore this by always
returning 0?
> Not sure what the ... and ??? mean? The commit message should have
> read "... traverse it from gpiofind_sysfs_register()" I agree but the
> latter?
I didn't realize this may not be obvious :-(.
> > > +}
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 5, 2024 at 2:38 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 05, 2024 at 02:19:10PM +0100, Bartosz Golaszewski wrote:
> > On Mon, Feb 5, 2024 at 1:36 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > With the list of GPIO devices now protected with SRCU we can use
> > > > gpio_device_find() to traverse it from sysfs.
>
> ...
>
> > > > +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> > > > +{
> > > > + struct gpio_device *gdev = gc->gpiodev;
> > > > + int ret;
> > > > +
> > > > + if (gdev->mockdev)
> > > > + return 0;
> > > > +
> > > > + ret = gpiochip_sysfs_register(gdev);
> > > > + if (ret)
> > > > + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
> > >
> > > > + return 0;
> > >
> > > ???
>
> What the point of function to be int if you effectively ignore this by always
> returning 0?
>
Because the signature of the callback expects an int to be returned?
Bart
> > Not sure what the ... and ??? mean? The commit message should have
> > read "... traverse it from gpiofind_sysfs_register()" I agree but the
> > latter?
>
> I didn't realize this may not be obvious :-(.
>
> > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Feb 05, 2024 at 02:39:40PM +0100, Bartosz Golaszewski wrote:
> On Mon, Feb 5, 2024 at 2:38 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 05, 2024 at 02:19:10PM +0100, Bartosz Golaszewski wrote:
> > > On Mon, Feb 5, 2024 at 1:36 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
...
> > > > > +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> > > > > +{
> > > > > + struct gpio_device *gdev = gc->gpiodev;
> > > > > + int ret;
> > > > > +
> > > > > + if (gdev->mockdev)
> > > > > + return 0;
> > > > > +
> > > > > + ret = gpiochip_sysfs_register(gdev);
> > > > > + if (ret)
> > > > > + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
> > > >
> > > > > + return 0;
> > > >
> > > > ???
> >
> > What the point of function to be int if you effectively ignore this by always
> > returning 0?
> >
>
> Because the signature of the callback expects an int to be returned?
But why do you return 0 instead of ret?
> > > Not sure what the ... and ??? mean? The commit message should have
> > > read "... traverse it from gpiofind_sysfs_register()" I agree but the
> > > latter?
> >
> > I didn't realize this may not be obvious :-(.
> >
> > > > > +}
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 5, 2024 at 2:47 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 05, 2024 at 02:39:40PM +0100, Bartosz Golaszewski wrote:
> > On Mon, Feb 5, 2024 at 2:38 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 05, 2024 at 02:19:10PM +0100, Bartosz Golaszewski wrote:
> > > > On Mon, Feb 5, 2024 at 1:36 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > > > +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> > > > > > +{
> > > > > > + struct gpio_device *gdev = gc->gpiodev;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (gdev->mockdev)
> > > > > > + return 0;
> > > > > > +
> > > > > > + ret = gpiochip_sysfs_register(gdev);
> > > > > > + if (ret)
> > > > > > + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
> > > > >
> > > > > > + return 0;
> > > > >
> > > > > ???
> > >
> > > What the point of function to be int if you effectively ignore this by always
> > > returning 0?
> > >
> >
> > Because the signature of the callback expects an int to be returned?
>
> But why do you return 0 instead of ret?
>
Because we don't want to *find* a device really. We just want to
iterate over all of them and call a callback. Any value other than 0
will be interpreted as a match. Besides: failure to register one GPIO
sysfs entry shouldn't maybe cause a failure for all subsequent
devices?
Bart
> > > > Not sure what the ... and ??? mean? The commit message should have
> > > > read "... traverse it from gpiofind_sysfs_register()" I agree but the
> > > > latter?
> > >
> > > I didn't realize this may not be obvious :-(.
> > >
> > > > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Feb 05, 2024 at 02:50:18PM +0100, Bartosz Golaszewski wrote:
> On Mon, Feb 5, 2024 at 2:47 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 05, 2024 at 02:39:40PM +0100, Bartosz Golaszewski wrote:
> > > On Mon, Feb 5, 2024 at 2:38 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Feb 05, 2024 at 02:19:10PM +0100, Bartosz Golaszewski wrote:
> > > > > On Mon, Feb 5, 2024 at 1:36 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
...
> > > > > > > +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> > > > > > > +{
> > > > > > > + struct gpio_device *gdev = gc->gpiodev;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (gdev->mockdev)
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + ret = gpiochip_sysfs_register(gdev);
> > > > > > > + if (ret)
> > > > > > > + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
> > > > > >
> > > > > > > + return 0;
> > > > > >
> > > > > > ???
> > > >
> > > > What the point of function to be int if you effectively ignore this by always
> > > > returning 0?
> > >
> > > Because the signature of the callback expects an int to be returned?
> >
> > But why do you return 0 instead of ret?
> >
>
> Because we don't want to *find* a device really. We just want to
> iterate over all of them and call a callback. Any value other than 0
> will be interpreted as a match. Besides: failure to register one GPIO
> sysfs entry shouldn't maybe cause a failure for all subsequent
> devices?
To me it's not obvious, hence I would like to see a comment before return 0.
> > > > > Not sure what the ... and ??? mean? The commit message should have
> > > > > read "... traverse it from gpiofind_sysfs_register()" I agree but the
> > > > > latter?
> > > >
> > > > I didn't realize this may not be obvious :-(.
> > > >
> > > > > > > +}
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 5, 2024 at 2:59 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 05, 2024 at 02:50:18PM +0100, Bartosz Golaszewski wrote:
> > On Mon, Feb 5, 2024 at 2:47 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 05, 2024 at 02:39:40PM +0100, Bartosz Golaszewski wrote:
> > > > On Mon, Feb 5, 2024 at 2:38 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Feb 05, 2024 at 02:19:10PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Mon, Feb 5, 2024 at 1:36 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > On Mon, Feb 05, 2024 at 10:34:03AM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > > > > > +static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
> > > > > > > > +{
> > > > > > > > + struct gpio_device *gdev = gc->gpiodev;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + if (gdev->mockdev)
> > > > > > > > + return 0;
> > > > > > > > +
> > > > > > > > + ret = gpiochip_sysfs_register(gdev);
> > > > > > > > + if (ret)
> > > > > > > > + chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
> > > > > > >
> > > > > > > > + return 0;
> > > > > > >
> > > > > > > ???
> > > > >
> > > > > What the point of function to be int if you effectively ignore this by always
> > > > > returning 0?
> > > >
> > > > Because the signature of the callback expects an int to be returned?
> > >
> > > But why do you return 0 instead of ret?
> > >
> >
> > Because we don't want to *find* a device really. We just want to
> > iterate over all of them and call a callback. Any value other than 0
> > will be interpreted as a match. Besides: failure to register one GPIO
> > sysfs entry shouldn't maybe cause a failure for all subsequent
> > devices?
>
> To me it's not obvious, hence I would like to see a comment before return 0.
>
I'll add it for v3.
Bart
> > > > > > Not sure what the ... and ??? mean? The commit message should have
> > > > > > read "... traverse it from gpiofind_sysfs_register()" I agree but the
> > > > > > latter?
> > > > >
> > > > > I didn't realize this may not be obvious :-(.
> > > > >
> > > > > > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
© 2016 - 2026 Red Hat, Inc.