Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues: - [High] Resource leak on `devm_pm_domain_attach_list` failure. - [High] Teardown order race in `rocket_job_fini` allows the hrtimer to be re-armed after cancellation. - [High] Race condition and hardware state corruption on RK3576 DMA errors due to double-processing and unsynchronized MMIO writes. Pre-existing issues: - [High] Unconditional MMIO access in shared IRQ handler causes bus faults while device is suspended. - [High] Leaked IOMMU group reference on job completion. - [High] The runtime suspend callback powers down the device without synchronizing the shared IRQ. - [High] Calling `drm_sched_fini` on a partially initialized scheduler causes a kernel panic. -- --- Patch [6]: [RFC PATCH v2 6/8] accel/rocket: add RK3576 NPU (RKNN) support --- commit 3624dfd3291e5171660b2a505645f90fa60606b8 Author: Jiaxing Hu <[email protected]> accel/rocket: add RK3576 NPU (RKNN) support This commit adds RK3576 support to the rocket DRM accelerator driver. It introduces new clocks, power domains, and a polled completion mechanism since the RK3576 has no maskable completion interrupt. > diff --git a/drivers/accel/rocket/rocket_core.c > b/drivers/accel/rocket/rocket_core.c > index b3b2fa9ba645a..140e37969a7b1 100644 > --- a/drivers/accel/rocket/rocket_core.c > +++ b/drivers/accel/rocket/rocket_core.c > @@ -65,6 +83,23 @@ int rocket_core_init(struct rocket_core *core) > return err; > } > > + /* > + * RK3576: the NPU spans TWO power domains (PD_NPU0 + PD_NPU1). > [ ... ] > + if (core->soc->multi_power_domain) { > + struct dev_pm_domain_list *pd_list; > + > + err = devm_pm_domain_attach_list(dev, NULL, &pd_list); > + if (err < 0) > + return dev_err_probe(dev, err, > + "failed to attach NPU power > domains\n"); > + } > + [Severity: High] Does this early return leak resources? In rocket_core_init(), rocket_job_init() is called earlier, which allocates non-devm resources such as an ordered workqueue and a DRM scheduler. If devm_pm_domain_attach_list() fails, the function returns directly with dev_err_probe() and skips the necessary rocket_job_fini() and iommu_group_put() cleanup. > diff --git a/drivers/accel/rocket/rocket_drv.c > b/drivers/accel/rocket/rocket_drv.c > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -260,7 +276,7 @@ static int rocket_device_runtime_suspend(struct device > *dev) > if (!rocket_job_is_idle(&rdev->cores[core])) > return -EBUSY; > > - clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), > rdev->cores[core].clks); > + clk_bulk_disable_unprepare(rdev->cores[core].soc->num_clks, > rdev->cores[core].clks); > [Severity: High] This is a pre-existing issue, but does this power down the device without synchronizing the shared IRQ? Since the driver uses IRQF_SHARED, another device sharing the IRQ line could trigger the interrupt handler just as the clocks are being disabled. Can this race condition cause the IRQ handler to execute on powered-down hardware, potentially leading to a kernel panic? Should we call synchronize_irq() before turning off the clocks? > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -326,12 +360,55 @@ static struct dma_fence *rocket_job_run(struct > drm_sched_job *sched_job) > [ ... ] > +static void rocket_job_handle_irq(struct rocket_core *core) > +{ > + u32 clear = 0x1ffff; > + > + if (core->soc->poll_completion) { > + /* Stop the completion poll -- we're handling it now. */ > + atomic_set(&core->poll_active, 0); > + hrtimer_cancel(&core->poll_timer); > + clear |= PC_INTERRUPT_MASK_RK3576_PC_DONE_0 | > + PC_INTERRUPT_MASK_RK3576_PC_DONE_1; > + } > + > pm_runtime_mark_last_busy(core->dev); > > rocket_pc_writel(core, OPERATION_ENABLE, 0x0); > - rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); > + rocket_pc_writel(core, INTERRUPT_CLEAR, clear); > [Severity: High] Can a DMA error on RK3576 trigger a race condition that corrupts hardware state? On RK3576, a DMA error drops OPERATION_ENABLE to 0 and raises a DMA error interrupt. This simultaneously triggers the threaded IRQ handler and the poll timer workqueue, which will both see OPERATION_ENABLE == 0. Both threads will concurrently invoke rocket_job_handle_irq(). Because this function clears OPERATION_ENABLE outside the job_lock, one thread can write 0 to OPERATION_ENABLE right after the other thread has already submitted the next job and written 1. Does this permanently halt the hardware by leaving the newly submitted job disabled? > scoped_guard(mutex, &core->job_lock) > if (core->in_flight_job) { > [ ... ] > iommu_detach_group(NULL, iommu_group_get(core->dev)); [Severity: High] This is a pre-existing issue, but does this leak the IOMMU group reference? The call to iommu_group_get() increments the reference count, but iommu_detach_group() does not consume or release it. Since job completion is a hot path, will this quickly overflow the refcount or leak memory? Should the driver just use the already cached core->iommu_group instead? > @@ -424,9 +514,22 @@ static irqreturn_t rocket_job_irq_handler(int irq, void > *data) > { > struct rocket_core *core = data; > u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS); [Severity: High] This is a pre-existing issue, but can this unconditional MMIO read cause a bus fault if the device is suspended? Because the driver uses IRQF_SHARED, another device could trigger rocket_job_irq_handler() while the NPU is runtime suspended and powered down. Reading from a powered-down device typically returns garbage data or causes a synchronous external abort. Should we use pm_runtime_get_if_active() to verify the device is powered before touching MMIO registers? > @@ -486,6 +593,10 @@ int rocket_job_init(struct rocket_core *core) > [ ... ] > err_sched: > drm_sched_fini(&core->sched); [Severity: High] This is a pre-existing issue, but can calling drm_sched_fini() here cause a kernel panic? If drm_sched_init() fails internally (e.g., due to memory allocation failure), the waitqueues and lists inside the scheduler structure are left uninitialized. When we jump to the err_sched label, drm_sched_fini() will unconditionally try to wake up these uninitialized waitqueues. > @@ -486,6 +593,10 @@ int rocket_job_init(struct rocket_core *core) > > void rocket_job_fini(struct rocket_core *core) > { > + atomic_set(&core->poll_active, 0); > + hrtimer_cancel(&core->poll_timer); > + cancel_work_sync(&core->poll_work); > + > drm_sched_fini(&core->sched); > [Severity: High] Is there a teardown order race that allows the hrtimer to be re-armed after cancellation? The poll timer and workqueue are cancelled before stopping the DRM scheduler. If a concurrent scheduler thread pulls a pending job and calls rocket_job_hw_submit(), it will restart poll_timer. Can this lead to a use-after-free when the timer fires on freed memory after teardown completes? Should we stop the scheduler before cancelling the timer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
