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 <[email protected]> > > > > > > > > > > > > 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
signature.asc
Description: PGP signature
