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 cleanu=
ps (`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 disa=
ble even if `phy_power_on()` failed in `dw_dp_link_enable()`, corrupting th=
e PHY power state and regulator counts.
--

--- Patch [3]: [PATCH v9 03/22] drm/bridge: synopsys: dw-dp: Fix error hand=
ling in dw_dp_link_enable() ---
commit 37d17812be83d5554aca9b8d15486591d59db100
Author: Sebastian Reichel <[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/br=
idge/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)
> =20

[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 =3D 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 =3D drm_dp_link_power_up(&dp->aux, dp->link.revision);
>  	if (ret < 0)
> -		return ret;
> +		goto err_phy_power_off;
> =20
>  	ret =3D 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);
> =20
> +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;
>  }

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803-synopsys-d=
[email protected]?part=3D3