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

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hello Linmao,

Thanks for your work.

On 2026-08-04 10:05:03 +0800, Linmao Li wrote:
> 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 <lilinmao@kylinos.cn>
> > > ---
> > >   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.

I thin I would prefers an explicit error path. Specially as you point 
out, the driver can work with or without an ISPCORE and having the error 
path explicit will make things more robust IMHO.

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

-- 
Kind Regards,
Niklas Söderlund