Re: [PATCH v4 02/31] accel: ethosu: Ensure suspended on removal

[sashiko-bot@kernel.org]

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) <robh@kernel.org>

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(&ethosudev->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/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=2