Re: [PATCH v2] gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()

[Linus Walleij]

On Tue, Feb 3, 2026 at 7:02 AM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> wrote:

> Since commit aab5c6f20023 ("gpio: set device type for GPIO chips"),
> `gdev->dev.release` is unset.  As a result, the reference count to
> `gdev->dev` isn't dropped on the error handling paths.
>
> Drop the reference on errors.
>
> Also reorder the instructions to make the error handling simpler.
> Now gpiochip_add_data_with_key() roughly looks like:
>
>    >>> Some memory allocation.  Go to ERR ZONE 1 on errors.
>    >>> device_initialize().
>
>    (gpiodev_release() takes over the responsibility for freeing the
>     resources of `gdev->dev`.  The subsequent error handling paths
>     shouldn't go through ERR ZONE 1 again which leads to double free.)

This doesn't need to be parenthesized, it should be pointed out
so people can follow it, also:

> @@ -1048,71 +1052,67 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>         int base = 0;
>         int ret;
>
> -       /*
> -        * First: allocate and populate the internal stat container, and
> -        * set up the struct device.
> -        */
>         gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
>         if (!gdev)
>                 return -ENOMEM;
> -
> -       gdev->dev.type = &gpio_dev_type;
> -       gdev->dev.bus = &gpio_bus_type;
> -       gdev->dev.parent = gc->parent;
> -       rcu_assign_pointer(gdev->chip, gc);
> -
>         gc->gpiodev = gdev;
>         gpiochip_set_data(gc, data);
>
> -       device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
> -
>         ret = ida_alloc(&gpio_ida, GFP_KERNEL);
>         if (ret < 0)
>                 goto err_free_gdev;
>         gdev->id = ret;
>
> -       ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
> +       ret = init_srcu_struct(&gdev->srcu);
>         if (ret)
>                 goto err_free_ida;
> +       rcu_assign_pointer(gdev->chip, gc);
>
> -       if (gc->parent && gc->parent->driver)
> -               gdev->owner = gc->parent->driver->owner;
> -       else if (gc->owner)
> -               /* TODO: remove chip->owner */
> -               gdev->owner = gc->owner;
> -       else
> -               gdev->owner = THIS_MODULE;
> +       ret = init_srcu_struct(&gdev->desc_srcu);
> +       if (ret)
> +               goto err_cleanup_gdev_srcu;
> +
> +       ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
> +       if (ret)
> +               goto err_cleanup_desc_srcu;
> +
> +       device_initialize(&gdev->dev);
> +       gdev->dev.type = &gpio_dev_type;


This is the point where, from this point on, gpiodev_release() will
get called from the type.

This is worth a comment in the code don't you think?

Something like:

/*
 * After this point any allocated resources will be free():d by
 * gpiodev_release(), if you add new resources then make sure
 * they get free():ed there.
 */
gdev->dev.type = &gpio_dev_type;

This helps developers to do the right thing.

With that added comment:
Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>

Yours,
Linus Walleij