Re: [PATCH v7 30/35] drm/msm/dp: enable MST on capable sinks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm/dp: DP_LINK_STATUS_CHANGED is ignored during MST, preventing link retrain
- [High] drm/msm/dp: lockless MST state modification races with unplug handling
- [Medium] drm/msm/dp: silent failure in MST enablement leaves display broken
--

commit 5689eb36d7ddcd812bb006fabf494a8f3a19f970
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>
Subject: drm/msm/dp: enable MST on capable sinks

This commit enables Multi-Stream Transport (MST) on DP sinks that
advertise MST capability. It activates the MST topology manager on
connect and disables it on disconnect. It also updates handlers to
manage the MST state lifecycle.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> @@ -269,10 +270,34 @@ static int msm_dp_display_lttpr_init(struct msm_dp_display_private *dp, u8 *dpcd
>  	return lttpr_count;
>  }
>  
> +static void msm_dp_display_mst_enable(struct msm_dp_display_private *dp)
> +{
> +	struct msm_dp *msm_dp = &dp->msm_dp_display;
> +	int ret;
> +
> +	/* Clear any pre-existing MST state before enabling MST. */
> +	ret = drm_dp_dpcd_write_byte(dp->aux, DP_MSTM_CTRL, 0);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to clear DP_MSTM_CTRL, ret=%d\n", ret);
> +		return;
> +	}
> +
> +	ret = drm_dp_dpcd_write_byte(dp->aux, DP_MSTM_CTRL,
> +				     DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC);

[Severity: High]
Does this DPCD write violate MST helper ordering? Calling this here appears
to prematurely enable the hardware, since drm_dp_mst_topology_mgr_set_mst()
internally manages this exact write when setting the state to true.

> +	if (ret < 0) {
> +		DRM_ERROR("sink MST enablement failed\n");
> +		return;
> +	}

[Severity: Medium]
If drm_dp_dpcd_write_byte() fails, this returns early without propagating
the error to the caller, leaving mst_active as false.

> +
> +	msm_dp->mst_active = true;
> +	msm_dp_mst_display_set_mgr_state(msm_dp, true);
> +}

[Severity: High]
Are these MST state variables safe to modify locklessly here?

When called in response to an IRQ, this can race concurrently with the
teardown sequence in msm_dp_hpd_unplug_handle() which modifies the same
state under the plugged_lock. Can this lead to topology manager corruption
or leaked resources?

[ ... ]

> @@ -316,6 +346,9 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	 */
>  	msm_dp_link_psm_config(dp->link, &dp->link->link_caps, false);
>  
> +	if (mst_mode == DRM_DP_MST)
> +		msm_dp_display_mst_enable(dp);
> +

[Severity: Medium]
If msm_dp_display_mst_enable() fails, the failure is ignored here. The
function returns rc = 0, assuming success, but the fallback SST EDID read
has been bypassed. Could this leave the display in a broken state with no
active topology or EDID after a hardware error?

>  	msm_dp_link_reset_phy_params_vx_px(dp->link);
>  
>  end:

[ ... ]

> @@ -499,6 +532,11 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
>  						 dp->panel->dpcd,
>  						 dp->panel->downstream_ports);
>  
> +	if (dp->msm_dp_display.mst_active) {
> +		msm_dp_mst_display_set_mgr_state(&dp->msm_dp_display, false);
> +		dp->msm_dp_display.mst_active = false;
> +	}

[Severity: High]
This modifies the mst_active state under dp->plugged_lock. Does the
lockless initialization in msm_dp_display_mst_enable() create a data race
with this sequence?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c

[ ... ]

> @@ -361,6 +361,21 @@ int msm_dp_mst_attach_encoder(struct msm_dp *dp_display, unsigned int stream_id,
>  	return 0;
>  }
>  
> +void msm_dp_mst_display_hpd_irq(struct msm_dp *dp_display)
> +{
> +	int rc;

[ ... ]

> +		rc = drm_dp_mst_hpd_irq_handle_event(&mst->mst_mgr, esi, ack, &handled);
> +		if (!handled)
> +			break;
> +
> +		rc = drm_dp_dpcd_write_byte(mst->dp_aux, esi_res, ack[1]);

[Severity: High]
If drm_dp_mst_hpd_irq_handle_event() receives a non-MST event like
LINK_STATUS_CHANGED, it sets handled to false. This breaks out of the loop
without acking the ESI.

Furthermore, the caller msm_dp_irq_hpd_handle() completely bypasses standard
link processing for MST:

msm_dp_irq_hpd_handle() {
    if (msm_dp_display->mst_active) {
        msm_dp_mst_display_hpd_irq(&dp->msm_dp_display);
        return 0;
    }
    ...
}

Does this prevent the driver from detecting link degradation and retraining
the link, potentially causing a permanent loss of display output?

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