Re: [PATCH 1/2] media: rcar-isp: Fix VSPX reference leaks

Hi Jacopo,

在 2026/8/3 21:43, Jacopo Mondi 写道:
> Hello Linmao Li
>
> On Mon, Aug 03, 2026 at 05:05:52PM +0800, Linmao Li wrote:
>> of_parse_phandle() and of_find_device_by_node() both acquire references,
>> but the ISPCORE probe never releases them. The device node reference is
>> leaked immediately, and the VSPX device reference is leaked on probe
>> failures and on driver removal.
>>
>> Drop the node reference once the platform device has been looked up and
>> release the device reference with a devm action.
>>
>> Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
>> Signed-off-by: Linmao Li <[email protected]>
>> ---
>>   drivers/media/platform/renesas/rcar-isp/core.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
>> index f3dc52c136120..8dafffdd8de68 100644
>> --- a/drivers/media/platform/renesas/rcar-isp/core.c
>> +++ b/drivers/media/platform/renesas/rcar-isp/core.c
>> @@ -781,6 +781,13 @@ int risp_core_registered(struct rcar_isp_core *core, struct v4l2_subdev *sd)
>>   	return 0;
>>   }
>>
>> +static void risp_core_put_device(void *data)
>> +{
>> +	struct device *dev = data;
>> +
>> +	put_device(dev);
>> +}
>> +
>>   static int risp_core_probe_resources(struct rcar_isp_core *core,
>>   				     struct platform_device *pdev)
>>   {
>> @@ -820,9 +827,15 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
>>   		return -ENODEV;
>>
>>   	vspx = of_find_device_by_node(of_vspx);
>> +	of_node_put(of_vspx);
> I was about to suggest to declared of_vspx as:
>
> 	struct device_node *of_vspx = __free(device_node) = NULL;
>
> But maybe it is not necessary since there's a single call place for
> of_node_put().
Agreed. The node is only used to look up the platform device and its
reference is dropped immediately afterwards, so I kept the explicit
of_node_put() to make the lifetime obvious.
>
>
>>   	if (!vspx)
>>   		return -ENODEV;
>>
>> +	ret = devm_add_action_or_reset(&pdev->dev, risp_core_put_device,
>> +				       &vspx->dev);
>> +	if (ret)
>> +		return ret;
>> +
> For my education: what are the drawbacks of using
> devm_add_action_or_reset() instead of releasing core->vspx on probe
> failures and _remove() ?
Explicit cleanup would work as well. I used a devm action to avoid
duplicating the put_device() across the probe error paths and the
remove path.

After the VSPX reference has been acquired, risp_core_probe_resources()
can still fail in vsp1_isp_init(), clk_prepare_enable() or
rppx1_create(). risp_core_probe() clears core->base on those failures,
so risp_core_remove() returns early without performing any cleanup. An
explicit implementation would therefore need a common error path in
addition to the put_device() in remove.

The drawbacks of the devm action are the additional devres allocation
and the less explicit release ordering. There is also a longer
reference lifetime in the optional-ISPCORE case: if rppx1_create()
fails with -ENODEV, the parent driver treats the ISP core as absent and
continues probing successfully, so the action is not unwound and the
VSPX reference is retained until the parent device is removed. This is
harmless, but explicit cleanup would release it earlier.

The action is registered after the reset, clock and IRQ devres, so its
put_device() runs before those are released due to the LIFO ordering.
There is no dependency between them.

I chose devm to keep the cleanup centralized, but I can switch to an
explicit error path if you prefer.

Thanks,
Linmao
>
> Thanks
>    j
>
>>   	/* Attach to VSP-X */
>>   	core->vspx.dev = &vspx->dev;
>>
>>
>> base-commit: 31152f5b0f8719f92063b8c6196cd5e34106c73d
>> --
>> 2.25.1
>>
>>