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

[Quentin Schulz]

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?

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