Skip to contentSkip to conversation

media: rcar-isp: fix core IRQ and probe error handling

PATCH NAK Myeonghun Pak 2 participants 4 messages Nov 10

Subsystems (3)
Media (V4L/DVB) Arm/RISC-V/Renesas architecture Media drivers for Renesas - vin
lore
Share & tools
lore

Myeonghun Pak

author
#0 lore raw

v5 cover media: rcar-isp: fix core IRQ and probe error handling

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

author
#1 lore raw

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

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 <[email protected]>
Signed-off-by Ijae Kim <[email protected]>
Signed-off-by Myeonghun Pak <[email protected]>

---
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(-)
drivers/media/platform/renesas/rcar-isp/core.c +17 -10
index 586a5adbedf..14f3c8f3505 100644
@@ -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);
drivers/media/platform/renesas/rcar-isp/risp-core.h +2 -0
index 627a762d6b1..9e057bff4ee 100644
@@ -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;
maintainer Nacked-by on v5 1/2 NAK
#2 lore raw

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

Hi Myeonghun,

Quoted 31 lines · 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 <[email protected]>
Signed-off-by Ijae Kim <[email protected]>
Signed-off-by Myeonghun Pak <[email protected]>

---
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(-)
drivers/media/platform/renesas/rcar-isp/core.c +0 -0
index 586a5adbedf..14f3c8f3505 100644
@@ -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);

drivers/media/platform/renesas/rcar-isp/core.c static int risp_core_probe_resources(struct rcar_isp_core *core, · line 809

Quoted 10 lines
drivers/media/platform/renesas/rcar-isp/core.c +0 -9
 
-	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.

Quoted 19 lines
drivers/media/platform/renesas/rcar-isp/core.c +10 -0
 	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;

drivers/media/platform/renesas/rcar-isp/core.c int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev, · line 901

Quoted 10 lines
drivers/media/platform/renesas/rcar-isp/core.c +4 -1
+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 <[email protected]>
Quoted 36 lines
drivers/media/platform/renesas/rcar-isp/core.c +3 -0
 }
 
 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);
drivers/media/platform/renesas/rcar-isp/risp-core.h +2 -0
index 627a762d6b1..9e057bff4ee 100644
@@ -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;
Signature · 2 lines
Kind Regards,
Niklas Söderlund
author
#3 lore raw

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

In reply to 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 <[email protected]>
Co-developed-by Ijae Kim <[email protected]>
Signed-off-by Ijae Kim <[email protected]>
Signed-off-by Myeonghun Pak <[email protected]>

---
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(-)
drivers/media/platform/renesas/rcar-isp/core.c +11 -6
index 14f3c8f3505..0a322014f3d 100644
@@ -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);