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

[Tom Rini]

On Thu, Dec 04, 2025 at 06:28:34PM +0100, Quentin Schulz wrote:
> Hi Tom,
> 
> On 12/4/25 6:15 PM, Tom Rini wrote:
> > 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).
> > 
> 
> I assume there's no way to have an issue reusing (if at all possible) the
> malloc pool from xPL in a later stage?

Correct. Anything from previous stage passed to next stage must be done
via that mechanism.

> > > > > 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.
> 
> Don't define devm_* functions for proper + !DEVRES so that it fails to
> build?

Yes, something clever can be worked out so that a platform that enables
say DM_RESET (select DEVRES in my WIP tree) and SPL_DM_RESET (no
SPL_DEVRES) builds fine, and say T2080RDB (no devm_* allocs) builds fine
and then still catch that ah, rkmtd also should select DEVRES.

That said, the first practical problem is that
drivers/clk/clk-uclass.c::clk_get_bulk calls devm_kcalloc() and so CLK
should select DEVRES and so that's going to catch a lot of other
existing callers. So that fix gets held off in the series I'd do until I
can catch all the others.

-- 
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/0939aabc/attachment.sig>