Re: [PATCH v12 02/14] accel/rocket: take the completion register writes under job_lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared hardirq handler accesses hardware registers without checking if the device is active via pm_runtime_get_if_active(), causing spurious interrupts and potential bus faults when suspended.
- [High] The runtime suspend callback disables hardware clocks without first calling `synchronize_irq()`, leading to potential race conditions and hardware lockups with the shared IRQ handler.
- [Medium] Leak of IOMMU group kobject reference in `rocket_job_handle_irq()`.
- [Medium] The module removal path calls `pm_runtime_disable()` which can cancel a pending asynchronous autosuspend, leaving the hardware clocks permanently enabled.
--

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

accel/rocket: take the completion register writes under job_lock

This patch fixes a race condition by moving the OPERATION_ENABLE and
INTERRUPT_CLEAR register writes inside the job_lock scoped_guard. This ensures
that stopping the block and deciding what to start next are performed as a
single atomic step.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index f404355058185..575945015ca3b 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -345,10 +345,15 @@ static void rocket_job_handle_irq(struct rocket_core *core)
>  {
>  	pm_runtime_mark_last_busy(core->dev);
>  
> -	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
> -	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
> +	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);
>  
> -	scoped_guard(mutex, &core->job_lock)
>  		if (core->in_flight_job) {
>  			if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
>  				rocket_job_hw_submit(core, core->in_flight_job);

[Severity: Medium]
This is a pre-existing issue, but does this code leak a kobject
reference every time a job completes?

Just below this hunk in rocket_job_handle_irq(), the completion path has:

			iommu_detach_group(NULL, iommu_group_get(core->dev));

Calling iommu_group_get(core->dev) retrieves the group and increments its
reference count.

However, iommu_detach_group() does not consume this reference, and there is
no matching iommu_group_put() in this path.

Could this be fixed by using the pre-cached core->iommu_group instead?

[ ... ]


[Severity: High]
This is a pre-existing issue, but can the shared IRQ handler cause an
asynchronous bus fault or spurious handling when the device is suspended?

The interrupt is registered as IRQF_SHARED in rocket_job_init():

drivers/accel/rocket/rocket_job.c:rocket_job_init() {
    ...
	ret = devm_request_threaded_irq(core->dev, core->irq,
					rocket_job_irq_handler,
					rocket_job_irq_handler_thread,
					IRQF_SHARED, dev_name(core->dev), core);
    ...
}

If another device on the same shared line triggers an interrupt while the
rocket device is runtime-suspended, the handler blindly reads from the hardware:

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

This unprotected read will typically return 0xffffffff when clocks are
disabled, which satisfies the bitmask checks and causes the handler to wake
the IRQ thread. The thread then executes on powered-off hardware.

Per subsystem guidelines, should we check if the device is active using
pm_runtime_get_if_active() before accessing registers here?


[Severity: High]
This is a pre-existing issue, but does the runtime suspend callback disable
clocks while the shared IRQ handler might still be executing?

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

Since the interrupt is registered as IRQF_SHARED, the handler could be running
concurrently on another CPU.

If clocks are disabled exactly while the in-flight handler attempts to access
hardware registers, it will cause a hardware lockup.

Per subsystem guidelines, drivers using IRQF_SHARED must call
synchronize_irq() in their runtime suspend callback before powering down.

Should a synchronize_irq() call be added here?


[Severity: Medium]
This is a pre-existing issue, but can unloading the module leave the hardware
powered on permanently?

At the end of rocket_core_init() and during job completions, an autosuspend
is scheduled using pm_runtime_put_autosuspend(). However, the removal path
calls pm_runtime_disable() directly:

drivers/accel/rocket/rocket_core.c:rocket_core_fini() {
	pm_runtime_dont_use_autosuspend(core->dev);
	pm_runtime_disable(core->dev);
	iommu_group_put(core->iommu_group);
    ...
}

Calling pm_runtime_disable() cancels any pending asynchronous autosuspend work.

If the module is removed while an autosuspend is pending, the device is left
powered on with clocks enabled.

Should we ensure the device is fully suspended (e.g., using
pm_runtime_put_sync()) before disabling runtime PM?

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