Re: [PATCH 2/2] media: rcar-isp: Release ISPCORE resources

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

Hello,

On 2026-08-04 10:25:35 +0800, Linmao Li wrote:
> Hi Jacopo,
> 
> 在 2026/8/3 22:17, Jacopo Mondi 写道:
> > Hi Linmao Li
> > 
> > On Mon, Aug 03, 2026 at 05:05:53PM +0800, Linmao Li wrote:
> > > v4l2_device_register() takes a reference to the parent device, but the
> > > ISPCORE remove path never calls v4l2_device_unregister(). The reference
> > > is therefore leaked whenever an ISPCORE is removed.
> > > 
> > > Probe failures after rppx1_create() also return without destroying the
> > > RPPX1 object.
> > > 
> > > Unregister the V4L2 device and destroy the RPPX1 object on the
> > > corresponding error paths, and unregister the V4L2 device during removal.
> > > v4l2_device_unregister() also unregisters all attached subdevices, so it
> > > replaces the narrower subdevice-only cleanup.
> > > 
> > > Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> > The driver has not landed in any Linux release, but it has just been
> > collected for the next merge window.
> > 
> > If your patches get collected as part of the same cycle, I don't think
> > there's any need for a Fixes tag ?
> 
> Agreed. I will drop the Fixes tags from both patches when respinning the
> series.

With Jacopo's concern addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> 
> Thanks,
> Linmao
> 
> > 
> > > Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> > > ---
> > >   drivers/media/platform/renesas/rcar-isp/core.c | 12 +++++++++---
> > >   1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> > > index 8dafffdd8de68..923970fd7841f 100644
> > > --- a/drivers/media/platform/renesas/rcar-isp/core.c
> > > +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> > > @@ -883,17 +883,23 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
> > > 
> > >   	ret = v4l2_device_register(core->dev, &core->v4l2_dev);
> > >   	if (ret)
> > > -		return ret;
> > > +		goto err_destroy_rpp;
> > > 
> > >   	ret = risp_core_create_subdev(core);
> > >   	if (ret)
> > > -		return ret;
> > > +		goto err_unregister_v4l2;
> > > 
> > >   	mutex_init(&core->io_lock);
> > >   	spin_lock_init(&core->lock);
> > >   	INIT_LIST_HEAD(&core->risp_jobs);
> > > 
> > >   	return 0;
> > > +
> > > +err_unregister_v4l2:
> > > +	v4l2_device_unregister(&core->v4l2_dev);
> > > +err_destroy_rpp:
> > > +	rppx1_destroy(core->rpp);
> > > +	return ret;
> > >   }
> > > 
> > >   void risp_core_remove(struct rcar_isp_core *core)
> > > @@ -907,7 +913,7 @@ void risp_core_remove(struct rcar_isp_core *core)
> > >   	for (unsigned int i = 0; i < RISP_CORE_NUM_PADS; i++)
> > >   		risp_core_io_destroy(&core->io[i]);
> > > 
> > > -	v4l2_device_unregister_subdev(&core->subdev);
> > > +	v4l2_device_unregister(&core->v4l2_dev);
> > As v4l2_device_unregister() unregister all subdevs of core->v4l2_dev,
> > this seems correct to me.
> > 
> > R-Car ISP is a little complicated, in the sense that the core->subdev
> > gets registered by risp_cs_registered() which is the handler of the
> > channel selector subdev .registered() callback, and to properly
> > balance we should unregister it in the (not implemented)
> > .unregistered() handler.
> > 
> > However, as this is called as part of the driver's remove handler, and
> > that's what we have at the moment, I guess this is ok
> > 
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > 
> > >   	mutex_destroy(&core->io_lock);
> > >   	rppx1_destroy(core->rpp);
> > > --
> > > 2.25.1
> > > 
> > > 

-- 
Kind Regards,
Niklas Söderlund