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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

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 ?

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