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

[Linmao Li]

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.


Thanks,
Linmao

> > Signed-off-by: Linmao Li <lilinmao@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxx>
>
> >   	mutex_destroy(&core->io_lock);
> >   	rppx1_destroy(core->rpp);
> > --
> > 2.25.1