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

[Andy Shevchenko]

On Wed, Oct 21, 2026 at 02:17:56PM -0700, Dmitry Torokhov wrote:
> 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()?

It's proper allocation mechanism for bitmap. With k*alloc*() this sounds like
a (non-critical) layering violation. This also might add an additional churn
in case the bitmap APIs will get their own type (currently we use the knowledge
on how unsigned long:s is used for bitmap, but imagine the allocation that adds
some header or tail to it).

...

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

OK.

-- 
With Best Regards,
Andy Shevchenko