Re: [PATCH v13 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 1 potential issue(s) to consider:
- [High] rocket: interrupt masking bypassed when PM is disabled or suspending
--

commit 514c825015ef4be7a517e9ee1eca6f94eae37e6d
Author: Jiaxing Hu <gahing@gahingwoo.com>
Subject: accel/rocket: wait for a running IRQ handler before resetting a core

This commit introduces a mechanism to wait for a threaded IRQ handler that
might already be running when rocket_reset() is called. It masks the
interrupt block, clears the raw status, and calls synchronize_irq() to
prevent a concurrent handler from running. It uses
pm_runtime_get_if_active() to avoid writing to registers while the power
domain is down, preventing an SError.

Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@gmail.com/
Link: https://lore.kernel.org/all/CAEWPSH5mxTbUkNouxm6yecMZYvDowquhvYvhaXQ8HoMtHD5U1g@mail.gmail.com/
Link: https://lore.kernel.org/all/20260912113717.6819-1-royalnet026@gmail.com/

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 575945015ca3b..dfe9135d86af5 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> @@ -377,9 +377,68 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
[ ... ]
> +	 * pm_runtime_get_if_active() does not invoke a callback -- it only
> +	 * takes a reference on an already-active device -- and
> +	 * pm_runtime_put_autosuspend() is asynchronous, so neither can re-enter
> +	 * this driver's runtime PM callbacks while the lock is held.
> +	 */
> +	scoped_guard(mutex, &core->job_lock) {
> +		if (pm_runtime_get_if_active(core->dev) > 0) {
> +			rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
> +			rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
> +			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]
Does this logic bypass interrupt masking when PM is disabled or during system
suspend?

If CONFIG_PM is disabled, pm_runtime_get_if_active() unconditionally returns
-EINVAL, which evaluates to <= 0.

Additionally, during system suspend, the driver uses
pm_runtime_force_suspend(). This increments the PM disable_depth before
invoking the runtime suspend callback to turn off the clocks. Because
disable_depth is non-zero, pm_runtime_get_if_active() returns -EINVAL even
though the device is still fully active at that point.

In both of these scenarios, the condition evaluates to false and the crucial
interrupt masking is skipped.

Since the hardware remains active and the interrupt is unmasked, can the
handler fire concurrently with or immediately after synchronize_irq(),
recreating the exact race condition this code aims to close?

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