Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] The shared IRQ handler accesses hardware registers without checking if the device is active. - [High] The runtime suspend callback disables hardware clocks without synchronizing the shared IRQ. - [High] The global `rdev` pointer is not reset to NULL when initialization fails, causing a kernel panic on subsequent probes. - [High] Hardware registers are read during probe without ensuring the device resets are explicitly deasserted. -- commit e068c6d40922642be56ef8ce61fd76a8d5727d7c Author: Midgy BALON <[email protected]> accel: rocket: Introduce per-SoC rocket_soc_data Add a per-SoC data structure carried in the OF match table, currently holding only the NPU AXI address width, and use it for the per-core DMA mask instead of a hardcoded 40-bit value. > diff --git a/drivers/accel/rocket/rocket_core.c > b/drivers/accel/rocket/rocket_core.c > index b3b2fa9ba645a..09c445af7de73 100644 > --- a/drivers/accel/rocket/rocket_core.c > +++ b/drivers/accel/rocket/rocket_core.c > @@ -21,6 +22,10 @@ int rocket_core_init(struct rocket_core *core) > u32 version; > int err = 0; > > + core->soc_data = of_device_get_match_data(dev); > + if (!core->soc_data) > + return dev_err_probe(dev, -EINVAL, "missing SoC match data\n"); > + > 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), [Severity: High] This isn't a bug introduced by this patch, but I noticed hardware registers are read during probe without ensuring the device resets are explicitly deasserted. Later in rocket_core_init(): version = rocket_pc_readl(core, VERSION); If the bootloader leaves the NPU in an asserted reset state, attempting to read its registers will cause a synchronous external abort. Would it make sense to deassert the resets before accessing registers here? [ ... ] > diff --git a/drivers/accel/rocket/rocket_drv.c > b/drivers/accel/rocket/rocket_drv.c > index 8bbbce594883e..384c38e13acce 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c [ ... ] > @@ -213,8 +213,12 @@ static void rocket_remove(struct platform_device *pdev) > } > } [Severity: High] This is a pre-existing issue, but I noticed the global rdev pointer is not reset to NULL when initialization fails in rocket_probe() in this file: rdev = rocket_device_init(drm_dev, &rocket_drm_driver, soc_data); if (IS_ERR(rdev)) { ... return PTR_ERR(rdev); } If this fails during the first core's probe, an error pointer is assigned to the global rdev pointer without being cleared. On the next core's probe, the check for NULL will fail and it will blindly dereference the error pointer. Could this be addressed by clearing the global pointer on the error path? [Severity: High] This is a pre-existing issue, but the runtime suspend callback in this driver disables hardware clocks without synchronizing the shared IRQ: rocket_device_runtime_suspend() { ... clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), ...); ... } Should this call synchronize_irq() first to avoid a race condition where the IRQ handler might be executing mid-flight on another CPU while the hardware is powered down? [Severity: High] This is a pre-existing issue, but in drivers/accel/rocket/rocket_job.c, the shared IRQ handler accesses hardware registers without checking if the device is active: rocket_job_irq_handler() { ... u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS); ... } Because the IRQ is registered with IRQF_SHARED, if the handler is invoked by another device while the NPU is runtime suspended, reading the registers can result in invalid data (~0u) and spurious interrupts. Should it use pm_runtime_get_if_active() before reading the status register? [ ... ] > static const struct of_device_id dt_match[] = { > - { .compatible = "rockchip,rk3588-rknn-core" }, > + { .compatible = "rockchip,rk3588-rknn-core", .data = &rk3588_soc_data }, > {} > }; > MODULE_DEVICE_TABLE(of, dt_match); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
