| Message ID | 20240205093418.39755-9-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: rework locking and object life-time control | expand |
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; ??? > +}
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 :-(. > > > +}
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 :-(. > > > > > > > +}
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 :-(. > > > > > > > > > > > +}
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 > >
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);