[PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Andy Shevchenko]

There is no need to repeat for-loop twice in the error path in
gpiochip_add_data_with_key(). Deduplicate it. While at it,
rename loop variable to be more specific and avoid ambguity.

It also properly unwinds the SRCU, i.e. in reversed order of allocating.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1706edb3ee3f..60fa7816c799 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -861,7 +861,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, j;
+	unsigned int desc_index;
 	int base = 0;
 	int ret = 0;
 
@@ -965,8 +965,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		}
 	}
 
-	for (i = 0; i < gc->ngpio; i++)
-		gdev->descs[i].gdev = gdev;
+	for (desc_index = 0; desc_index < gc->ngpio; desc_index++)
+		gdev->descs[desc_index].gdev = gdev;
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
@@ -992,19 +992,16 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	if (ret)
 		goto err_cleanup_gdev_srcu;
 
-	for (i = 0; i < gc->ngpio; i++) {
-		struct gpio_desc *desc = &gdev->descs[i];
+	for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
+		struct gpio_desc *desc = &gdev->descs[desc_index];
 
 		ret = init_srcu_struct(&desc->srcu);
-		if (ret) {
-			for (j = 0; j < i; j++)
-				cleanup_srcu_struct(&gdev->descs[j].srcu);
-			goto err_free_gpiochip_mask;
-		}
+		if (ret)
+			goto err_cleanup_desc_srcu;
 
-		if (gc->get_direction && gpiochip_line_is_valid(gc, i)) {
+		if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
 			assign_bit(FLAG_IS_OUT,
-				   &desc->flags, !gc->get_direction(gc, i));
+				   &desc->flags, !gc->get_direction(gc, desc_index));
 		} else {
 			assign_bit(FLAG_IS_OUT,
 				   &desc->flags, !gc->direction_input);
@@ -1061,9 +1058,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	gpiochip_free_hogs(gc);
 	of_gpiochip_remove(gc);
 err_cleanup_desc_srcu:
-	for (i = 0; i < gdev->ngpio; i++)
-		cleanup_srcu_struct(&gdev->descs[i].srcu);
-err_free_gpiochip_mask:
+	while (desc_index--)
+		cleanup_srcu_struct(&gdev->descs[desc_index].srcu);
 	gpiochip_free_valid_mask(gc);
 err_cleanup_gdev_srcu:
 	cleanup_srcu_struct(&gdev->srcu);
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Bartosz Golaszewski]

On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> There is no need to repeat for-loop twice in the error path in
> gpiochip_add_data_with_key(). Deduplicate it. While at it,
> rename loop variable to be more specific and avoid ambguity.
>
> It also properly unwinds the SRCU, i.e. in reversed order of allocating.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

This doesn't apply on top of gpio/for-next, I think it depends on one
of your earlier patches?

>  drivers/gpio/gpiolib.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 1706edb3ee3f..60fa7816c799 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -861,7 +861,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, j;
> +       unsigned int desc_index;
>         int base = 0;
>         int ret = 0;
>
> @@ -965,8 +965,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>                 }
>         }
>
> -       for (i = 0; i < gc->ngpio; i++)
> -               gdev->descs[i].gdev = gdev;
> +       for (desc_index = 0; desc_index < gc->ngpio; desc_index++)
> +               gdev->descs[desc_index].gdev = gdev;
>
>         BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
>         BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
> @@ -992,19 +992,16 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>         if (ret)
>                 goto err_cleanup_gdev_srcu;
>
> -       for (i = 0; i < gc->ngpio; i++) {
> -               struct gpio_desc *desc = &gdev->descs[i];
> +       for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
> +               struct gpio_desc *desc = &gdev->descs[desc_index];
>
>                 ret = init_srcu_struct(&desc->srcu);
> -               if (ret) {
> -                       for (j = 0; j < i; j++)
> -                               cleanup_srcu_struct(&gdev->descs[j].srcu);
> -                       goto err_free_gpiochip_mask;
> -               }
> +               if (ret)
> +                       goto err_cleanup_desc_srcu;
>
> -               if (gc->get_direction && gpiochip_line_is_valid(gc, i)) {
> +               if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
>                         assign_bit(FLAG_IS_OUT,
> -                                  &desc->flags, !gc->get_direction(gc, i));
> +                                  &desc->flags, !gc->get_direction(gc, desc_index));
>                 } else {
>                         assign_bit(FLAG_IS_OUT,
>                                    &desc->flags, !gc->direction_input);
> @@ -1061,9 +1058,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>         gpiochip_free_hogs(gc);
>         of_gpiochip_remove(gc);
>  err_cleanup_desc_srcu:
> -       for (i = 0; i < gdev->ngpio; i++)
> -               cleanup_srcu_struct(&gdev->descs[i].srcu);
> -err_free_gpiochip_mask:
> +       while (desc_index--)

What about gdev->descs[0]?

> +               cleanup_srcu_struct(&gdev->descs[desc_index].srcu);
>         gpiochip_free_valid_mask(gc);
>  err_cleanup_gdev_srcu:
>         cleanup_srcu_struct(&gdev->srcu);
> --
> 2.43.0.rc1.1.gbec44491f096
>

Bart

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > There is no need to repeat for-loop twice in the error path in
> > gpiochip_add_data_with_key(). Deduplicate it. While at it,
> > rename loop variable to be more specific and avoid ambguity.
> >
> > It also properly unwinds the SRCU, i.e. in reversed order of allocating.

...

> This doesn't apply on top of gpio/for-next, I think it depends on one
> of your earlier patches?

Yes, on the fix with error path.

...

> > +       while (desc_index--)
> 
> What about gdev->descs[0]?

What about it? :-)

for (i = i - 1; i >= 0; i--)
while (--i >= 0)
while (i--)

are all equivalents.

The difference is what the value will i get _after_ the loop.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Bartosz Golaszewski]

On Thu, Feb 22, 2024 at 2:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > There is no need to repeat for-loop twice in the error path in
> > > gpiochip_add_data_with_key(). Deduplicate it. While at it,
> > > rename loop variable to be more specific and avoid ambguity.
> > >
> > > It also properly unwinds the SRCU, i.e. in reversed order of allocating.
>
> ...
>
> > This doesn't apply on top of gpio/for-next, I think it depends on one
> > of your earlier patches?
>
> Yes, on the fix with error path.
>
> ...
>
> > > +       while (desc_index--)
> >
> > What about gdev->descs[0]?
>
> What about it? :-)
>
> for (i = i - 1; i >= 0; i--)
> while (--i >= 0)
> while (i--)
>
> are all equivalents.
>
> The difference is what the value will i get _after_ the loop.

Ugh of course. But the first one is more readable given I got tricked
by variant #3 at a quick glance but the for loop says out loud what it
does.

Bart

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 02:30:03PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 22, 2024 at 2:28 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > +       while (desc_index--)
> > >
> > > What about gdev->descs[0]?
> >
> > What about it? :-)
> >
> > for (i = i - 1; i >= 0; i--)
> > while (--i >= 0)
> > while (i--)
> >
> > are all equivalents.
> >
> > The difference is what the value will i get _after_ the loop.
> 
> Ugh of course. But the first one is more readable given I got tricked
> by variant #3 at a quick glance but the for loop says out loud what it
> does.

I disagree. `while (i--)` is very well known cleanup pattern.
Less letters to parse, easier to understand.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Bartosz Golaszewski]

On Thu, Feb 22, 2024 at 2:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 22, 2024 at 02:30:03PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Feb 22, 2024 at 2:28 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > > > +       while (desc_index--)
> > > >
> > > > What about gdev->descs[0]?
> > >
> > > What about it? :-)
> > >
> > > for (i = i - 1; i >= 0; i--)
> > > while (--i >= 0)
> > > while (i--)
> > >
> > > are all equivalents.
> > >
> > > The difference is what the value will i get _after_ the loop.
> >
> > Ugh of course. But the first one is more readable given I got tricked
> > by variant #3 at a quick glance but the for loop says out loud what it
> > does.
>
> I disagree. `while (i--)` is very well known cleanup pattern.
> Less letters to parse, easier to understand.
>

Whatever, I don't have a strong opinion, just rebase it and resend.

Bart

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 03:38:56PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 02:30:03PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Feb 22, 2024 at 2:28 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > > +       while (desc_index--)
> > > >
> > > > What about gdev->descs[0]?
> > >
> > > What about it? :-)
> > >
> > > for (i = i - 1; i >= 0; i--)
> > > while (--i >= 0)
> > > while (i--)
> > >
> > > are all equivalents.
> > >
> > > The difference is what the value will i get _after_ the loop.
> > 
> > Ugh of course. But the first one is more readable given I got tricked
> > by variant #3 at a quick glance but the for loop says out loud what it
> > does.
> 
> I disagree. `while (i--)` is very well known cleanup pattern.
> Less letters to parse, easier to understand.

$ git grep -n 'while (i--)' | wc -l
298
$ git grep -n 'while (--i >= 0)' | wc -l
246
$ git grep -n 'for (--i; i >= 0; i--)' | wc -l
29
$ git grep -n 'for (i = i - 1; i >= 0; i--)' | wc -l
17

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 02:40:05PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 22, 2024 at 2:39 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 22, 2024 at 02:30:03PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Feb 22, 2024 at 2:28 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > > > > On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > > > +       while (desc_index--)
> > > > >
> > > > > What about gdev->descs[0]?
> > > >
> > > > What about it? :-)
> > > >
> > > > for (i = i - 1; i >= 0; i--)
> > > > while (--i >= 0)
> > > > while (i--)
> > > >
> > > > are all equivalents.
> > > >
> > > > The difference is what the value will i get _after_ the loop.
> > >
> > > Ugh of course. But the first one is more readable given I got tricked
> > > by variant #3 at a quick glance but the for loop says out loud what it
> > > does.
> >
> > I disagree. `while (i--)` is very well known cleanup pattern.
> > Less letters to parse, easier to understand.
> 
> Whatever, I don't have a strong opinion, just rebase it and resend.

Sure (just will wait to the fix to be settled down first), thanks for review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Bartosz Golaszewski]

On Thu, Feb 22, 2024 at 2:41 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 22, 2024 at 02:40:05PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Feb 22, 2024 at 2:39 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Feb 22, 2024 at 02:30:03PM +0100, Bartosz Golaszewski wrote:
> > > > On Thu, Feb 22, 2024 at 2:28 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > > > > > On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > > > > > +       while (desc_index--)
> > > > > >
> > > > > > What about gdev->descs[0]?
> > > > >
> > > > > What about it? :-)
> > > > >
> > > > > for (i = i - 1; i >= 0; i--)
> > > > > while (--i >= 0)
> > > > > while (i--)
> > > > >
> > > > > are all equivalents.
> > > > >
> > > > > The difference is what the value will i get _after_ the loop.
> > > >
> > > > Ugh of course. But the first one is more readable given I got tricked
> > > > by variant #3 at a quick glance but the for loop says out loud what it
> > > > does.
> > >
> > > I disagree. `while (i--)` is very well known cleanup pattern.
> > > Less letters to parse, easier to understand.
> >
> > Whatever, I don't have a strong opinion, just rebase it and resend.
>
> Sure (just will wait to the fix to be settled down first), thanks for review!
>

I realized you haven't resent it after all, do you still want to change this?

Bart

Re: [PATCH v1 1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key()

[Andy Shevchenko]

On Mon, Mar 04, 2024 at 04:15:19PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 22, 2024 at 2:41 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 22, 2024 at 02:40:05PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Feb 22, 2024 at 2:39 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Feb 22, 2024 at 02:30:03PM +0100, Bartosz Golaszewski wrote:
> > > > > On Thu, Feb 22, 2024 at 2:28 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Thu, Feb 22, 2024 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > > > > > +       while (desc_index--)
> > > > > > >
> > > > > > > What about gdev->descs[0]?
> > > > > >
> > > > > > What about it? :-)
> > > > > >
> > > > > > for (i = i - 1; i >= 0; i--)
> > > > > > while (--i >= 0)
> > > > > > while (i--)
> > > > > >
> > > > > > are all equivalents.
> > > > > >
> > > > > > The difference is what the value will i get _after_ the loop.
> > > > >
> > > > > Ugh of course. But the first one is more readable given I got tricked
> > > > > by variant #3 at a quick glance but the for loop says out loud what it
> > > > > does.
> > > >
> > > > I disagree. `while (i--)` is very well known cleanup pattern.
> > > > Less letters to parse, easier to understand.
> > >
> > > Whatever, I don't have a strong opinion, just rebase it and resend.
> >
> > Sure (just will wait to the fix to be settled down first), thanks for review!
> 
> I realized you haven't resent it after all, do you still want to change this?

Yes. U can prepare a new version later today.

-- 
With Best Regards,
Andy Shevchenko