Re: [PATCH v11 03/14] accel/rocket: wait for a running IRQ handler before resetting a core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Lockless modification of INTERRUPT_MASK in rocket_reset() races with concurrent task submission in the IRQ handler, potentially leaving interrupts enabled.
- [Medium] Fails to mask interrupts when Runtime PM is disabled, incorrectly treating -EINVAL as an inactive state.

Pre-existing issues:
- [High] Missing iommu_group_put() leaks the IOMMU group reference on every completed job.
- [High] The threaded IRQ handler unconditionally accesses hardware registers without ensuring the device is active, causing an async SError on spurious or delayed interrupts.
--

commit dafaccb47666f525ef9a0018c8357108f3591b85
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: wait for a running IRQ handler before resetting a core

rocket_reset() calls drm_sched_stop(), which stops the scheduler and
returns. It does not wait for a threaded handler that is already
running, so the comment that follows, "Remaining interrupts have been
handled", states an assumption rather than something the code arranges.

[ ... ]

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 5f0f9682e57ca..3c0ed46050669 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -377,9 +377,37 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  	drm_sched_stop(&core->sched, bad);
>  
>  	/*
> -	 * Remaining interrupts have been handled, but we might still have
> -	 * stuck jobs. Let's make sure the PM counters stay balanced by
> -	 * manually calling pm_runtime_put_noidle().
> +	 * Mask the block before waiting. hw_submit() arms INTERRUPT_MASK on
> +	 * every submit and only the hardirq clears it, so on an ordinary
> +	 * timeout it is still live and a completion can arrive after the sync
> +	 * returns. The next submit re-arms it, so nothing is lost here.
> +	 *
> +	 * Only when the device is already awake, though. This function holds no
> +	 * runtime PM reference of its own: the only one in the window belongs to
> +	 * in_flight_job, and the completion path may have put it and cleared the
> +	 * pointer before the timeout worker got here. drm_sched_stop() above can
> +	 * block for a long time, and it drops every pending job's credits, so
> +	 * rocket_job_is_idle() is true and nothing keeps the core resumed. On
> +	 * this hardware a register access with the domain down takes an async
> +	 * SError, so a reset must not be the thing that causes one.
> +	 */
> +	if (pm_runtime_get_if_active(core->dev) > 0) {

[Severity: Medium]
Does this condition fail to account for the -EINVAL return value when
Runtime PM is disabled?

pm_runtime_get_if_active() returns -EINVAL when Runtime PM is disabled.
The condition > 0 evaluates to false, causing the interrupt masking to
be skipped. Even though the device is always awake when PM is disabled,
the mask is still required to prevent concurrent access during reset.

[Severity: High]
Does this lockless register write race with the IRQ handler's hardware
submission?

If rocket_reset() checks pm_runtime_get_if_active() and writes 0x0 to
INTERRUPT_MASK concurrently while the threaded IRQ handler calls
rocket_job_hw_submit() (holding core->job_lock) after reading
reset.pending == 0:

IRQ Handler:
rocket_job_hw_submit()
    rocket_pc_writel(core, INTERRUPT_MASK, PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1);

If this write occurs after rocket_reset()'s write, the interrupt is left
enabled. This could allow the IRQ handler to execute while the device is
suspended, triggering an async SError.

> +		rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
> +		pm_runtime_put_autosuspend(core->dev);
> +	}
> +
> +	/*
> +	 * drm_sched_stop() returns without waiting for a threaded handler that
> +	 * is already running, so wait for one here. This has to stay outside
> +	 * job_lock: the handler takes that lock, so waiting for it while
> +	 * holding it would deadlock instead of fencing anything.
> +	 */
> +	synchronize_irq(core->irq);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this code leak the IOMMU group reference
on every completed job?

drivers/accel/rocket/rocket_job.c:rocket_job_next_locked() {
    ...
	iommu_detach_group(NULL, iommu_group_get(core->dev));
    ...
}

iommu_group_get() increments the kobject reference counter, but
iommu_detach_group() does not consume the reference. Without a matching
call to iommu_group_put(), this causes a permanent resource leak that
could prevent device unbinding.

[Severity: High]
This is a pre-existing issue, but does this unconditional register access risk
an async SError on spurious or delayed interrupts?

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
    ...
	scoped_guard(mutex, &core->job_lock) {
		/*
		 * Stopping the block belongs under the lock. hw_submit() writes
		 * OPERATION_ENABLE too, and outside the lock this zero can land
		 * after that one and stop a task that has only just started.
		 */
		rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
		rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
    ...
}

The threaded IRQ handler unconditionally accesses hardware registers without
verifying if the device is active (e.g., using pm_runtime_get_if_active()).
If a spurious or delayed interrupt runs after the device has suspended,
writing to powered-off hardware will trigger an async SError and kernel
panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831081956.84871-1-gahing@gahingwoo.com?part=3