| Message ID | 20240205093418.39755-7-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:01AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Extend the GPIO descriptor with an SRCU structure in order to serialize > the access to the label. Initialize and clean it up where applicable. ... > + for (i = 0; i < gdev->ngpio; i++) > + cleanup_srcu_struct(&gdev->descs[i].srcu); for_each_gpio_desc()? (It might be that the latter should be reworked a bit first, dunno) ... > + for (j = 0; j < i; j++) > + cleanup_srcu_struct(&desc->srcu); What does this loop mean? > + goto err_remove_of_chip; > + } ... > +err_cleanup_desc_srcu: > + for (i = 0; i < gdev->ngpio; i++) > + cleanup_srcu_struct(&gdev->descs[i].srcu); As per above (use existing for_each macro), ... > + struct srcu_struct srcu; > }; I am wondering if moving it to the top of the struct will give a more performant code.
On Mon, Feb 5, 2024 at 2:48 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 05, 2024 at 10:34:01AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Extend the GPIO descriptor with an SRCU structure in order to serialize > > the access to the label. Initialize and clean it up where applicable. > > ... > > > + for (i = 0; i < gdev->ngpio; i++) > > + cleanup_srcu_struct(&gdev->descs[i].srcu); > > for_each_gpio_desc()? That works with chips not devices, we'd need to add a variant for gpio_device, but see below: > > (It might be that the latter should be reworked a bit first, dunno) > > ... > > > + for (j = 0; j < i; j++) > > + cleanup_srcu_struct(&desc->srcu); > > What does this loop mean? I open-coded it because I want to store the value of i to go back and destroy the SRCU structs on failure. > > > + goto err_remove_of_chip; > > + } > > ... > > > +err_cleanup_desc_srcu: > > + for (i = 0; i < gdev->ngpio; i++) > > + cleanup_srcu_struct(&gdev->descs[i].srcu); > > As per above (use existing for_each macro), > > ... > > > + struct srcu_struct srcu; > > }; > > I am wondering if moving it to the top of the struct will give a more > performant code. > Nah, that would be strictly theoretical. It could matter with container_of() but not with a simple pointer dereference. Bart > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Feb 05, 2024 at 02:54:08PM +0100, Bartosz Golaszewski wrote: > On Mon, Feb 5, 2024 at 2:48 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 05, 2024 at 10:34:01AM +0100, Bartosz Golaszewski wrote: ... > > > + for (j = 0; j < i; j++) > > > + cleanup_srcu_struct(&desc->srcu); > > > > What does this loop mean? > > I open-coded it because I want to store the value of i to go back and > destroy the SRCU structs on failure. Where/how is j being used? > > > + goto err_remove_of_chip; > > > + }
On Mon, 5 Feb 2024 14:57:45 +0100, Andy Shevchenko <andriy.shevchenko@linux.intel.com> said: > On Mon, Feb 05, 2024 at 02:54:08PM +0100, Bartosz Golaszewski wrote: >> On Mon, Feb 5, 2024 at 2:48 PM Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > On Mon, Feb 05, 2024 at 10:34:01AM +0100, Bartosz Golaszewski wrote: > > ... > >> > > + for (j = 0; j < i; j++) >> > > + cleanup_srcu_struct(&desc->srcu); >> > >> > What does this loop mean? >> >> I open-coded it because I want to store the value of i to go back and >> destroy the SRCU structs on failure. > > Where/how is j being used? > In this bit: for (i = 0; i < gc->ngpio; i++) { struct gpio_desc *desc = &gdev->descs[i]; ret = init_srcu_struct(&desc->srcu); if (ret) { for (j = 0; j < i; j++) cleanup_srcu_struct(&desc->srcu); goto err_remove_of_chip; } Bart >> > > + goto err_remove_of_chip; >> > > + } > > -- > With Best Regards, > Andy Shevchenko > > >
On Mon, Feb 05, 2024 at 06:04:23AM -0800, Bartosz Golaszewski wrote: > On Mon, 5 Feb 2024 14:57:45 +0100, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> said: > > On Mon, Feb 05, 2024 at 02:54:08PM +0100, Bartosz Golaszewski wrote: > >> On Mon, Feb 5, 2024 at 2:48 PM Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> wrote: > >> > On Mon, Feb 05, 2024 at 10:34:01AM +0100, Bartosz Golaszewski wrote: ... > >> > > + for (j = 0; j < i; j++) > >> > > + cleanup_srcu_struct(&desc->srcu); > >> > > >> > What does this loop mean? > >> > >> I open-coded it because I want to store the value of i to go back and > >> destroy the SRCU structs on failure. > > > > Where/how is j being used? > > > > In this bit: I am sorry, but I don't see how... > for (i = 0; i < gc->ngpio; i++) { > struct gpio_desc *desc = &gdev->descs[i]; > > ret = init_srcu_struct(&desc->srcu); > if (ret) { > for (j = 0; j < i; j++) > cleanup_srcu_struct(&desc->srcu); So, you call the same several times, why? > goto err_remove_of_chip; > } > >> > > + goto err_remove_of_chip; > >> > > + }
On Mon, Feb 5, 2024 at 3:06 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 05, 2024 at 06:04:23AM -0800, Bartosz Golaszewski wrote: > > On Mon, 5 Feb 2024 14:57:45 +0100, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> said: > > > On Mon, Feb 05, 2024 at 02:54:08PM +0100, Bartosz Golaszewski wrote: > > >> On Mon, Feb 5, 2024 at 2:48 PM Andy Shevchenko > > >> <andriy.shevchenko@linux.intel.com> wrote: > > >> > On Mon, Feb 05, 2024 at 10:34:01AM +0100, Bartosz Golaszewski wrote: > > ... > > > >> > > + for (j = 0; j < i; j++) > > >> > > + cleanup_srcu_struct(&desc->srcu); > > >> > > > >> > What does this loop mean? > > >> > > >> I open-coded it because I want to store the value of i to go back and > > >> destroy the SRCU structs on failure. > > > > > > Where/how is j being used? > > > > > > > In this bit: > > I am sorry, but I don't see how... > > > for (i = 0; i < gc->ngpio; i++) { > > struct gpio_desc *desc = &gdev->descs[i]; > > > > ret = init_srcu_struct(&desc->srcu); > > if (ret) { > > for (j = 0; j < i; j++) > > cleanup_srcu_struct(&desc->srcu); > > So, you call the same several times, why? Ah, now I feel stupid. You're right of course, I'll fix it. Bart > > > goto err_remove_of_chip; > > } > > > >> > > + goto err_remove_of_chip; > > >> > > + } > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 5c041d57077b..ea0c0158faaf 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -672,6 +672,10 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid); static void gpiodev_release(struct device *dev) { struct gpio_device *gdev = to_gpio_device(dev); + unsigned int i; + + for (i = 0; i < gdev->ngpio; i++) + cleanup_srcu_struct(&gdev->descs[i].srcu); ida_free(&gpio_ida, gdev->id); kfree_const(gdev->label); @@ -832,7 +836,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, struct lock_class_key *request_key) { struct gpio_device *gdev; - unsigned int i; + unsigned int i, j; int base = 0; int ret = 0; @@ -965,6 +969,13 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, for (i = 0; i < gc->ngpio; i++) { struct gpio_desc *desc = &gdev->descs[i]; + ret = init_srcu_struct(&desc->srcu); + if (ret) { + for (j = 0; j < i; j++) + cleanup_srcu_struct(&desc->srcu); + goto err_remove_of_chip; + } + if (gc->get_direction && gpiochip_line_is_valid(gc, i)) { assign_bit(FLAG_IS_OUT, &desc->flags, !gc->get_direction(gc, i)); @@ -976,7 +987,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, ret = gpiochip_add_pin_ranges(gc); if (ret) - goto err_remove_of_chip; + goto err_cleanup_desc_srcu; acpi_gpiochip_add(gc); @@ -1015,6 +1026,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, gpiochip_irqchip_free_valid_mask(gc); err_remove_acpi_chip: acpi_gpiochip_remove(gc); +err_cleanup_desc_srcu: + for (i = 0; i < gdev->ngpio; i++) + cleanup_srcu_struct(&gdev->descs[i].srcu); err_remove_of_chip: gpiochip_free_hogs(gc); of_gpiochip_remove(gc); diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 1058f326fe2b..6e14b629c48b 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -17,6 +17,7 @@ #include <linux/module.h> #include <linux/notifier.h> #include <linux/rwsem.h> +#include <linux/srcu.h> #define GPIOCHIP_NAME "gpiochip" @@ -147,6 +148,7 @@ void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action); * @label: Name of the consumer * @name: Line name * @hog: Pointer to the device node that hogs this line (if any) + * @srcu: SRCU struct protecting the label pointer. * * These are obtained using gpiod_get() and are preferable to the old * integer-based handles. @@ -184,6 +186,7 @@ struct gpio_desc { #ifdef CONFIG_OF_DYNAMIC struct device_node *hog; #endif + struct srcu_struct srcu; }; #define gpiod_not_found(desc) (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)