Re: [PATCH v5 1/2] media: rcar-isp: release the core IRQ before destroying RPPX1

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

Hi Myeonghun,

On 2026-11-09 21:59:04 -0500, Myeonghun Pak wrote:
> risp_core_irq() accesses the separately allocated RPPX1 object before
> taking core->lock. The managed IRQ can remain registered after probe
> failure or removal frees that object.
> 
> Request the IRQ after initializing RPPX1 and the handler's state.
> Free and synchronize it after draining I/O queues but before destroying
> RPPX1, keeping completion IRQs available during drain. Unwind the
> subdevice and mutex on IRQ failure, and convert -ENODEV to -ENXIO so
> the parent does not mistake the failure for an absent core.
> 
> Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
> Changes in v5:
> - Regenerate for git send-email; no code changes from v4.
> 
> No hardware runtime testing.
> 
>  .../media/platform/renesas/rcar-isp/core.c    | 27 ++++++++++++-------
>  .../platform/renesas/rcar-isp/risp-core.h     |  2 ++
>  2 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> index 586a5adbedf..14f3c8f3505 100644
> --- a/drivers/media/platform/renesas/rcar-isp/core.c
> +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> @@ -798,15 +798,6 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
>  	if (IS_ERR(core->base))
>  		return PTR_ERR(core->base);
>  
> -	ret = platform_get_irq_byname(pdev, "core");
> -	if (ret < 0)
> -		return -ENODEV;
> -
> -	ret = devm_request_irq(&pdev->dev, ret, risp_core_irq, IRQF_SHARED,
> -			       KBUILD_MODNAME, core);
> -	if (ret)
> -		return ret;
> -

I'm not convince this needs to be moved, but it might. But can't it be 
moved inside risp_core_probe_resources() to make things more clear?  
Worse case you need an error path that frees the interrupt.

>  	core->clk = devm_clk_get(&pdev->dev, "core");
>  	if (IS_ERR(core->clk))
>  		return -ENODEV;
> @@ -887,14 +878,27 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
>  	spin_lock_init(&core->lock);
>  	INIT_LIST_HEAD(&core->risp_jobs);
>  
> +	ret = platform_get_irq_byname(pdev, "core");
> +	if (ret < 0)
> +		goto err_cleanup_subdev;
> +	core->irq = ret;
> +
> +	ret = devm_request_irq(core->dev, core->irq, risp_core_irq, IRQF_SHARED,
> +			       KBUILD_MODNAME, core);
> +	if (ret)
> +		goto err_cleanup_subdev;
> +
>  	return 0;
>  
> +err_cleanup_subdev:
> +	mutex_destroy(&core->io_lock);
> +	media_entity_cleanup(&core->subdev.entity);
>  err_unregister_v4l2:
>  	v4l2_device_unregister(&core->v4l2_dev);
>  err_destroy_rpp:
>  	rppx1_destroy(core->rpp);
>  	put_device(core->vspx.dev);
> -	return ret;
> +	return ret == -ENODEV ? -ENXIO : ret;

I don't like this. It makes the code harder to read. Also looking at 2/2 
this is never checked and you have the same substitution done here before 
propagating the error further.

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

>  }
>  
>  void risp_core_remove(struct rcar_isp_core *core)
> @@ -905,9 +909,12 @@ void risp_core_remove(struct rcar_isp_core *core)
>  
>  	dev_info(core->dev, "Remove ISP Core\n");
>  
> +	/* Keep completion IRQs available while streaming queues are drained. */
>  	for (unsigned int i = 0; i < RISP_CORE_NUM_PADS; i++)
>  		risp_core_io_destroy(&core->io[i]);
>  
> +	devm_free_irq(core->dev, core->irq, core);
> +
>  	v4l2_device_unregister(&core->v4l2_dev);
>  
>  	mutex_destroy(&core->io_lock);
> diff --git a/drivers/media/platform/renesas/rcar-isp/risp-core.h b/drivers/media/platform/renesas/rcar-isp/risp-core.h
> index 627a762d6b1..9e057bff4ee 100644
> --- a/drivers/media/platform/renesas/rcar-isp/risp-core.h
> +++ b/drivers/media/platform/renesas/rcar-isp/risp-core.h
> @@ -112,6 +112,7 @@ struct rcar_isp_vspx {
>   * @dev:	(OF) device
>   * @rppaddr:	Hardware address of the RPP ISP (from OF)
>   * @clk:	The clock for the ISP CORE
> + * @irq:	The interrupt for the ISP CORE
>   * @rstc:	The reset for the ISP Core
>   * @csrstc:	The reset for the ISP Channel Selector
>   *
> @@ -138,6 +139,7 @@ struct rcar_isp_core {
>  
>  	u32 rppaddr;
>  	struct clk *clk;
> +	int irq;
>  
>  	struct reset_control *rstc;
>  	struct reset_control *csrstc;

-- 
Kind Regards,
Niklas Söderlund