diff mbox series

[v1,1/1] gpiolib: Fix the error path order in gpiochip_add_data_with_key()

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

Commit Message

Andy Shevchenko Feb. 21, 2024, 7:28 p.m. UTC 
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(-)

Comments

Bartosz Golaszewski Feb. 22, 2024, 9:37 a.m. UTC | #1 
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
>
Andy Shevchenko Feb. 22, 2024, 1:25 p.m. UTC | #2 
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);

?
Bartosz Golaszewski Feb. 22, 2024, 1:33 p.m. UTC | #3 
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
Andy Shevchenko Feb. 22, 2024, 1:37 p.m. UTC | #4 
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;
Bartosz Golaszewski Feb. 22, 2024, 1:39 p.m. UTC | #5 
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
Andy Shevchenko Feb. 29, 2024, 4:29 p.m. UTC | #6 
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?
Bartosz Golaszewski Feb. 29, 2024, 5:26 p.m. UTC | #7 
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
Andy Shevchenko Feb. 29, 2024, 6:21 p.m. UTC | #8 
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!
Bartosz Golaszewski March 1, 2024, 7:41 a.m. UTC | #9 
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
Andy Shevchenko March 1, 2024, 5:31 p.m. UTC | #10 
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 mbox series

Patch

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);