Re: [PATCH 5/5] gpiolib: avoid krealloc() in gpiod_get_array()

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Andy,

On Wed, Oct 21, 2026 at 10:34:01PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 21, 2026 at 12:02:10PM -0700, Dmitry Torokhov wrote:
> > +static struct gpio_descs *gpio_descs_alloc(unsigned int count, struct gpio_desc *first_desc)
> > +{
> > +	struct gpio_device *gdev = gpiod_to_gpio_device(first_desc);
> > +	struct gpio_array *array_info;
> > +	struct gpio_descs *descs;
> > +	size_t descs_size, alloc_size;
> > +	unsigned int bitmap_size;
> > +
> > +	/*
> > +	 * If pin hardware number of array member 0 is also 0, select
> > +	 * its chip as a candidate for fast bitmap processing path.
> > +	 */
> > +	if (gpiod_hwgpio(first_desc) != 0)
> > +		return kzalloc_flex(*descs, desc, count);
> > +
> > +	descs_size = struct_size(descs, desc, count);
> > +	bitmap_size = BITS_TO_LONGS(max(gdev->ngpio, count));
> > +	alloc_size = descs_size + struct_size(array_info, invert_mask, 3 * bitmap_size);
> > +
> > +	descs = kzalloc(alloc_size, GFP_KERNEL);
> > +	if (!descs)
> > +		return NULL;
> > +
> > +	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);
>
> With the above being done, I would rather see bitmap_zalloc() for bitmaps.

struct gpio_array is defined with a flexible array member (invert_mask[])
specifically so that descs, array_info, and all three bitmaps live in a
single allocation. Switching to bitmap_zalloc() would turn one allocation
into four or five separate allocations and require freeing the bitmaps
both in gpiod_put_array() and when cancelling the fast path in
gpio_descs_check_fastpath(), so I would prefer to keep the single
contiguous allocation here.

> >  	count = gpiod_count(dev, con_id);
> > -	if (count < 0)
> > -		return ERR_PTR(count);
>
> Leave this and simply add
>
> > -	struct gpio_descs *descs __free(gpiod_put_array) =
> > -		kzalloc_flex(*descs, desc, count);
> > -	if (!descs)
> > -		return ERR_PTR(-ENOMEM);
> > +	if (count <= 0)
> > +		return ERR_PTR(count ?: -ENOENT);
>
> 	if (count == 0)
> 		return -ENOENT;
>
> The currently proposed code is less readable.

Looking closer at gpiod_count(), all of its real backends
(of_gpio_count(), acpi_gpio_count(), swnode_gpio_count(), and
platform_gpio_count()) return either a positive count or -ENOENT, and
the !CONFIG_GPIO_ACPI stub for acpi_gpio_count() returns -ENODEV. The
only outlier is the !CONFIG_OF_GPIO stub for of_gpio_count() in
drivers/gpio/gpiolib-of.h, which returns 0 (and also causes
gpiod_count() to skip the if (count < 0) fallback to
platform_gpio_count()).

Do we actually need to check for count == 0 in gpiod_get_array(), or can
we fix the !CONFIG_OF_GPIO stub of of_gpio_count() to return -ENOENT and
leave the count < 0 check in gpiod_get_array() untouched?

> >  	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);
> >  
> > +		if (i == 0) {
>
> This is almost invariant to the loop. Can we move it out?
>
> > +			descs = gpio_descs_alloc(count, desc);
> > +			if (!descs)
> >  				return ERR_PTR(-ENOMEM);
> >  		}
> >  
> > +		gpio_descs_check_fastpath(descs, desc);
> >  
> > +		descs->desc[descs->ndescs++] = no_free_ptr(desc);
> >  	}

gpio_descs_alloc() needs the first descriptor (desc at index 0, acquired
by gpiod_get_index() inside the loop) to check gpiod_hwgpio(desc) and
gdev->ngpio so it knows whether to allocate space for array_info and the
bitmaps up front instead of reallocating with krealloc(). Before the
loop we have not looked up the first descriptor yet.

Thanks.

-- 
Dmitry