Re: [PATCH v4 1/2] accel/rocket: Fix error path handling in rocket_job_run()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `synchronize_irq()` in the runtime suspend callback for a device using shared IRQs.
--

commit badac0ffd08d84278aad7a9068c91e38322fa44f
Author: ZhaoJinming <zhaojinming@uniontech.com>

accel/rocket: Fix error path handling in rocket_job_run()

This commit fixes bugs in the error paths of rocket_job_run() by replacing
pm_runtime_get_sync() with pm_runtime_resume_and_get() to prevent leaking
the runtime PM reference. It also properly releases fence references on
error and returns ERR_PTR instead of an unsignaled fence.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 2f1861f960ccb..cf9f61682dd8b 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> @@ -310,13 +310,22 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
>  		dma_fence_put(job->done_fence);
>  	job->done_fence = dma_fence_get(fence);
>  
> -	ret = pm_runtime_get_sync(core->dev);
> -	if (ret < 0)
> -		return fence;
> +	ret = pm_runtime_resume_and_get(core->dev);

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
runtime PM handling here, I noticed a regression in the runtime suspend path:

drivers/accel/rocket/rocket_drv.c:rocket_device_runtime_suspend() {
    ...
	if (!rocket_job_is_idle(&rdev->cores[core]))
		return -EBUSY;

	clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks),
				   rdev->cores[core].clks);

	return 0;
}

Since the device's IRQ is registered with the IRQF_SHARED flag, should
synchronize_irq() be called before gating the device clocks?

Without synchronization, could a concurrently running IRQ handler on another
CPU be halfway through executing when the clocks are abruptly disabled,
potentially causing an external abort when the handler attempts its next
MMIO access?

I saw that the IRQ handler was updated in a later commit ("accel/rocket: Fix
iommu_group leak and unsafe IRQ register access") to use
pm_runtime_get_if_active(), but doesn't the PM subsystem still mandate this
synchronization pattern for all drivers using IRQF_SHARED to guarantee no
handler is executing mid-flight during power down?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610030322.2802744-1-zhaojinming@uniontech.com?part=1