Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Active devm-managed IRQ during probe error paths can trigger use-after-free or system crash due to destroyed scheduler. Pre-existing issues: - [High] IRQ handler is registered before device clocks are enabled, risking system crash on pending or spurious interrupt. -- commit 884e296c2a78cb8e0657cc20be2687762177ac12 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Fix probe error cleanup This patch unwinds initialized resources on both failure paths in probe: ethosu_init() and drm_dev_register(). It also avoids calling drm_sched_fini() after a failed drm_sched_init(). > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index df76253d01a64..c684e28ac7871 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -375,9 +375,20 @@ static int ethosu_probe(struct platform_device *pdev) > [Severity: High] This is a pre-existing issue, but is it safe that the IRQ handler is registered and unmasked before the device clocks are enabled? In ethosu_probe(), ethosu_job_init() is called before ethosu_init(): ethosu_drv.c:ethosu_probe() { ... ret = ethosu_job_init(ethosudev); if (ret) return ret; ret = ethosu_init(ethosudev); ... } Inside ethosu_job_init(), devm_request_threaded_irq() unmasks the IRQ. If a pending interrupt from a prior boot or a spurious interrupt fires, the handler ethosu_job_irq_handler() executes and immediately reads NPU_REG_STATUS. However, the hardware clocks are not enabled until ethosu_init() calls ethosu_device_resume(). Could accessing this register with disabled clocks trigger a synchronous external abort and crash the system? > ret = ethosu_init(ethosudev); > if (ret) > - return ret; > + goto err_job_fini; > > ret = drm_dev_register(ðosudev->base, 0); > + if (ret) > + goto err_sram_free; > + > + return 0; > + > +err_sram_free: > + if (ethosudev->sram) > + gen_pool_free(ethosudev->srampool, (unsigned > long)ethosudev->sram, > + ethosudev->npu_info.sram_size); > +err_job_fini: > + ethosu_job_fini(ethosudev); [Severity: High] Could the devm-managed IRQ trigger a use-after-free or a crash during this teardown window on probe failure? This patch adds ethosu_job_fini() to the probe error path. When ethosu_job_fini() executes, it tears down the DRM scheduler via drm_sched_fini(). Because the IRQ was allocated via devm_request_threaded_irq() in ethosu_job_init() and is not explicitly disabled before ethosu_job_fini(), the interrupt remains active until the probe function returns and the devres cleanup runs. If an interrupt fires in this window, the handler could access the destroyed scheduler when calling drm_sched_fault(&dev->sched). Additionally, since ethosu_init() already suspended the device on its successful path, could the handler crash the system by reading the status register with clocks disabled? > return ret; > } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
