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. > > 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
signature.asc
Description: PGP signature
