Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] accel: ethosu: double ethosu_device_suspend() call in ethosu_remove() --
commit 8f693e6f09e933d0b127a2a725128f9cb43526dc Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Ensure suspended on removal This commit adds a runtime PM barrier and a direct call to ethosu_device_suspend() in ethosu_remove() when the device is not runtime suspended, aiming to ensure clocks are disabled on device removal. > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index 38af57807cee7..ae7ebf29cb2f6 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -387,6 +387,11 @@ static void ethosu_remove(struct platform_device *pdev) > > drm_dev_unregister(ðosudev->base); > ethosu_job_fini(ethosudev); > + > + pm_runtime_barrier(&pdev->dev); > + if (!pm_runtime_status_suspended(&pdev->dev)) > + ethosu_device_suspend(&pdev->dev); [Severity: High] Can calling ethosu_device_suspend() directly here while runtime PM is still enabled and runtime_status is RPM_ACTIVE cause ethosu_device_suspend() to be invoked a second time during devres teardown, underflowing the clock enable and prepare counts? In ethosu_init() in drivers/accel/ethosu/ethosu_drv.c, runtime PM is configured with autosuspend and devres: 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); During driver unbind or device removal under CONFIG_PM=y with power/control=auto, __device_release_driver() calls pm_runtime_get_sync() and pm_runtime_put_sync() before invoking ethosu_remove(), which leaves the device in RPM_ACTIVE with usage_count == 0 and the autosuspend timer armed. In ethosu_remove(), pm_runtime_barrier() cancels the autosuspend timer, and ethosu_device_suspend() then runs: clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks); However, runtime PM remains enabled (disable_depth == 0) and runtime_status remains RPM_ACTIVE. Immediately after ethosu_remove() returns, __device_release_driver() -> device_unbind_cleanup() -> devres_release_all() runs pm_runtime_disable_action() in drivers/base/power/runtime.c: static void pm_runtime_disable_action(void *data) { pm_runtime_dont_use_autosuspend(data); pm_runtime_disable(data); } Because pm_runtime_dont_use_autosuspend() runs before pm_runtime_disable(), it calls __pm_runtime_use_autosuspend(dev, false) -> update_autosuspend() -> rpm_idle(dev, RPM_AUTO) -> rpm_suspend(dev, RPM_AUTO). Since disable_depth is 0, usage_count is 0, use_autosuspend is now false, and runtime_status is still RPM_ACTIVE, rpm_suspend() synchronously invokes ethosu_device_suspend() a second time, calling clk_bulk_disable_unprepare() on clocks that were already disabled and unprepared. Also, while runtime_status remains RPM_ACTIVE after ethosu_device_suspend() has disabled the clocks, could an open DRM file descriptor closing or issuing an ioctl concurrently (such as ethosu_postclose() -> ethosu_perfmon_close_file() -> ethosu_perfmon_stop_locked() checking pm_runtime_get_if_active(), or ethosu_ioctl_perfmon_get_values() calling pm_runtime_resume_and_get()) see RPM_ACTIVE and access ethosu->pmu_regs with the NPU clocks disabled? Would calling pm_runtime_dont_use_autosuspend(&pdev->dev) and pm_runtime_disable(&pdev->dev) before checking pm_runtime_status_suspended() (and calling pm_runtime_set_suspended(&pdev->dev) after ethosu_device_suspend()) avoid both issues? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
