| Message ID | 20240221193647.13777-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] gpiolib: Deduplicate cleanup for-loop in gpiochip_add_data_with_key() | expand |
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
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.
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 > >
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.
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
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
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!
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
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.
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);
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(-)