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

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

On Wed, Oct 21, 2026 at 11:54:39PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 21, 2026 at 01:39:49PM -0700, Dmitry Torokhov wrote:
> > 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:
> 
> ...
> 
> > > > +	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.
> 
> Why? You can also allocate 3x bitmaps with bitmap_zalloc(). So I see only
> a single additional allocation.

So the proposal is to continue combining gpio_descs and gpio_array
allocation but split out bitmap allocations (still combining them into
one, so needing to round up nbits).

This looks like worst combination of all. Why do you prefer
bitmap_zalloc()?

> 
> ...
> 
> > > >  	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?
> 
> I would fix of_gpio_count() stub. But I haven't check its users for correctness
> of such a change. I think it would be weird for them to expect 0 when OF_GPIO=n.

OK, I can fix up the stub. The only user of of_gpio_count() is
gpiod_count().

> 
> ...
> 
> > > >  	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.
> 
> That's why I put 'almost' which means we might need to duplicate a few calls
> outside as well.

Yeah, I think doing it inside the loop rather than duplication
gpiod_get_index() outside makes it cleaner.

Thanks.

-- 
Dmitry