Re: [PATCH v6 27/29] drm/msm/dp: enable MST on capable sinks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Implicit boolean evaluation of enum drm_dp_mst_mode leads to incorrectly enabling MST on SST-only sinks.
--

commit c042ac1b2a4e178e7b21389f0bc30a1309bea2b3
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

    drm/msm/dp: enable MST on capable sinks
    
    MST topology discovery requires the sink to operate in MST mode.
    Enable MST on sinks that advertise MST capability and are connected
    to a multi-stream capable DP controller.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index f350c1d03b06d..dbc0e82d333a5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -287,14 +311,17 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	if (rc)
>  		goto end;
>  
> -	drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
> -	drm_edid_connector_update(connector, drm_edid);
> +	if (msm_dp_ctrl_get_stream_cnt(dp->ctrl) <= 1 ||
> +	    !drm_dp_read_mst_cap(dp->aux, dp->panel->dpcd)) {
        ^^^^

[Severity: High]
Does this implicit boolean evaluation of the return value correctly handle
SST-only sinks with sideband messaging?

The drm_dp_read_mst_cap() function returns an enum drm_dp_mst_mode. If a sink
returns DRM_DP_SST_SIDEBAND_MSG (which has a value of 2), the ! negation will
evaluate to false. Will this cause the driver to incorrectly skip the necessary
SST EDID read block here?

> +		drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
> +		drm_edid_connector_update(connector, drm_edid);
>  
> -	if (!drm_edid) {
> -		DRM_ERROR("panel edid read failed\n");
> -		/* check edid read fail is due to unplug */
> -		if (!msm_dp_aux_is_link_connected(dp->aux))
> -			return -ETIMEDOUT;
> +		if (!drm_edid) {
> +			DRM_ERROR("panel edid read failed\n");
> +			/* check edid read fail is due to unplug */
> +			if (!msm_dp_aux_is_link_connected(dp->aux))
> +				return -ETIMEDOUT;
> +		}
>  	}
>  
>  	msm_dp_link_process_request(dp->link);
> @@ -316,6 +343,10 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	 */
>  	msm_dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>  
> +	if (msm_dp_ctrl_get_stream_cnt(dp->ctrl) > 1 &&
> +	    drm_dp_read_mst_cap(dp->aux, dp->panel->dpcd))
> +		msm_dp_display_mst_enable(dp);
        ^^^^

[Severity: High]
Similarly, will this cause DRM_DP_SST_SIDEBAND_MSG to be treated as fully
MST capable?

Since drm_dp_read_mst_cap() returns 2 for DRM_DP_SST_SIDEBAND_MSG, this
condition evaluates to true. Could this lead to incorrectly executing
msm_dp_display_mst_enable() to enable MST features on an SST sink, improperly
writing DP_MST_EN to DP_MSTM_CTRL?

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

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