Re: [PATCH v4 04/31] accel: ethosu: Disable clocks on PM setup failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel: ethosu: double clock disable in ethosu_init() on PM enable failure
--

commit f5f434ba4c9847ae5b3231ccf71531bf3ccbd16b
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Disable clocks on PM setup failure

This commit calls ethosu_device_suspend() on the error path when
devm_pm_runtime_set_active_enabled() fails in ethosu_init(), aiming to
disable the clocks that were enabled earlier by ethosu_device_resume().

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 9ee144c41550c..1a65e9eef562f 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;
> +	}

[Severity: Medium]
Can this cause a double call to clk_bulk_disable_unprepare() on
ethosudev->clks if devm_pm_runtime_set_active_enabled() fails inside
devm_pm_runtime_enable()?

In drivers/accel/ethosu/ethosu_drv.c:ethosu_init(),
pm_runtime_use_autosuspend() is called and then
devm_pm_runtime_set_active_enabled() runs while dev->power.usage_count is
still 0, because pm_runtime_get_noresume() is only called after
devm_pm_runtime_set_active_enabled() succeeds.

Inside drivers/base/power/runtime.c:devm_pm_runtime_set_active_enabled():

int devm_pm_runtime_set_active_enabled(struct device *dev)
{
	int err;

	err = pm_runtime_set_active(dev);
	if (err)
		return err;

	err = devm_add_action_or_reset(dev, pm_runtime_set_suspended_action, dev);
	if (err)
		return err;

	return devm_pm_runtime_enable(dev);
}

And in drivers/base/power/runtime.c:devm_pm_runtime_enable():

static void pm_runtime_disable_action(void *data)
{
	pm_runtime_dont_use_autosuspend(data);
	pm_runtime_disable(data);
}

int devm_pm_runtime_enable(struct device *dev)
{
	pm_runtime_enable(dev);

	return devm_add_action_or_reset(dev, pm_runtime_disable_action, dev);
}

If the devm_add_action_or_reset() allocation in devm_pm_runtime_enable()
fails, it invokes pm_runtime_disable_action() while runtime PM is still
enabled, runtime_status is RPM_ACTIVE, and usage_count is 0.

pm_runtime_dont_use_autosuspend() clears use_autosuspend and calls
update_autosuspend() -> rpm_idle() -> rpm_suspend(), which synchronously
executes the runtime_suspend callback (ethosu_device_suspend()) and disables
ethosudev->clks. When devm_pm_runtime_set_active_enabled() then returns the
error to ethosu_init(), calling ethosu_device_suspend() unconditionally in
the error path will disable and unprepare ethosudev->clks a second time.

Would calling pm_runtime_get_noresume() before
devm_pm_runtime_set_active_enabled() (and pairing it with
pm_runtime_put_noidle() on error) prevent runtime PM from suspending the
device during failure cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=4