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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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.

...

> > >  	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.

...

> > >  	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.

-- 
With Best Regards,
Andy Shevchenko