Re: [PATCH] drm/tegra: dc: Propagate powergate failures on suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Returning an error from the suspend callback causes a permanent usecount desynchronization in host1x and leaks runtime PM and clock resources.
- [High] Returning early on suspend failure leaves the clock running, but the caller unconditionally drops the performance state, violating DVFS constraints.
--

commit c243459159c6fcb1671762433a70d770456532d9
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

drm/tegra: dc: Propagate powergate failures on suspend

A lifecycle transition can continue after a required state restore
operation fails, leaving software state inconsistent with hardware.

Propagate the failure and unwind state acquired by the transition so
callers cannot observe a false success.

> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 0b3fcc7011b35..bac78b57dcffb 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -2878,8 +2878,14 @@ static int tegra_dc_runtime_suspend(struct host1x_client *client)
>  		return err;
>  	}

[Severity: High]
This is a pre-existing issue, but does returning an error from the suspend
callback cause a permanent usecount desynchronization in host1x?

If tegra_dc_runtime_suspend() fails during reset_control_assert() or
tegra_pmc_powergate_power_off(), it returns an error.

host1x_client_suspend() intercepts this error and skips decrementing
client->usecount, leaving it at 1.

The caller tegra_crtc_atomic_disable() (which returns void) cannot abort the
disable process and ignores the error.

Upon the next atomic enable, host1x_client_resume() skips calling the resume
callback because usecount is 1 (not 0), and increments it to 2.

The subsequent disable drops it to 1, skipping suspend. This permanently
disables callbacks, leaking clocks, power domains, and runtime PM references.

>  
> -	if (dc->soc->has_powergate)
> -		tegra_pmc_powergate_power_off(dc->pmc, dc->powergate);
> +	if (dc->soc->has_powergate) {
> +		err = tegra_pmc_powergate_power_off(dc->pmc, dc->powergate);
> +		if (err < 0) {
> +			dev_err(dev, "failed to powergate partition: %d\n", err);
> +			reset_control_deassert(dc->rst);
> +			return err;

[Severity: High]
This isn't a bug introduced by this patch, but does returning early on suspend
failure leave the clock running, while the caller unconditionally drops the
performance state?

If tegra_dc_runtime_suspend() returns early on failure, it skips
clk_disable_unprepare(dc->clk). The hardware clock remains enabled.

However, the caller tegra_crtc_atomic_disable() proceeds to drop the voltage:

drivers/gpu/drm/tegra/dc.c:tegra_crtc_atomic_disable() {
    ...
	err = host1x_client_suspend(&dc->client);
	if (err < 0)
		dev_err(dc->dev, "failed to suspend: %d\n", err);

	if (dc->has_opp_table) {
		err = dev_pm_genpd_set_performance_state(dc->dev, 0);
    ...
}

Operating the hardware at an active frequency with zero voltage violates DVFS
constraints and can cause hardware brownouts, potentially locking up the
system bus or hanging the SoC.

> +		}
> +	}
>  
>  	clk_disable_unprepare(dc->clk);
>  	pm_runtime_put_sync(dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829042030.21346-1-pengpeng@iscas.ac.cn?part=1