Re: [PATCH v6 16/29] 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] PM runtime reference and PHY state are leaked when msm_dp_display_prepare_link() fails after acquiring resources.
- [High] active_stream_cnt unconditionally underflows to UINT_MAX when display disable is called after a failed link preparation.
- [Low] Logically inverted debug message for !link_ready condition.
--

commit 229804790247809bb388c425e9b3744dffd08236
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 6103fa67f3ee9..90e3d2c4ef478 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -638,6 +638,10 @@ int msm_dp_display_prepare_link(struct msm_dp *msm_dp_display)
>  	if (msm_dp_display->is_edp)
>  		msm_dp_hpd_plug_handle(dp);
>  
> +	/* Serialized by modeset lock or mst_lock. */
> +	if (msm_dp_display->link_ready)
> +		return 0;
> +
>  	rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
>  	if (rc) {
>  		DRM_ERROR("failed to pm_runtime_resume\n");
> @@ -659,7 +663,11 @@ int msm_dp_display_prepare_link(struct msm_dp *msm_dp_display)
>  		}
>  	}
>  
> -	return msm_dp_ctrl_prepare_stream_on(dp->ctrl, dp->panel, force_link_train);
> +	rc = msm_dp_ctrl_prepare_stream_on(dp->ctrl, dp->panel, force_link_train);
> +	if (!rc)
> +		msm_dp_display->link_ready = true;
> +
> +	return rc;
>  }

[Severity: High]
Is there a leak of the PM runtime reference here?

If msm_dp_display_prepare_link() fails after acquiring the PM reference
but before setting link_ready to true:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_prepare_link() {
    ...
	rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
    ...
	if (dp->link->sink_count == 0)
		return -ENOTCONN;
    ...
}

It appears the new early return in msm_dp_display_unprepare() will skip
dropping the PM reference and exiting the PHY on error paths:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_unprepare() {
    ...
	if (!msm_dp_display->link_ready) {
		drm_dbg_dp(dp->drm_dev, "Link already setup, return\n");
		return;
	}
    ...
	pm_runtime_put_sync(&msm_dp_display->pdev->dev);
    ...
}

[ ... ]

> @@ -1493,14 +1501,16 @@ void msm_dp_display_enable_helper(struct msm_dp *msm_dp_display, struct msm_dp_p
>  
>  	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);
>  
> -	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);

[Severity: High]
Can active_stream_cnt underflow during teardown?

If msm_dp_display_prepare_link() fails, link_ready remains false. The call
to msm_dp_display_enable() above is skipped, and active_stream_cnt is not
incremented.

However, during teardown, msm_dp_display_disable() is called without checking
link_ready:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_disable() {
    ...
	msm_dp_display->active_stream_cnt--;
    ...
}

Will this decrement an already zero active_stream_cnt, breaking subsequent
modesets that check if (!msm_dp_display->active_stream_cnt)?

[ ... ]

> @@ -1543,6 +1553,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");
> +		return;
> +	}
> +

[Severity: Low]
Is this debug message logically inverted? It prints "Link already setup, return"
when the condition explicitly checks that the link is NOT ready
(!msm_dp_display->link_ready).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-msm-dp-mst-v6-0-c91d35d6fb9e@oss.qualcomm.com?part=16