[PATCH] pinctrl: single: Add missing free in single_allocate_function

[Tom Rini]

On Wed, Dec 03, 2025 at 05:23:47PM +0100, Quentin Schulz wrote:
> Hi Tom,
> 
> On 12/3/25 5:11 PM, Tom Rini wrote:
> > On Wed, Dec 03, 2025 at 04:36:49PM +0100, Quentin Schulz wrote:
> > > Hi François,
> > > 
> > > On 12/2/25 7:39 PM, Francois Berder wrote:
> > > > If func->pins could not be allocated, one must also free
> > > > func variable that was allocated previously.
> > > > 
> > > 
> > > Well.... devm_* functions should take care of this when the device is
> > > removed (or probe failed), but if and only if CONFIG_DEVRES is enabled.
> > > However, in that case, this code may be executed outside of a probe scenario
> > > I guess (it is called in set_state() callback from the pinctrl device). This
> > > thus makes sense to me here.
> > > 
> > > Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>
> > > 
> > > I'm also wondering if we shouldn't check the return value of
> > > single_configure_pins/bits in set_state instead of always returning 0?
> > > 
> > > We probably need to devm_kfree a bunch of other devm_ allocations as well?
> > > 
> > > I see a loop in single_add_gpio_func() we should probably unwind if there's
> > > a devm_kzalloc which fails?
> > > 
> > > Maybe we need a .remove callback where we devm_kfree all of functions and
> > > gpiofuncs lists from single_priv, in case DEVRES isn't actually set?
> > 
> > I've wondered, but not fired off a build to check, what the growth is
> > going to be on default y for DEVRES. There's only a few platforms /
> > chips doing it today and I think most drivers are written like the
> > kernel where freeing is automatic, so there's lots of cases like this
> 
> I see devm I assume it's automatically freed (or whatever counterpart of the
> devm function is) if the device is removed or fails to probe but that's
> rarely the actual case.
> 
> The issue here is that DEVRES is not enough, we would need SPL_DEVRES,
> TPL_DEVRES, VPL_DEVRES, WHATEVER_DEVRES (added and) enabled as well as we do
> compile devres only if $(CONFIG_$(PHASE_)DEVRES) and then surround code with
> CONFIG_IS_ENABLED(DEVRES).

Yes, we'd also need to see about being concerned with xPL or not, more
widely, with respect to DEVRES.

> > one here.
> > 
> 
> I think this one is an exception as the device may still exist if the error
> path is taken, so I think it makes sense to explicitly free it here,
> regardless of DEVRES compilation state.

And then it would only be the case of having to worry about some
failure paths rather than how it's leaked today, if I follow things
right.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20251203/31734730/attachment.sig>