Re: [PATCH v12 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 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardirq handler reads hardware registers on a potentially suspended device without checking if it is active.
- [High] Missing `synchronize_irq()` in runtime suspend callback allows powering down hardware while an IRQ handler is mid-flight.
- [High] The threaded IRQ handler accesses hardware registers without verifying the device is awake.
- [High] Leaking an `iommu_group` reference upon job completion.
- [High] Calling `drm_sched_fini()` on the error path of a failed `drm_sched_init()` causes a NULL pointer dereference.
--

commit f451c258aa3279d254b112c34e76371c476fb507
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.

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

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 575945015ca3b..0be8db391989b 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c

[ ... ]

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
	struct rocket_core *core = data;
	u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);

[Severity: High]
This is a pre-existing issue, but does this unconditional register read
crash the system if the device is suspended when the shared interrupt fires?

The driver registers its hardware IRQ with IRQF_SHARED (shared with the
IOMMU). If the shared interrupt fires while the rocket device is
runtime-suspended, rocket_job_irq_handler() immediately executes
rocket_pc_readl() without checking if it is active. Accessing registers
while the device's clocks are disabled triggers an SError kernel panic.

drivers/accel/rocket/rocket_drv.c:rocket_device_runtime_suspend() {
	...
	clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks),
				   rdev->cores[core].clks);

	return 0;
}

[Severity: High]
This is a pre-existing issue, but does missing synchronize_irq() allow
powering down hardware while an IRQ handler is mid-flight?

The rocket_device_runtime_suspend() callback disables clocks via
clk_bulk_disable_unprepare() without prior synchronization of the shared IRQ
line. If an interrupt fires just before this callback, the handler may run
concurrently with the suspend sequence, disabling hardware clocks while the
IRQ handler is still executing, leading to SError kernel panics.

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
	pm_runtime_mark_last_busy(core->dev);

	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);

[Severity: High]
This is a pre-existing issue, but does the threaded IRQ handler access
hardware registers without verifying the device is awake?

If the threaded handler is awakened (e.g., if the hardirq reads 0xffffffff
from powered-off hardware, which evaluates as true for DPU interrupt bits),
it unconditionally writes to OPERATION_ENABLE and INTERRUPT_CLEAR. Since it
does not call pm_runtime_get_if_active(), these writes will cause an SError
if the device is suspended.

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

[Severity: High]
This is a pre-existing issue, but does this leak an iommu_group reference
upon job completion?

iommu_group_get() increments the reference count of the group, but
iommu_detach_group() does not consume it. Since there is no corresponding
iommu_group_put() called, this causes a persistent kobject refcount leak
that eventually overflows.

drivers/accel/rocket/rocket_job.c:rocket_job_init() {
	...
err_sched:
	drm_sched_fini(&core->sched);

	destroy_workqueue(core->reset.wq);
	return ret;
}

[Severity: High]
This is a pre-existing issue, but does calling drm_sched_fini() on the error
path of a failed drm_sched_init() cause a NULL pointer dereference?

If drm_sched_init() fails, it cleans up internally, leaving sched->sched_rq
as NULL but sched->num_rqs > 0. The driver's err_sched path unconditionally
calls drm_sched_fini(), which iterates up to num_rqs and attempts to kfree
sched->sched_rq[i], dereferencing the NULL sched_rq pointer array.

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