Re: [PATCH v7 16/35] drm/msm/dp: add link_ready to manage link-level operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm/dp: active_stream_cnt integer underflow on link preparation failure
- [High] drm/msm/dp: PM runtime and PHY resource leak on failed link preparation
- [Low] drm/msm/dp: misleading debug message in unprepare path
--

commit 76ccdbadef8f1f0f27aff459380cb7a38f3abbd5
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: add link_ready to manage link-level operations

In MST mode, multiple streams share the same DP link. Track a link_ready
state so msm_dp_display_prepare_link() runs only once per link and
repeated calls are skipped.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 49a718f4e4649..487c76e0f990d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> @@ -1501,14 +1509,16 @@ void msm_dp_display_enable_helper(struct msm_dp *msm_dp_display,
>  
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  
> -	rc = msm_dp_display_enable(dp, msm_dp_panel);
> -	if (rc)
> -		DRM_ERROR("DP display enable failed, rc=%d\n", rc);
> +	if (msm_dp_display->link_ready) {
> +		rc = msm_dp_display_enable(dp, msm_dp_panel);
> +		if (rc)
> +			DRM_ERROR("DP display enable failed, rc=%d\n", rc);

[Severity: High]
If msm_dp_display_prepare_link() failed (e.g., sink_count == 0), link_ready
is false and we skip calling msm_dp_display_enable(). This means
active_stream_cnt is never incremented.

However, during atomic teardown, msm_dp_display_atomic_post_disable_helper()
unconditionally calls msm_dp_display_disable(), which will decrement the
counter regardless:

msm_dp_display_disable() {
    ...
    msm_dp_display->active_stream_cnt--;
    ...
}

Can this cause active_stream_cnt to underflow to 0xFFFFFFFF?

If it underflows, a subsequent call to msm_dp_display_prepare_link() would
fail to initialize the PHY because it checks:

    if (!msm_dp_display->active_stream_cnt) {
        msm_dp_display_host_phy_init(dp);
        ...

>  
> -	rc = msm_dp_display_post_enable(msm_dp_display);
> -	if (rc) {
> -		DRM_ERROR("DP display post enable failed, rc=%d\n", rc);
> -		msm_dp_display_disable(dp, msm_dp_panel);
> +		rc = msm_dp_display_post_enable(msm_dp_display);
> +		if (rc) {
> +			DRM_ERROR("DP display post enable failed, rc=%d\n", rc);
> +			msm_dp_display_disable(dp, msm_dp_panel);
> +		}
>  	}
>  
>  	drm_dbg_dp(msm_dp_display->drm_dev, "type=%d Done\n", msm_dp_display->connector_type);
> @@ -1565,6 +1575,11 @@ void msm_dp_display_unprepare(struct msm_dp *msm_dp_display)
>  
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  
> +	if (!msm_dp_display->link_ready) {
> +		drm_dbg_dp(dp->drm_dev, "Link already setup, return\n");

[Severity: Low]
Does this debug message contradict the check? It says "Link already setup",
but it is printed when link_ready is false.

[Severity: High]
If msm_dp_display_prepare_link() failed after successfully calling
pm_runtime_resume_and_get(), link_ready remains false. For example:

msm_dp_display_prepare_link() {
    ...
    rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
    ...
    if (dp->link->sink_count == 0)
        return -ENOTCONN;
    ...
}

When msm_dp_display_unprepare() is later called, this new early return
will cause us to skip the cleanup code:

    msm_dp_display_host_phy_exit(dp);
    pm_runtime_put_sync(&msm_dp_display->pdev->dev);

Will this result in leaking the PM runtime reference and leaving the PHY
initialized?

> +		return;
> +	}
> +
>  	if (msm_dp_display->active_stream_cnt) {
>  		drm_dbg_dp(dp->drm_dev, "stream still active, return\n");
>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=16