[PATCH v3 0/2] media: rcar-isp: fix core registration and IRQ lifetime

[PATCH v3 0/2] media: rcar-isp: fix core registration and IRQ lifetime

[Myeonghun Pak]

The ISP core IRQ is registered before the RPPX1 object and interrupt
handler state are initialized. On removal, it remains registered until
after RPPX1 has been freed. A handler can therefore access RPPX1 outside
its lifetime.

Patch 1 prepares the return-value handling for the later IRQ request in
patch 2. It converts registration-stage -ENODEV to -ENXIO after cleanup,
so the parent does not mistake that failure for an absent optional core.
As requested by Niklas, the caller uses only the return value and does
not inspect core->base. Resource-setup results and the existing
channel-selector fallback behavior remain unchanged.

Patch 2 requests the IRQ after its dependencies are initialized and
releases it before destroying RPPX1. Its code is unchanged from v2.

Compile-tested with Clang 22.1.2, W=1 (arm64). No runtime testing.

Changes in v3:
- Replace the caller's core->base check with -ENODEV to -ENXIO translation
  at the registration error exit in risp_core_probe().
- Keep resource-setup return values and fallback behavior unchanged.
- Describe patch 1 as a prerequisite for patch 2, without assuming that
  the existing V4L2 registration helpers return -ENODEV.
- Keep the IRQ lifetime fix in patch 2 unchanged.

Changes in v2:
- Split the error-handling change into patch 1.
- Explain the RPPX1 allocation, free and pre-lock IRQ accesses in patch 2.

v2: https://lore.kernel.org/r/20261015004159.2202899-1-mhun512@gmail.com
v1: https://lore.kernel.org/r/20261011214448.2043581-1-mhun512@gmail.com

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

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

base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5

[PATCH v3 1/2] media: rcar-isp: distinguish absent cores from registration failures

[Myeonghun Pak]

risp_probe() interprets -ENODEV from risp_core_probe() as an absent
optional core and continues with the channel selector. Keep that result
for the existing resource-setup fallback, but distinguish it from a
failure after core resources have been initialized.

The following patch moves the core IRQ request after RPPX1 and the
registration state have been initialized. If that request returns
-ENODEV, its error path releases RPPX1 and the VSPX device reference.
Passing -ENODEV back unchanged would make the parent continue probing,
although core->base still indicates a core whose resources have already
been released.

Translate -ENODEV to -ENXIO at the registration error exit, after the
existing cleanup. The parent can then handle the failure using only the
return value, without inspecting core->base. Leave all resource-setup
results unchanged, including the existing fallback when VSPX information
is absent.

This prepares the error handling for the IRQ lifetime fix in the next
patch; it does not depend on an existing -ENODEV return from the V4L2
registration helpers.

This issue was identified during our ongoing research while reviewing
kernel code.

Suggested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Assisted-by: LLM
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 v3:
- Express the distinction in the return value, as suggested by Niklas,
  instead of checking core->base in the caller.
- Translate -ENODEV only after resource setup has succeeded, keeping
  existing resource-setup and channel-selector fallback behavior.
- Describe this patch as a prerequisite for the IRQ registration move.

Changes in v2:
- Split the error-handling change out of the IRQ lifetime fix.

v2: https://lore.kernel.org/r/20261015004159.2202899-1-mhun512@gmail.com
v1: https://lore.kernel.org/r/20261011214448.2043581-1-mhun512@gmail.com

Compile-tested with Clang 22.1.2, W=1 (arm64). No runtime testing.

 drivers/media/platform/renesas/rcar-isp/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
index 586a5adbedfa8..373d9ca9374fe 100644
--- a/drivers/media/platform/renesas/rcar-isp/core.c
+++ b/drivers/media/platform/renesas/rcar-isp/core.c
@@ -894,7 +894,8 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
 err_destroy_rpp:
 	rppx1_destroy(core->rpp);
 	put_device(core->vspx.dev);
-	return ret;
+	/* Do not report a registration failure as an absent core. */
+	return ret == -ENODEV ? -ENXIO : ret;
 }
 
 void risp_core_remove(struct rcar_isp_core *core)

Re: [PATCH v3 1/2] media: rcar-isp: distinguish absent cores from registration failures

[Niklas Söderlund]

On 2026-10-17 22:46:53 -0400, Myeonghun Pak wrote:
> risp_probe() interprets -ENODEV from risp_core_probe() as an absent
> optional core and continues with the channel selector. Keep that result
> for the existing resource-setup fallback, but distinguish it from a
> failure after core resources have been initialized.
> 
> The following patch moves the core IRQ request after RPPX1 and the
> registration state have been initialized.

No it doesn't.

> If that request returns
> -ENODEV, its error path releases RPPX1 and the VSPX device reference.
> Passing -ENODEV back unchanged would make the parent continue probing,
> although core->base still indicates a core whose resources have already
> been released.
> 
> Translate -ENODEV to -ENXIO at the registration error exit, after the
> existing cleanup. The parent can then handle the failure using only the
> return value, without inspecting core->base. Leave all resource-setup
> results unchanged, including the existing fallback when VSPX information
> is absent.
> 
> This prepares the error handling for the IRQ lifetime fix in the next
> patch; it does not depend on an existing -ENODEV return from the V4L2
> registration helpers.
> 
> This issue was identified during our ongoing research while reviewing
> kernel code.
> 
> Suggested-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Assisted-by: LLM
> 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 v3:
> - Express the distinction in the return value, as suggested by Niklas,
>   instead of checking core->base in the caller.
> - Translate -ENODEV only after resource setup has succeeded, keeping
>   existing resource-setup and channel-selector fallback behavior.
> - Describe this patch as a prerequisite for the IRQ registration move.
> 
> Changes in v2:
> - Split the error-handling change out of the IRQ lifetime fix.
> 
> v2: https://lore.kernel.org/r/20261015004159.2202899-1-mhun512@gmail.com
> v1: https://lore.kernel.org/r/20261011214448.2043581-1-mhun512@gmail.com
> 
> Compile-tested with Clang 22.1.2, W=1 (arm64). No runtime testing.
> 
>  drivers/media/platform/renesas/rcar-isp/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> index 586a5adbedfa8..373d9ca9374fe 100644
> --- a/drivers/media/platform/renesas/rcar-isp/core.c
> +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> @@ -894,7 +894,8 @@ int risp_core_probe(struct rcar_isp_core *core, struct platform_device *pdev,
>  err_destroy_rpp:
>  	rppx1_destroy(core->rpp);
>  	put_device(core->vspx.dev);
> -	return ret;
> +	/* Do not report a registration failure as an absent core. */
> +	return ret == -ENODEV ? -ENXIO : ret;

This completely misses the error code from risp_core_probe_resources() 
which, as I understand it, is the one you initially wanted to 
differentiate on? This change still propagates the return code from it 
and just chnages -ENODEV to -ENXIO if it happens to be returned from 
v4l2_device_register() or risp_core_create_subdev().

Please stop using an LLM to throw spaghetti at the wall and if you want 
to have this change read, understand and change the code by hand.

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

>  }
>  
>  void risp_core_remove(struct rcar_isp_core *core)

-- 
Kind Regards,
Niklas Söderlund

[PATCH v3 2/2] media: rcar-isp: release the core interrupt before destroying RPPX1

[Myeonghun Pak]

core->rpp is a separately allocated struct rppx1: rppx1_create()
allocates it with kzalloc_obj(), and rppx1_destroy() frees it with
kfree(). It is not part of the device-managed core allocation.

risp_core_irq() calls rppx1_interrupt(core->rpp, ...) before taking
core->lock. rppx1_interrupt() accesses rpp->base through rppx1_read()
and rppx1_write(), and can also use rpp->dev. An interrupt handler that
has passed the ISPCORE status check can still be using this object when
risp_core_remove() frees it. Stopping the video queues and taking
core->lock do not join that earlier part of the handler. The managed IRQ
is otherwise released only after remove returns.

Request the IRQ after the RPPX1 object, subdevice, locks and job list have
been initialized, so probe error paths cannot leave a handler using an
uninitialized or freed RPPX1 object. Release and synchronize the IRQ after
stopping the I/O nodes but before unregistering the V4L2 device and
destroying RPPX1.

This issue was identified during our ongoing research while reviewing
kernel code.

Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
Assisted-by: LLM
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 v3:
- No code changes relative to v2.

Changes in v2:
- Identify the separately allocated RPPX1 object and the rpp->base/rpp->dev
  accesses in the interrupt handler, rather than the core lock lifetime.
- Move -ENODEV handling into a separate prerequisite patch.
- No code changes relative to the IRQ portion of v1.

Compile-tested with Clang 22.1.2, W=1 (arm64). No runtime testing.

 drivers/media/platform/renesas/rcar-isp/core.c      | 13 +++++++++----
 drivers/media/platform/renesas/rcar-isp/risp-core.h |  2 ++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
index 373d9ca9374fe..78cbb549fd18f 100644
--- a/drivers/media/platform/renesas/rcar-isp/core.c
+++ b/drivers/media/platform/renesas/rcar-isp/core.c
@@ -802,10 +802,7 @@ static int risp_core_probe_resources(struct rcar_isp_core *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->irq = ret;
 
 	core->clk = devm_clk_get(&pdev->dev, "core");
 	if (IS_ERR(core->clk))
@@ -887,8 +884,15 @@ 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 = devm_request_irq(core->dev, core->irq, risp_core_irq, IRQF_SHARED,
+			       KBUILD_MODNAME, core);
+	if (ret)
+		goto err_destroy_mutex;
+
 	return 0;
 
+err_destroy_mutex:
+	mutex_destroy(&core->io_lock);
 err_unregister_v4l2:
 	v4l2_device_unregister(&core->v4l2_dev);
 err_destroy_rpp:
@@ -909,6 +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]);
 
+	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 627a762d6b192..70432cf4a88ca 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:	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;