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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Feb 21, 2024 at 8:36 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---

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