Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Unsafe hardware access in shared IRQ handler without Runtime PM check. - [High] Missing synchronize_irq() in runtime suspend callback. - [High] SRAM is allocated but left uninitialized during probe, leading to potential use of uninitialized memory. -- --- Patch [4]: [PATCH v2 4/5] accel/ethosu: propagate SRAM initialization errors --- commit 4382c986c5c63a85ef8db5d2db6b39f2605d7154 Author: GuoHan Zhao <[email protected]> accel/ethosu: propagate SRAM initialization errors ethosu_sram_init() returns -ENOMEM if allocation from a declared SRAM pool fails. ethosu_init() ignores the error after recording the pool size. Probe then registers a device with a nonzero SRAM size and a zero sramphys address, allowing jobs to program address zero for SRAM. Propagate the error and synchronously suspend the device before the probe path releases the scheduler. Link: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index 6b97d22444a7c..c825b67e9cba0 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -331,7 +331,9 @@ static int ethosu_init(struct ethosu_device *ethosudev) > ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + > NPU_REG_ID); > ethosudev->npu_info.config = config = readl_relaxed(ethosudev->regs + > NPU_REG_CONFIG); > > - ethosu_sram_init(ethosudev); > + ret = ethosu_sram_init(ethosudev); > + if (ret) > + goto err_runtime_suspend; [Severity: High] This is a pre-existing issue, but is the SRAM left uninitialized during the first active window after probe? Earlier in ethosu_init(), we call ethosu_device_resume(), which attempts to zero the SRAM via ethosu_reset(). However, since ethosu_sram_init() hasn't run yet, SRAM isn't allocated and zeroing is skipped. If a job is submitted within the 50ms autosuspend delay window after probe finishes, pm_runtime_resume_and_get() will increment the usage counter but skip ethosu_device_resume() since the device is already active. Does this mean the first job could run with uninitialized garbage data in the SRAM, leading to unpredictable hardware behavior? > > dev_info(ethosudev->base.dev, > "Ethos-U NPU, arch v%ld.%ld.%ld, rev r%ldp%ld, cmd stream > ver%ld, %d MACs, %dKB SRAM\n", [ ... ] > @@ -346,6 +348,9 @@ static int ethosu_init(struct ethosu_device *ethosudev) > > return 0; > > +err_runtime_suspend: > + ethosu_runtime_suspend(ethosudev); > + return ret; > err_suspend: > pm_runtime_dont_use_autosuspend(ethosudev->base.dev); > ethosu_device_suspend(ethosudev->base.dev); [Severity: High] This is a pre-existing issue, but does ethosu_device_suspend() need to call synchronize_irq() before disabling the clocks? Looking at ethosu_device_suspend(), it calls clk_bulk_disable_unprepare() directly without synchronizing with the interrupt handler: drivers/accel/ethosu/ethosu_drv.c:ethosu_device_suspend() { struct ethosu_device *ethosudev = dev_get_drvdata(dev); clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks); return 0; } If the autosuspend timer expires and ethosu_device_suspend() executes while an interrupt is mid-flight on another CPU, could the clocks be disabled while the IRQ handler is actively accessing hardware registers, resulting in a bus fault? [Severity: High] This is a pre-existing issue, but is it safe for ethosu_job_irq_handler() to access hardware registers without checking if the device is active? When the device enters runtime suspend and its clocks are disabled, a different device sharing the same IRQ line could trigger an interrupt. The ethosu_job_irq_handler() unconditionally reads NPU_REG_STATUS: drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() { ... u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); ... } Without calling pm_runtime_get_if_active() to check if the device is active first, could this lead to a bus fault and kernel panic if the device is suspended and unclocked? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
