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