Re: [PATCH v9 03/22] drm/bridge: synopsys: dw-dp: Fix error handling in dw_dp_link_enable()

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

New issues:
- [High] The patch introduces a double-free bug by adding error path cleanups (`phy_power_off()` and `drm_dp_link_power_down()`) in `dw_dp_link_enable()`. Because `atomic_enable` returns `void` and cannot abort a commit, the DRM core unconditionally invokes `atomic_disable` later, executing a second cleanup.

Pre-existing issues:
- [High] A preexisting bug where `phy_power_off()` is called on atomic disable even if `phy_power_on()` failed in `dw_dp_link_enable()`, corrupting the PHY power state and regulator counts.
--

--- Patch [3]: [PATCH v9 03/22] drm/bridge: synopsys: dw-dp: Fix error handling in dw_dp_link_enable() ---
commit 37d17812be83d5554aca9b8d15486591d59db100
Author: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/[email protected]>

drm/bridge: synopsys: dw-dp: Fix error handling in dw_dp_link_enable()

This commit adds missing error handling in dw_dp_link_enable(), which
failed to release resources it already requested before hitting an error.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 06997208945ed..e332d66b80571 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1636,10 +1636,19 @@ static int dw_dp_link_enable(struct dw_dp *dp)
>  

[Severity: High]
This is a pre-existing issue, but does the same unbalanced power state
happen if phy_power_on() fails?

Looking at dw_dp_link_enable(), which executes just before this hunk:

    ret = phy_power_on(dp->phy);
    if (ret)
        return ret;

If phy_power_on() fails, we return early. However, because atomic_enable()
returns void and cannot abort a commit, the DRM core will still
unconditionally call dw_dp_link_disable() later:

dw_dp_link_disable() {
    ...
    phy_power_off(dp->phy);
}

Will this unconditional cleanup drop the phy power count to -1, permanently
breaking subsequent power on attempts because the next phy_power_on()
will only increment the count to 0 and skip hardware initialization?

>  	ret = drm_dp_link_power_up(&dp->aux, dp->link.revision);
>  	if (ret < 0)
> -		return ret;
> +		goto err_phy_power_off;
>  
>  	ret = dw_dp_link_train(dp);
> +	if (ret < 0)
> +		goto err_link_power_down;
> +
> +	return 0;
> +
> +err_link_power_down:
> +	drm_dp_link_power_down(&dp->aux, dp->link.revision);
>  
> +err_phy_power_off:
> +	phy_power_off(dp->phy);

[Severity: High]
Will this explicit rollback result in phy_power_off() being called twice?

Since the DRM core will unconditionally invoke atomic_disable() later
when the commit fails, dw_dp_link_disable() will execute and perform
its own cleanup:

dw_dp_link_disable() {
    ...
    if (dw_dp_hpd_detect(dp))
        drm_dp_link_power_down(&dp->aux, dp->link.revision);
    ...
    phy_power_off(dp->phy);
}

Doing the cleanup here in the error path means these functions will be
called a second time.

Can this double cleanup cause the phy power count to drop to -1, resulting
in the same permanent breakage of the display pipeline described above?

>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-synopsys-dw-dp-improvements-v9-0-2e6e46135312-ZGY8ohtN/[email protected]?part=3