[PATCH v5 0/2] media: rcar-isp: fix core IRQ and probe error handling

[PATCH v5 0/2] media: rcar-isp: fix core IRQ and probe error handling

[Myeonghun Pak]

Fix the core IRQ lifetime before distinguishing absent optional cores
from initialization failures.

Changes in v5:
- Regenerate for git send-email without wrapped diff lines.
- Use media-committers next as the base; no code changes from v4.

No hardware runtime testing.

Myeonghun Pak (2):
  media: rcar-isp: release the core IRQ before destroying RPPX1
  media: rcar-isp: distinguish absent cores from probe failures

 .../media/platform/renesas/rcar-isp/core.c    | 44 ++++++++++++-------
 .../platform/renesas/rcar-isp/risp-core.h     |  2 +
 2 files changed, 30 insertions(+), 16 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4

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

[Myeonghun Pak]

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

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

[Niklas Söderlund]

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

[PATCH v5 2/2] media: rcar-isp: distinguish absent cores from probe failures

[Myeonghun Pak]

risp_probe() interprets -ENODEV as an absent optional core, but resource
setup also returns it for initialization failures and deferred probing.

Reserve -ENODEV for missing core MMIO or the renesas,vspx property.
Preserve clock/reset errors, defer until the referenced VSPX device
exists, and use -ENXIO for malformed VSPX data or failed RPPX1 creation.
Convert remaining resource -ENODEV errors to -ENXIO so the parent
rejects failed initialization instead of continuing with the channel
selector.

Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
Suggested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
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.

 drivers/media/platform/renesas/rcar-isp/core.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
index 14f3c8f3505..0a322014f3d 100644
--- a/drivers/media/platform/renesas/rcar-isp/core.c
+++ b/drivers/media/platform/renesas/rcar-isp/core.c
@@ -800,20 +800,20 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
 
 	core->clk = devm_clk_get(&pdev->dev, "core");
 	if (IS_ERR(core->clk))
-		return -ENODEV;
+		return PTR_ERR(core->clk);
 
 	core->rstc = devm_reset_control_get(&pdev->dev, "core");
 	if (IS_ERR(core->rstc))
-		return -ENODEV;
+		return PTR_ERR(core->rstc);
 
 	of_vspx = of_parse_phandle(pdev->dev.of_node, "renesas,vspx", 0);
 	if (!of_vspx)
-		return -ENODEV;
+		return -ENXIO;
 
 	vspx = of_find_device_by_node(of_vspx);
 	of_node_put(of_vspx);
 	if (!vspx)
-		return -ENODEV;
+		return -EPROBE_DEFER;
 
 	/* Attach to VSP-X */
 	core->vspx.dev = &vspx->dev;
@@ -840,7 +840,7 @@ static int risp_core_probe_resources(struct rcar_isp_core *core,
 	clk_disable_unprepare(core->clk);
 
 	if (!core->rpp) {
-		ret = -ENODEV;
+		ret = -ENXIO;
 		goto err_put_vspx;
 	}
 
@@ -860,10 +860,15 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
 	core->csrstc = csrstc;
 	core->csbase = csbase;
 
+	/* Keep channel-selector-only configurations working. */
+	if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "core") ||
+	    !of_find_property(pdev->dev.of_node, "renesas,vspx", NULL))
+		return -ENODEV;
+
 	ret = risp_core_probe_resources(core, pdev);
 	if (ret) {
 		core->base = NULL;
-		return ret;
+		return ret == -ENODEV ? -ENXIO : ret;
 	}
 
 	ret = v4l2_device_register(core->dev, &core->v4l2_dev);