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?
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?
Cheers,
Quentin
