Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] Unconditional clock and reset requests break driver initialization on RK3588. - [High] Unhandled completion interrupts cause an interrupt storm on RK3588. - [High] Using cancel_work() in rocket_reset() causes a race condition and hardware state corruption. - [High] TOCTOU race in rocket_job_timedout() allows MMIO reads on a suspended device. - [Medium] DPU and DPU_RDMA pre-arming logic is missing despite claims in the commit message. Pre-existing issues: - [High] Shared IRQ handler unconditionally accesses MMIO without verifying the device is active. -- --- Patch [7]: [RFC PATCH 7/9] accel/rocket: add RK3576 NPU (RKNN) support --- commit 44e533d6c3c461a28b2f0fb3ee35a8a9e75c974a Author: Jiaxing Hu <[email protected]> accel/rocket: add RK3576 NPU (RKNN) support Add RK3576 support to the rocket DRM accelerator driver (used with the Mesa Teflon TFLite delegate). [ ... ] > - map the DPU (0x4000) and DPU_RDMA (0x5000) blocks and pre-arm them [Severity: Medium] The commit message mentions mapping and pre-arming the DPU and DPU_RDMA blocks, and the accessor macros like rocket_dpu_writel are defined in rocket_core.h. However, looking at rocket_job_hw_submit() in rocket_job.c, there are no writes to these registers. Was the pre-arming logic omitted by accident? Without it, might the compute path stall after loading one slice on RK3576? > diff --git a/drivers/accel/rocket/rocket_core.c > b/drivers/accel/rocket/rocket_core.c > --- a/drivers/accel/rocket/rocket_core.c > +++ b/drivers/accel/rocket/rocket_core.c > @@ -22,12 +23,23 @@ int rocket_core_init(struct rocket_core *core) > int err = 0; > > core->resets[0].id = "srst_a"; > - core->resets[1].id = "srst_h"; > err = devm_reset_control_bulk_get_exclusive(&pdev->dev, > ARRAY_SIZE(core->resets), > core->resets); [Severity: High] Will this break initialization on RK3588? The srst_h reset line has been removed from the array, meaning the RK3588 hardware might not be fully reset during probe. > if (err) > return dev_err_probe(dev, err, "failed to get resets for core > %d\n", core->index); > > + core->clks[0].id = "aclk"; > + core->clks[1].id = "hclk"; > + core->clks[2].id = "npu"; > + core->clks[3].id = "pclk"; > + /* > + * RK3576: the CBUF (convolution buffer) has its own clock domain. The > CNA > + * fills the CBUF and CORE reads from it; without these the compute path > + * stalls after loading one slice (RDMA, which bypasses the CBUF, still > + * runs). The vendor keeps all NPU clocks on whenever powered. > + */ > + core->clks[4].id = "aclk_cbuf"; > + core->clks[5].id = "hclk_cbuf"; > err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks); [Severity: High] Does this unconditional request for RK3576-specific clocks cause the probe to fail on RK3588? If aclk_cbuf and hclk_cbuf do not exist in the RK3588 device tree, devm_clk_bulk_get() will return -ENOENT and break support for the older platform. [ ... ] > 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 [ ... ] > @@ -137,8 +148,21 @@ static void rocket_job_hw_submit(struct rocket_core > *core, struct rocket_job *jo > rocket_pc_writel(core, REGISTER_AMOUNTS, > PC_REGISTER_AMOUNTS_PC_DATA_AMOUNT((task->regcmd_count > + 1) / 2 - 1)); > > - rocket_pc_writel(core, INTERRUPT_MASK, PC_INTERRUPT_MASK_DPU_0 | > PC_INTERRUPT_MASK_DPU_1); > - rocket_pc_writel(core, INTERRUPT_CLEAR, PC_INTERRUPT_CLEAR_DPU_0 | > PC_INTERRUPT_CLEAR_DPU_1); > + /* Enable DMA-error interrupts; PC_DONE (bits 28-29) is polled, see > above. */ > + rocket_pc_writel(core, INTERRUPT_MASK, > + PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1 > | > + PC_INTERRUPT_MASK_CORE_0 | PC_INTERRUPT_MASK_CORE_1 > | > + PC_INTERRUPT_MASK_PPU_0 | PC_INTERRUPT_MASK_PPU_1 > | > + PC_INTERRUPT_MASK_CNA_CSC_0 | > PC_INTERRUPT_MASK_CNA_CSC_1 | > + PC_INTERRUPT_MASK_DMA_READ_ERROR | > + PC_INTERRUPT_MASK_DMA_WRITE_ERROR); [Severity: High] Could this lead to an interrupt storm on RK3588? In rocket_job_hw_submit(), completion interrupts like DPU_0 and CORE_0 are unmasked in hardware. However, rocket_job_irq_handler() now masks INTERRUPT_RAW_STATUS to only check DMA errors and returns IRQ_NONE. This means the completion bits are never cleared and the interrupt is never acknowledged. [ ... ] > @@ -326,56 +352,99 @@ static struct dma_fence *rocket_job_run(struct > drm_sched_job *sched_job) [ ... ] > static void > rocket_reset(struct rocket_core *core, struct drm_sched_job *bad) > { > + struct rocket_job *job; > + > if (!atomic_read(&core->reset.pending)) > return; > > + atomic_set(&core->poll_active, 0); > + hrtimer_cancel(&core->poll_timer); > + cancel_work(&core->poll_work); [Severity: High] Is it safe to use the non-blocking cancel_work() here instead of cancel_work_sync()? If a job timeout occurs while poll_work is actively executing on another CPU, it will continue running alongside the reset sequence. Since rocket_job_handle_irq() writes to OPERATION_ENABLE outside of the job_lock, this unguarded write could abruptly halt a newly submitted job after the reset completes. [ ... ] > @@ -385,7 +454,14 @@ static enum drm_gpu_sched_stat > rocket_job_timedout(struct drm_sched_job *sched_j > struct rocket_device *rdev = job->rdev; > struct rocket_core *core = sched_to_core(rdev, sched_job->sched); > > - dev_err(core->dev, "NPU job timed out"); > + if (pm_runtime_active(core->dev)) > + dev_err(core->dev, > + "NPU job timed out: RAW_STATUS=0x%08x MASK=0x%08x > OP_EN=0x%08x\n", > + rocket_pc_readl(core, INTERRUPT_RAW_STATUS), > + rocket_pc_readl(core, INTERRUPT_MASK), > + rocket_pc_readl(core, OPERATION_ENABLE)); > + else > + dev_err(core->dev, "NPU job timed out (device not active)\n"); [Severity: High] Is there a race condition here that could allow MMIO reads on a suspended device? The lockless pm_runtime_active() check does not take a reference to prevent suspension. If the device autosuspends in the narrow window between the check and the rocket_pc_readl() calls, this might trigger a bus fault. Would using pm_runtime_get_if_active() be safer? [ ... ] > @@ -420,14 +496,16 @@ 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 since the IRQ handler is registered with IRQF_SHARED, could reading INTERRUPT_RAW_STATUS unconditionally cause a kernel panic if the device is runtime suspended? If another device on the same IRQ line fires, reading unpowered MMIO registers will trigger a synchronous external abort. Should this check pm_runtime_get_if_active() first? > + /* Only bits 0-13 (DMA errors) can raise this IRQ; PC_DONE is polled. */ > + u32 active = raw_status & 0x3fff; > + > + if (!active) > + return IRQ_NONE; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
