Re: [PATCH v2 6/7] gpiolib: avoid krealloc() in gpiod_get_array()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] gpiolib: delayed initialization of __free() variable in gpiod_get_array()
--

commit ab11568406b8d4aa5255ce95e09509f3e6c86285
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

gpiolib: avoid krealloc() in gpiod_get_array()

This commit defers allocating the gpio_descs structure in gpiod_get_array()
until the first GPIO descriptor is retrieved so that it can be allocated in
a single pass without needing krealloc(). It also factors descriptor
allocation and fast-path checks into helper functions.

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e71f8865559f6..cf311c1a707c9 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c

[ ... ]

> @@ -5226,105 +5311,35 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
>  						const char *con_id,
>  						enum gpiod_flags flags)
>  {
> -	struct gpio_desc *desc;
> -	struct gpio_device *gdev;
> -	struct gpio_array *array_info = NULL;
> -	int i, count, bitmap_size;
> -	unsigned long dflags;
> +	struct gpio_descs *descs __free(gpiod_put_array) = NULL;
> +	int i, count;
>  
>  	count = gpiod_count(dev, con_id);
>  	if (count < 0)
>  		return ERR_PTR(count);
>  
> -	struct gpio_descs *descs __free(gpiod_put_array) =
> -		kzalloc_flex(*descs, desc, count);
> -	if (!descs)
> -		return ERR_PTR(-ENOMEM);
> -
>  	for (i = 0; i < count; i++) {
> -		desc = gpiod_get_index(dev, con_id, i, flags);
> +		struct gpio_desc *desc __free(gpiod_put) =
> +			gpiod_get_index(dev, con_id, i, flags);
>  		if (IS_ERR(desc))
>  			return ERR_CAST(desc);
>  
> -		descs->desc[descs->ndescs++] = desc;
> -
> -		gdev = gpiod_to_gpio_device(desc);
> -		/*
> -		 * If pin hardware number of array member 0 is also 0, select
> -		 * its chip as a candidate for fast bitmap processing path.
> -		 */
> -		if (i == 0 && gpiod_hwgpio(desc) == 0) {
> -			struct gpio_descs *array;
> -			size_t descs_size = struct_size(descs, desc, count);
> -
> -			bitmap_size = BITS_TO_LONGS(gdev->ngpio > count ?
> -						    gdev->ngpio : count);
> -
> -			array = krealloc(descs, descs_size +
> -					 struct_size(array_info, invert_mask, 3 * bitmap_size),
> -					 GFP_KERNEL | __GFP_ZERO);
> -			if (!array)
> +		if (i == 0) {
> +			descs = gpio_descs_alloc(count, desc);
> +			if (!descs)
>  				return ERR_PTR(-ENOMEM);
> -
> -			descs = array;
> -
> -			array_info = (void *)descs + descs_size;
> -			array_info->get_mask = array_info->invert_mask +
> -						  bitmap_size;
> -			array_info->set_mask = array_info->get_mask +
> -						  bitmap_size;
> -
> -			array_info->desc = descs->desc;
> -			array_info->size = count;
> -			array_info->gdev = gdev;
> -			bitmap_set(array_info->get_mask, 0, count);
> -			bitmap_set(array_info->set_mask, 0, count);
> -			descs->info = array_info;
>  		}

[Severity: Low]
This isn't a bug, but in gpiod_get_array(), descs is declared at the top of
the function with __free(gpiod_put_array) = NULL and then assigned later
inside the loop when i == 0.

The cleanup.h conventions ask that __free() variables be defined and
initialized in a single statement at the point of resource acquisition
rather than initialized to NULL at the top of the function and assigned
later. Could the first descriptor be fetched before the loop, or the
allocation structured so that descs is defined and initialized in one
statement?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-b4-gpiolib-free-v2-0-ea46767c9cf8@gmail.com?part=6