Message ID | 20240221192846.4156888-1-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | [v1,1/1] gpiolib: Fix the error path order in gpiochip_add_data_with_key() | expand |
On Wed, Feb 21, 2024 at 8:28 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > After shuffling the code, error path wasn't updated correctly. > Fix it here. > > Fixes: ba5c5effe02c ("gpio: initialize descriptor SRCU structure before adding OF-based chips") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 4b4812bbcafd..1706edb3ee3f 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1056,6 +1056,7 @@ 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); > + gpiochip_remove_pin_ranges(gc); > err_remove_of_chip: > gpiochip_free_hogs(gc); This undoes machine_gpiochip_add() and I think it also needs to be moved before acpi_gpiochip_remove(). Bart > of_gpiochip_remove(gc); > @@ -1063,7 +1064,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > for (i = 0; i < gdev->ngpio; i++) > cleanup_srcu_struct(&gdev->descs[i].srcu); > err_free_gpiochip_mask: > - gpiochip_remove_pin_ranges(gc); > gpiochip_free_valid_mask(gc); > err_cleanup_gdev_srcu: > cleanup_srcu_struct(&gdev->srcu); > -- > 2.43.0.rc1.1.gbec44491f096 >
On Thu, Feb 22, 2024 at 10:37:06AM +0100, Bartosz Golaszewski wrote: > On Wed, Feb 21, 2024 at 8:28 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > After shuffling the code, error path wasn't updated correctly. > > Fix it here. > > gpiochip_irqchip_free_valid_mask(gc); > > err_remove_acpi_chip: > > acpi_gpiochip_remove(gc); > > + gpiochip_remove_pin_ranges(gc); > > err_remove_of_chip: > > gpiochip_free_hogs(gc); > > of_gpiochip_remove(gc); > > This undoes machine_gpiochip_add() and I think it also needs to be > moved before acpi_gpiochip_remove(). You mean it should be like gpiochip_irqchip_free_valid_mask(gc); gpiochip_free_hogs(gc); err_remove_acpi_chip: acpi_gpiochip_remove(gc); gpiochip_remove_pin_ranges(gc); err_remove_of_chip: of_gpiochip_remove(gc); ?
On Thu, 22 Feb 2024 14:25:24 +0100, Andy Shevchenko <andriy.shevchenko@linux.intel.com> said: > On Thu, Feb 22, 2024 at 10:37:06AM +0100, Bartosz Golaszewski wrote: >> On Wed, Feb 21, 2024 at 8:28 PM Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > >> > After shuffling the code, error path wasn't updated correctly. >> > Fix it here. > >> > gpiochip_irqchip_free_valid_mask(gc); >> > err_remove_acpi_chip: >> > acpi_gpiochip_remove(gc); >> > + gpiochip_remove_pin_ranges(gc); >> > err_remove_of_chip: >> > gpiochip_free_hogs(gc); >> > of_gpiochip_remove(gc); >> >> This undoes machine_gpiochip_add() and I think it also needs to be >> moved before acpi_gpiochip_remove(). > > You mean it should be like > > gpiochip_irqchip_free_valid_mask(gc); > gpiochip_free_hogs(gc); > err_remove_acpi_chip: > acpi_gpiochip_remove(gc); > gpiochip_remove_pin_ranges(gc); > err_remove_of_chip: > of_gpiochip_remove(gc); > > ? > > -- > With Best Regards, > Andy Shevchenko > > > Yes, because the sequence is: ret = of_gpiochip_add(gc); if (ret) goto err_cleanup_desc_srcu; ret = gpiochip_add_pin_ranges(gc); if (ret) goto err_remove_of_chip; acpi_gpiochip_add(gc); machine_gpiochip_add(gc); ret = gpiochip_irqchip_init_valid_mask(gc); if (ret) goto err_remove_acpi_chip; Bartosz
On Thu, Feb 22, 2024 at 05:33:59AM -0800, Bartosz Golaszewski wrote: > On Thu, 22 Feb 2024 14:25:24 +0100, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> said: > > On Thu, Feb 22, 2024 at 10:37:06AM +0100, Bartosz Golaszewski wrote: > >> On Wed, Feb 21, 2024 at 8:28 PM Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> wrote: > >> > > >> > After shuffling the code, error path wasn't updated correctly. > >> > Fix it here. > >> > gpiochip_irqchip_free_valid_mask(gc); > >> > err_remove_acpi_chip: > >> > acpi_gpiochip_remove(gc); > >> > + gpiochip_remove_pin_ranges(gc); > >> > err_remove_of_chip: > >> > gpiochip_free_hogs(gc); > >> > of_gpiochip_remove(gc); > >> > >> This undoes machine_gpiochip_add() and I think it also needs to be > >> moved before acpi_gpiochip_remove(). > > > > You mean it should be like > > > > gpiochip_irqchip_free_valid_mask(gc); > > gpiochip_free_hogs(gc); But should it be here... > > err_remove_acpi_chip: ...or here? I'm sorry I really need more (morning) coffee, maybe you can simply update yourself or submit a correct fix? > > acpi_gpiochip_remove(gc); > > gpiochip_remove_pin_ranges(gc); > > err_remove_of_chip: > > of_gpiochip_remove(gc); > > > > ? > > Yes, because the sequence is: > > ret = of_gpiochip_add(gc); > if (ret) > goto err_cleanup_desc_srcu; > > ret = gpiochip_add_pin_ranges(gc); > if (ret) > goto err_remove_of_chip; > > acpi_gpiochip_add(gc); > > machine_gpiochip_add(gc); > > ret = gpiochip_irqchip_init_valid_mask(gc); > if (ret) > goto err_remove_acpi_chip;
On Thu, Feb 22, 2024 at 2:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Feb 22, 2024 at 05:33:59AM -0800, Bartosz Golaszewski wrote: > > On Thu, 22 Feb 2024 14:25:24 +0100, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> said: > > > On Thu, Feb 22, 2024 at 10:37:06AM +0100, Bartosz Golaszewski wrote: > > >> On Wed, Feb 21, 2024 at 8:28 PM Andy Shevchenko > > >> <andriy.shevchenko@linux.intel.com> wrote: > > >> > > > >> > After shuffling the code, error path wasn't updated correctly. > > >> > Fix it here. > > >> > gpiochip_irqchip_free_valid_mask(gc); > > >> > err_remove_acpi_chip: > > >> > acpi_gpiochip_remove(gc); > > >> > + gpiochip_remove_pin_ranges(gc); > > >> > err_remove_of_chip: > > >> > gpiochip_free_hogs(gc); > > >> > of_gpiochip_remove(gc); > > >> > > >> This undoes machine_gpiochip_add() and I think it also needs to be > > >> moved before acpi_gpiochip_remove(). > > > > > > You mean it should be like > > > > > > gpiochip_irqchip_free_valid_mask(gc); > > > > gpiochip_free_hogs(gc); > > But should it be here... > > > > err_remove_acpi_chip: > > ...or here? > > I'm sorry I really need more (morning) coffee, maybe you can simply update > yourself or submit a correct fix? Ok, I'll apply this and send a fix on top of it. Bart
On Thu, Feb 22, 2024 at 02:39:28PM +0100, Bartosz Golaszewski wrote: > On Thu, Feb 22, 2024 at 2:37 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Feb 22, 2024 at 05:33:59AM -0800, Bartosz Golaszewski wrote: > > > On Thu, 22 Feb 2024 14:25:24 +0100, Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> said: > > > > On Thu, Feb 22, 2024 at 10:37:06AM +0100, Bartosz Golaszewski wrote: > > > >> On Wed, Feb 21, 2024 at 8:28 PM Andy Shevchenko > > > >> <andriy.shevchenko@linux.intel.com> wrote: ... > > > >> > gpiochip_irqchip_free_valid_mask(gc); > > > >> > err_remove_acpi_chip: > > > >> > acpi_gpiochip_remove(gc); > > > >> > + gpiochip_remove_pin_ranges(gc); > > > >> > err_remove_of_chip: > > > >> > gpiochip_free_hogs(gc); > > > >> > of_gpiochip_remove(gc); > > > >> > > > >> This undoes machine_gpiochip_add() and I think it also needs to be > > > >> moved before acpi_gpiochip_remove(). > > > > > > > > You mean it should be like > > > > > > > > gpiochip_irqchip_free_valid_mask(gc); > > > > > > gpiochip_free_hogs(gc); > > > > But should it be here... > > > > > > err_remove_acpi_chip: > > > > ...or here? > > > > I'm sorry I really need more (morning) coffee, maybe you can simply update > > yourself or submit a correct fix? > > Ok, I'll apply this and send a fix on top of it. I don't see any progress with this. Do I need to do something?
On Thu, Feb 29, 2024 at 5:29 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > I'm sorry I really need more (morning) coffee, maybe you can simply update > > > yourself or submit a correct fix? > > > > Ok, I'll apply this and send a fix on top of it. > > I don't see any progress with this. Do I need to do something? No, it just fell through the cracks. I applied this now and sent my own fix on top. Bart
On Thu, Feb 29, 2024 at 06:26:29PM +0100, Bartosz Golaszewski wrote: > On Thu, Feb 29, 2024 at 5:29 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > I'm sorry I really need more (morning) coffee, maybe you can simply update > > > > yourself or submit a correct fix? > > > > > > Ok, I'll apply this and send a fix on top of it. > > > > I don't see any progress with this. Do I need to do something? > > No, it just fell through the cracks. I applied this now and sent my > own fix on top. Thank you!
On Thu, Feb 29, 2024 at 7:21 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Feb 29, 2024 at 06:26:29PM +0100, Bartosz Golaszewski wrote: > > On Thu, Feb 29, 2024 at 5:29 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > I'm sorry I really need more (morning) coffee, maybe you can simply update > > > > > yourself or submit a correct fix? > > > > > > > > Ok, I'll apply this and send a fix on top of it. > > > > > > I don't see any progress with this. Do I need to do something? > > > > No, it just fell through the cracks. I applied this now and sent my > > own fix on top. > > Thank you! > > -- > With Best Regards, > Andy Shevchenko > > Hey! I now realized that this commit doesn't really fix ba5c5effe02c ("gpio: initialize descriptor SRCU structure before adding OF-based chips"). It addresses an issue introduced as long ago as commit 2f4133bb5f14 ("gpiolib: No need to call gpiochip_remove_pin_ranges() twice"). I will change the Fixes tag, queue it for fixes and send it to Torvalds for rc7, then merge them back into for-next. Bart
On Fri, Mar 01, 2024 at 08:41:09AM +0100, Bartosz Golaszewski wrote: > On Thu, Feb 29, 2024 at 7:21 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Feb 29, 2024 at 06:26:29PM +0100, Bartosz Golaszewski wrote: > > > On Thu, Feb 29, 2024 at 5:29 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > I'm sorry I really need more (morning) coffee, maybe you can simply update > > > > > > yourself or submit a correct fix? > > > > > > > > > > Ok, I'll apply this and send a fix on top of it. > > > > > > > > I don't see any progress with this. Do I need to do something? > > > > > > No, it just fell through the cracks. I applied this now and sent my > > > own fix on top. > > I now realized that this commit doesn't really fix ba5c5effe02c > ("gpio: initialize descriptor SRCU structure before adding OF-based > chips"). It addresses an issue introduced as long ago as commit > 2f4133bb5f14 ("gpiolib: No need to call gpiochip_remove_pin_ranges() > twice"). Oh, that means it revealed the issue :-) > I will change the Fixes tag, queue it for fixes and send it to > Torvalds for rc7, then merge them back into for-next. Thank you!
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 4b4812bbcafd..1706edb3ee3f 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1056,6 +1056,7 @@ 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); + gpiochip_remove_pin_ranges(gc); err_remove_of_chip: gpiochip_free_hogs(gc); of_gpiochip_remove(gc); @@ -1063,7 +1064,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, for (i = 0; i < gdev->ngpio; i++) cleanup_srcu_struct(&gdev->descs[i].srcu); err_free_gpiochip_mask: - gpiochip_remove_pin_ranges(gc); gpiochip_free_valid_mask(gc); err_cleanup_gdev_srcu: cleanup_srcu_struct(&gdev->srcu);
After shuffling the code, error path wasn't updated correctly. Fix it here. Fixes: ba5c5effe02c ("gpio: initialize descriptor SRCU structure before adding OF-based chips") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)