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 <[email protected]> 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/[email protected]?part=30
