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

[Tom Rini]

On Wed, Dec 03, 2025 at 10:28:26AM -0600, Tom Rini wrote:
> 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.

Checking this out now, and we've already handled the xPL case by
deciding that we just don't support DEVRES there (and there's likely not
space to do the work nor reason to, given the lifespan of the runtime).

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

Here's where it gets more interesting. For the cases where we could be,
but aren't, enabling DEVRES it's around 500 bytes growth, with the
reward that we get proper and expected memory management. The challenge
is that we have some platforms where there's nothing making use of
devm_kmalloc and friends and so we need to go around and manually select
DEVRES on functionality that makes use of it.

If this is a good architectural change, which it probably is, but
shouldn't be buried in this thread.

-- 
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/20251204/2a6c51da/attachment.sig>