[PATCH v4] media: qcom: iris: guard IRQ handler with runtime PM check

[PATCH v4] media: qcom: iris: guard IRQ handler with runtime PM check

[Hungyu Lin]

Guard hardware register access in the threaded IRQ handler with
pm_runtime_get_if_active().

A possible ordering exists where the top-half IRQ handler returns
IRQ_WAKE_THREAD, runtime PM suspend powers down the VPU, and the
threaded IRQ handler subsequently runs and accesses hardware
registers through iris_vpu_clear_interrupt().

Avoid touching registers when the device is no longer active by
skipping interrupt processing when runtime PM indicates that the
device is suspended.

Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
---
v4:
- Clarify the possible ordering between IRQ_WAKE_THREAD,
  runtime PM suspend, and the threaded IRQ handler.
- Describe the race condition motivating the runtime PM check.

v3:
- Remove the early enable_irq() from the PM-inactive early-return path.
- IRQ re-enablement is already handled by iris_vpu_power_on() after power-on.

v2:
- Use pm_runtime_get_if_active() instead of pm_runtime_get_if_in_use().
- Handle negative runtime PM return values correctly.
- Return IRQ_NONE when interrupt processing is skipped.

 drivers/media/platform/qcom/iris/iris_hfi_common.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/qcom/iris/iris_hfi_common.c b/drivers/media/platform/qcom/iris/iris_hfi_common.c
index 621c66593d88..ab2ec1e75309 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_common.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_common.c
@@ -100,10 +100,17 @@ irqreturn_t iris_hfi_isr(int irq, void *data)
 irqreturn_t iris_hfi_isr_handler(int irq, void *data)
 {
 	struct iris_core *core = data;
+	int ret;
 
 	if (!core)
 		return IRQ_NONE;
 
+	if (IS_ENABLED(CONFIG_PM)) {
+		ret = pm_runtime_get_if_active(core->dev);
+		if (ret <= 0)
+			return IRQ_NONE;
+	}
+
 	mutex_lock(&core->lock);
 	pm_runtime_mark_last_busy(core->dev);
 	iris_vpu_clear_interrupt(core);
@@ -111,6 +118,8 @@ irqreturn_t iris_hfi_isr_handler(int irq, void *data)
 
 	core->hfi_response_ops->hfi_response_handler(core);
 
+	pm_runtime_put_autosuspend(core->dev);
+
 	if (!iris_vpu_watchdog(core, core->intr_status))
 		enable_irq(irq);
 
-- 
2.34.1

Re: [PATCH v4] media: qcom: iris: guard IRQ handler with runtime PM check

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The threaded IRQ handler returns early without calling `enable_irq()` when the device is runtime suspended or runtime PM is disabled, permanently leaking the IRQ disable depth and disabling the interrupt.
--

commit 300804196935f196b366554c39d9f042b16bde60
Author: Hungyu Lin <dennylin0707@gmail.com>

media: qcom: iris: guard IRQ handler with runtime PM check

Guard hardware register access in the threaded IRQ handler with
pm_runtime_get_if_active() to avoid touching registers when suspended.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_common.c b/drivers/media/platform/qcom/iris/iris_hfi_common.c
> index 621c66593d88d..ab2ec1e753097 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_common.c
> @@ -100,10 +100,17 @@ irqreturn_t iris_hfi_isr(int irq, void *data)
>  irqreturn_t iris_hfi_isr_handler(int irq, void *data)
>  {
>  	struct iris_core *core = data;
> +	int ret;
>  
>  	if (!core)
>  		return IRQ_NONE;
>  
> +	if (IS_ENABLED(CONFIG_PM)) {
> +		ret = pm_runtime_get_if_active(core->dev);
> +		if (ret <= 0)
> +			return IRQ_NONE;

[Severity: High]
Does this early return leave the interrupt permanently disabled?

Looking at the top-half handler iris_hfi_isr() in this same file, it 
unconditionally calls disable_irq_nosync(irq) before returning IRQ_WAKE_THREAD.

If a suspend sequence begins between the execution of the top-half and the
threaded handler, pm_runtime_get_if_active() will return 0. The threaded
handler will then exit early here without calling enable_irq(irq).

This leaves the IRQ disable depth permanently incremented. During resume,
even if the power-on sequence calls enable_irq(), the depth never returns
to 0, leaving the hardware interrupt permanently masked and potentially causing
all future device operations to timeout.

Should this early return path also call enable_irq(irq) to balance the
disable depth?

> +	}
> +
>  	mutex_lock(&core->lock);
>  	pm_runtime_mark_last_busy(core->dev);
>  	iris_vpu_clear_interrupt(core);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608001128.80090-1-dennylin0707@gmail.com?part=1

Re: [PATCH v4] media: qcom: iris: guard IRQ handler with runtime PM check

[Konrad Dybcio]

On 6/8/26 2:11 AM, Hungyu Lin wrote:
> Guard hardware register access in the threaded IRQ handler with
> pm_runtime_get_if_active().
> 
> A possible ordering exists where the top-half IRQ handler returns
> IRQ_WAKE_THREAD, runtime PM suspend powers down the VPU, and the
> threaded IRQ handler subsequently runs and accesses hardware
> registers through iris_vpu_clear_interrupt().
> 
> Avoid touching registers when the device is no longer active by
> skipping interrupt processing when runtime PM indicates that the
> device is suspended.
> 
> Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
> ---

[...]

> --- a/drivers/media/platform/qcom/iris/iris_hfi_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_common.c
> @@ -100,10 +100,17 @@ irqreturn_t iris_hfi_isr(int irq, void *data)
>  irqreturn_t iris_hfi_isr_handler(int irq, void *data)
>  {
>  	struct iris_core *core = data;
> +	int ret;
>  
>  	if (!core)
>  		return IRQ_NONE;
>  
> +	if (IS_ENABLED(CONFIG_PM)) {

I'm not sure this driver is much useful without CONFIG_PM

> +		ret = pm_runtime_get_if_active(core->dev);
> +		if (ret <= 0)
> +			return IRQ_NONE;
> +	}

Have you actually hit this issue, or is it purely theoretical? We
shouldn't be receiving interrupts at the tail end of suspend callbacks
(and there's a disable_irq_nosync() right after the HW is disabled)

Konrad

Re: [PATCH v4] media: qcom: iris: guard IRQ handler with runtime PM check

[Hungyu Lin]

> Have you actually hit this issue, or is it purely theoretical? We
> shouldn't be receiving interrupts at the tail end of suspend callbacks
> (and there's a disable_irq_nosync() right after the HW is disabled)

I have not observed this on hardware.

My concern comes from disable_irq_nosync().

Its documentation states that it does not ensure
existing instances of the IRQ handler have completed
before returning.

Given that, could a queued threaded handler still
execute after the VPU has been powered down?

Thanks,
Hungyu