Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The driver leaks hardware clocks on removal because it fails to synchronously suspend the device or flush pending autosuspends before PM runtime is disabled. - [High] Destroying the DRM scheduler in `ethosu_remove()` while userspace holds open file descriptors leads to a Use-After-Free in `ethosu_postclose()`. - [Medium] The return value of `ethosu_sram_init()` is ignored, which can cause the driver to silently mask memory allocation failures. -- commit 527f62d6ec40e5aa68386e337081401539f1068b Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Disable clocks on PM setup failure ethosu_init() enables the clocks directly to reset and query the NPU before runtime PM takes over. If runtime-PM setup fails, the error path returns while those clocks remain enabled. Disable the clocks before returning the setup error to balance the initial direct resume. > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index c684e28ac7871..65b148e770e57 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -311,8 +311,10 @@ static int ethosu_init(struct ethosu_device *ethosudev) > pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50); > pm_runtime_use_autosuspend(ethosudev->base.dev); > ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev); > - if (ret) > + if (ret) { > + ethosu_device_suspend(ethosudev->base.dev); > return ret; > + } > pm_runtime_get_noresume(ethosudev->base.dev); > > ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + > NPU_REG_ID); [Severity: Medium] This is a pre-existing issue, but should the return value of ethosu_sram_init() be checked in ethosu_init()? drivers/accel/ethosu/ethosu_drv.c:ethosu_init() { ... ethosu_sram_init(ethosudev); if (!ethosu_is_u65(ethosudev)) ethosudev->pmu_regs += 0x1000; ... } If gen_pool_dma_alloc() fails in ethosu_sram_init(), it returns -ENOMEM. However, ethosu_init() ignores this return value and continues initialization with a 0-sized SRAM pool. If SRAM is configured in the device tree but allocation fails, the error is suppressed. [Severity: High] This is a pre-existing issue, but does this driver leak hardware clocks on removal? drivers/accel/ethosu/ethosu_drv.c:ethosu_remove() { struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev); drm_dev_unregister(ðosudev->base); ethosu_job_fini(ethosudev); if (ethosudev->sram) gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram, ethosudev->npu_info.sram_size); } It appears the remove function completes without synchronously suspending the device or flushing pending autosuspends before PM runtime is disabled. If a job completes and releases its PM reference via pm_runtime_put_autosuspend() (delay 50ms), and the device is unbound via ethosu_remove() before the autosuspend timer expires, the devm teardown sequence will call pm_runtime_disable(). This cancels pending autosuspend timers without suspending the hardware, leaving the clocks permanently enabled. As stated in the subsystem guidelines, synchronous suspend (pm_runtime_force_suspend() or pm_runtime_put_sync()) is required before pm_runtime_disable(). [Severity: High] This is a pre-existing issue, but could destroying the DRM scheduler in ethosu_remove() while userspace holds open file descriptors lead to a use-after-free? If userspace has an open file descriptor for the device and a privileged user unbinds the driver via sysfs, ethosu_remove() calls ethosu_job_fini(), which executes drm_sched_fini(&dev->sched). This frees the scheduler's runqueues (sched->sched_rq[i]). Later, when userspace closes the file descriptor, the DRM core invokes: drivers/accel/ethosu/ethosu_job.c:ethosu_job_close() { struct drm_sched_entity *entity = ðosu_priv->sched_entity; drm_sched_entity_destroy(entity); } drm_sched_entity_destroy() attempts to flush the entity by accessing the freed scheduler runqueues. This accesses entity->rq->sched, where entity->rq is a dangling pointer to the freed runqueue. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
