Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
New issues: - [High] Hardware reset triggered while polling timer and workqueue are still active. - [High] Unclocked MMIO writes in `rocket_poll_work_fn` due to stale sequence numbers. - [High] Bypassing job completion in `rocket_poll_work_fn` during teardown leads to deadlocks and resource leaks. - [High] Concurrent execution of `rocket_poll_work_fn` and `rocket_job_handle_irq` corrupts hardware state and skips tasks. - [High] Late hardware interrupts re-arm the polling timer after teardown, causing a Use-After-Free. Pre-existing issues: - [High] Unclocked MMIO access in shared IRQ handler (`rocket_job_irq_handler`). - [High] Reset workqueue can be used after destruction due to late IRQ firing. - [High] IOMMU group reference leak on every job completion. - [High] DRM device is registered and exposed to userspace before core hardware and schedulers are initialized. - [High] Unbinding a non-terminal core corrupts `rdev->num_cores` and leads to state overwrite. -- --- Patch [6]: [RFC PATCH v5 v5 6/8] accel/rocket: add RK3576 NPU (RKNN) support --- commit 49430ea8bb109098d159ff322aab30ccbfc4c20a Author: Jiaxing Hu <[email protected]> accel/rocket: add RK3576 NPU (RKNN) support The RK3576 carries the same RKNN block as the RK3588, with two cores instead of three and a few platform differences: The patch adds RK3576 support, managing its separate clock domains, power domains, and polling the completion interrupt via an hrtimer. > diff --git a/drivers/accel/rocket/rocket_device.c > b/drivers/accel/rocket/rocket_device.c > --- a/drivers/accel/rocket/rocket_device.c > +++ b/drivers/accel/rocket/rocket_device.c > @@ -31,6 +31,10 @@ struct rocket_device *rocket_device_init(struct > platform_device *pdev, [ ... ] > err = drm_dev_register(ddev, 0); > if (err) > return ERR_PTR(err); > > return rdev; > } [Severity: High] This is a pre-existing issue, but does rocket_device_init() expose the DRM device to userspace before the hardware is actually ready? It looks like drm_dev_register() is called here, exposing the device, but rocket_probe() only initializes the core hardware and schedulers via rocket_core_init() afterward. Could userspace race to open the device and submit jobs before the schedulers and memory maps are initialized? > 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 > @@ -176,6 +176,7 @@ static int rocket_probe(struct platform_device *pdev) > > rdev->cores[core].rdev = rdev; > rdev->cores[core].dev = &pdev->dev; > + rdev->cores[core].soc = of_device_get_match_data(&pdev->dev); > rdev->cores[core].index = core; > > rdev->num_cores++; [Severity: High] This is a pre-existing issue, but does the core removal logic in rocket_remove() corrupt the device state if a non-terminal core is unbound? If rocket_remove() is called for core 0 on a 2-core setup, it blindly decrements rdev->num_cores to 1. If another core is then bound, rocket_probe() will use rdev->num_cores (1) as the index, overwriting the active rdev->cores[1] structure currently in use. > 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 [ ... ] > +static enum hrtimer_restart rocket_poll_timer_fn(struct hrtimer *timer) > +{ > + struct rocket_core *core = container_of(timer, struct rocket_core, > poll_timer); > + u32 raw; > + > + if (!atomic_read(&core->poll_active)) > + return HRTIMER_NORESTART; > + > + core->poll_work_seq = core->poll_seq; > + > + raw = rocket_pc_readl(core, INTERRUPT_RAW_STATUS); [Severity: High] Can the hardware reset logic race with this polling timer and workqueue? When a job times out, the DRM scheduler calls rocket_reset(), which triggers rocket_core_reset() without first stopping the active poll timer or work. If the timer fires or the work executes concurrently, will they attempt to read or write MMIO registers on a device undergoing reset or without active clocks? > + if ((raw & (PC_INTERRUPT_RAW_STATUS_DPU_0 | > PC_INTERRUPT_RAW_STATUS_DPU_1)) || > + ++core->poll_ticks >= RK3576_POLL_MAX_TICKS) { > + atomic_set(&core->poll_active, 0); > + schedule_work(&core->poll_work); > + return HRTIMER_NORESTART; > + } > + > + hrtimer_forward_now(timer, ns_to_ktime(RK3576_POLL_INTERVAL_NS)); > + return HRTIMER_RESTART; > +} > + > +/* Start the job's next task, or retire it. Caller holds job_lock. */ > +static void rocket_job_next_locked(struct rocket_core *core) > +{ > + lockdep_assert_held(&core->job_lock); > + > + if (!core->in_flight_job) > + return; > + > + if (core->in_flight_job->next_task_idx < > core->in_flight_job->task_count) { > + rocket_job_hw_submit(core, core->in_flight_job); > + return; > + } > + > + iommu_detach_group(NULL, iommu_group_get(core->dev)); [Severity: High] This is a pre-existing issue, but does calling iommu_group_get() here leak the IOMMU group reference? It appears iommu_group_get() acquires a reference that is passed to iommu_detach_group(), but iommu_detach_group() does not consume the reference, and there is no matching iommu_group_put(). Will this cause the reference count to overflow or permanently pin IOMMU resources? > + dma_fence_signal(core->in_flight_job->done_fence); > + pm_runtime_put_autosuspend(core->dev); > + core->in_flight_job = NULL; > +} > + > +static void rocket_poll_work_fn(struct work_struct *work) > +{ > + struct rocket_core *core = container_of(work, struct rocket_core, > poll_work); > + > + pm_runtime_mark_last_busy(core->dev); > + > + scoped_guard(mutex, &core->job_lock) { > + /* > + * The interrupt can land while this work is queued, retire the > job > + * and start the next task. poll_seq only moves under job_lock, > in > + * hw_submit, so comparing it here says whether that happened. > Doing > + * it outside the lock would leave the window open rather than > close > + * it, and this work would then submit a task on top of a live > one. > + */ > + if (READ_ONCE(core->poll_dying) || core->poll_work_seq != > core->poll_seq) > + return; [Severity: High] Does returning early here on poll_dying cause a resource leak and deadlock? If poll_work_fn() returns early without calling rocket_job_next_locked(), it looks like the done_fence of the currently executing job is never signaled, the PM runtime reference is never released, and the IOMMU group is never detached. [Severity: High] Can this sequence check incorrectly pass if the last task of a job completes via a shared IRQ? If rocket_job_handle_irq() finishes the last task, it drops the PM runtime reference and clears in_flight_job. However, because rocket_job_hw_submit() isn't called, poll_seq is not updated. When this work function subsequently runs, it passes the sequence check (poll_work_seq == poll_seq) and proceeds to write to hardware. Since the PM reference was already dropped, could this cause a fatal unclocked MMIO access? > + > + rocket_pc_writel(core, OPERATION_ENABLE, 0x0); > + rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); > + > + rocket_job_next_locked(core); > + } > +} > + > static void rocket_job_handle_irq(struct rocket_core *core) > { > + if (core->soc->poll_completion) { > + atomic_set(&core->poll_active, 0); > + hrtimer_cancel(&core->poll_timer); > + } > + > pm_runtime_mark_last_busy(core->dev); > > rocket_pc_writel(core, OPERATION_ENABLE, 0x0); [Severity: High] Can this hardware write outside the job_lock race with task submission? The rocket_job_handle_irq() function writes 0 to OPERATION_ENABLE before acquiring the job_lock. If poll_work_fn() is concurrently inside the lock submitting the next task (which sets OPERATION_ENABLE to 1), the IRQ handler can immediately overwrite it to 0, disabling the new task. The IRQ handler would then acquire job_lock and call rocket_job_next_locked(), which retires the newly started task without it ever executing. > 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); > - return; > - } > - > - iommu_detach_group(NULL, iommu_group_get(core->dev)); > - dma_fence_signal(core->in_flight_job->done_fence); > - pm_runtime_put_autosuspend(core->dev); > - core->in_flight_job = NULL; > - } > + rocket_job_next_locked(core); > } [ ... ] > @@ -460,6 +540,10 @@ int rocket_job_init(struct rocket_core *core) > int ret; > > INIT_WORK(&core->reset.work, rocket_reset_work); > + INIT_WORK(&core->poll_work, rocket_poll_work_fn); > + hrtimer_setup(&core->poll_timer, rocket_poll_timer_fn, CLOCK_MONOTONIC, > + HRTIMER_MODE_REL); > + atomic_set(&core->poll_active, 0); > spin_lock_init(&core->fence_lock); > mutex_init(&core->job_lock); > > @@ -501,8 +585,23 @@ int rocket_job_init(struct rocket_core *core) [Severity: High] This is a pre-existing issue, but does the shared IRQ handler perform unclocked MMIO access? The rocket_job_irq_handler (which calls rocket_job_handle_irq) is registered with IRQF_SHARED. If it reads INTERRUPT_RAW_STATUS immediately without checking if the device's power domain and clocks are active, a shared interrupt firing while the NPU is runtime suspended could cause a synchronous external abort. > void rocket_job_fini(struct rocket_core *core) > { > + /* > + * Stop the poll from starting hardware work before tearing anything > + * down: it submits the next task, and drm_sched_fini() does not wait > + * for work already queued. Cancel after the scheduler is gone, so a > + * job running now cannot re-arm the timer behind the cancel. > + */ > + if (core->soc->poll_completion) > + WRITE_ONCE(core->poll_dying, true); > + > drm_sched_fini(&core->sched); > > + if (core->soc->poll_completion) { > + atomic_set(&core->poll_active, 0); > + hrtimer_cancel(&core->poll_timer); > + cancel_work_sync(&core->poll_work); > + } [Severity: High] Can a late hardware interrupt re-arm the polling timer after teardown? While hrtimer_cancel() is called here, the hardware IRQ is not disabled. If an interrupt fires after this point, rocket_job_handle_irq() will execute, progress the job, and call rocket_job_hw_submit(), which re-arms the timer via hrtimer_start(). Once the rocket_core structure is freed, could the armed timer fire and access freed memory? > + > cancel_work_sync(&core->reset.work); > destroy_workqueue(core->reset.wq); > } [Severity: High] This is a pre-existing issue, but can the reset workqueue be used after it is destroyed? Because the shared IRQ remains active until devres cleans it up after rocket_remove(), a late interrupt hitting the MAC interrupt condition could call queue_work() on this destroyed workqueue, leading to a kernel panic. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
