Re: [PATCH v12 04/14] accel/rocket: let the core suspend after a reset

[sashiko-bot@kernel.org]

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

New issues:
- [High] Async vs Synchronous Runtime PM Put race condition violates hardware power cycle constraints.

Pre-existing issues:
- [High] Shared IRQ handler accesses hardware registers without verifying runtime PM state, risking SError crashes.
- [High] Runtime suspend callback disables clocks without calling synchronize_irq().
- [High] Reference count leak of IOMMU group in job completion path.
--

commit 230b2e3fd33edbead6ba7eab402e2a3e06d6ef3a
Author: Jiaxing Hu <gahing@gahingwoo.com>
accel/rocket: let the core suspend after a reset

This commit changes the runtime PM put in rocket_reset() from noidle to
autosuspend. This aims to allow the core to suspend and cycle its power
domain after a reset, which is necessary on platforms like RK3576 to
recover the IOMMU.

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

> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -421,12 +421,12 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  
>  	/*
>  	 * No handler is running now, but we might still have stuck jobs. Let's
> -	 * make sure the PM counters stay balanced by manually calling
> -	 * pm_runtime_put_noidle().
> +	 * make sure the PM counters stay balanced by putting the reference the
> +	 * job took, and request idle while doing it so the core can suspend.
>  	 */
>  	scoped_guard(mutex, &core->job_lock) {
>  		if (core->in_flight_job)
> -			pm_runtime_put_noidle(core->dev);
> +			pm_runtime_put_autosuspend(core->dev);

[Severity: High]
Will this asynchronous suspend actually have time to trigger before the
scheduler starts again?

The RK3576 NPU requires a power domain cycle (suspend and resume) to reset
its bus interface and recover the IOMMU after a job timeout. However,
pm_runtime_put_autosuspend() queues an asynchronous suspend that may not
fire immediately.

[ ... ]

>  		iommu_detach_group(NULL, core->iommu_group);
>  
>  		core->in_flight_job = NULL;
>  	}
>  
>  	/* Proceed with reset now. */
>  	rocket_core_reset(core);
>  
>  	/* NPU has been reset, we can clear the reset pending bit. */
>  	atomic_set(&core->reset.pending, 0);
>  
>  	/* Restart the scheduler */
>  	drm_sched_start(&core->sched, 0);
>  }

[Severity: High]
If there are multiple jobs queued, doesn't restarting the scheduler here
immediately dequeue the next job?

If the next job calls rocket_job_run() and invokes
pm_runtime_resume_and_get(), this would cancel the pending autosuspend
before the timer expires. Would it be safer to use a synchronous suspend
(like pm_runtime_put_sync_suspend()) to enforce the hardware ordering
constraint before restarting the scheduler?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
issue with how the shared IRQ handler accesses hardware registers:

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

Since the interrupt is shared (IRQF_SHARED) with the IOMMU, couldn't this
handler be invoked unconditionally while the rocket_core is in an
RPM_SUSPENDED state? Reading memory-mapped registers of a suspended device
could cause an asynchronous SError and crash the system. Should this be
guarded with pm_runtime_get_if_active()?

[Severity: High]
This is a pre-existing issue, but the runtime suspend callback disables
clocks without synchronizing the shared IRQ:

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

Because the device uses a shared interrupt, is there a race condition where
the interrupt handler might be actively executing on another CPU while the
runtime suspend callback powers down the hardware? Should this callback use
synchronize_irq() first?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
reference count leak in the job completion path:

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

Does iommu_detach_group() consume the reference returned by
iommu_group_get()? If it doesn't, this seems to leak a reference on the
IOMMU group every time a job is completed. Could this use the cached
core->iommu_group or add a corresponding iommu_group_put()?

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