Re: [PATCH v7 08/15] drm/msm/dp: break up dp_display_enable into two parts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing error return when sink_count == 0 leads to unclocked hardware accesses and kernel panic.
- [High] Missing early return when link training fails causes stream enablement on a broken link.
--

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

drm/msm/dp: break up dp_display_enable into two parts

This commit splits dp_display_enable() into two parts: prepare and
enable APIs. This allows inserting MST payload functions between enabling
stream clocks and programming the registers.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 155beefdfdef2..2cf4c984c7ca7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -617,7 +617,40 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
>  	return 0;
>  }
>  
> -static int msm_dp_display_enable(struct msm_dp_display_private *dp, bool force_link_train)
> +static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
> +{
> +	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
> +	int rc = 0;
> +	bool force_link_train = false;
> +
> +	drm_dbg_dp(dp->drm_dev, "sink_count=%d\n", dp->link->sink_count);
> +
> +	if (msm_dp_display->is_edp)
> +		msm_dp_hpd_plug_handle(dp);
> +
> +	rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
> +	if (rc) {
> +		DRM_ERROR("failed to pm_runtime_resume\n");
> +		return rc;
> +	}
> +
> +	if (dp->link->sink_count == 0)
> +		return rc;

[Severity: Critical]
Does this unintentionally allow the enable sequence to continue when the
sink_count is 0?

Since pm_runtime_resume_and_get() succeeded, rc is 0 here. Returning 0
causes the caller, msm_dp_bridge_atomic_enable(), to continue directly into
msm_dp_display_enable() because the prepare step appears successful:

msm_dp_bridge_atomic_enable() {
    ...
    rc = msm_dp_display_prepare_link(dp);
    if (rc) {
        DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
        return;
    }

    rc = msm_dp_display_enable(dp);
    ...
}

Because msm_dp_ctrl_on_link() was skipped, the DP link clocks are not
enabled. Will the subsequent call to msm_dp_ctrl_on_stream() within
msm_dp_display_enable() then perform an unclocked hardware access when
writing to REG_DP_STATE_CTRL, potentially triggering a bus stall and kernel
panic?

> +
> +	if (!msm_dp_display->power_on) {
> +		msm_dp_display_host_phy_init(dp);
> +		force_link_train = true;
> +	}
> +
> +	rc = msm_dp_ctrl_on_link(dp->ctrl);
> +	if (rc)
> +		DRM_ERROR("Failed link training (rc=%d)\n", rc);
> +	// TODO: schedule drm_connector_set_link_status_property()
> +
> +	return msm_dp_ctrl_prepare_stream_on(dp->ctrl, force_link_train);
> +}

[Severity: High]
If msm_dp_ctrl_on_link() fails, does this mask the failure and force the
pipeline to stream video over a broken link?

In the original code, an error from msm_dp_ctrl_on_link() would explicitly
return and abort the atomic enable sequence. Here, the error is logged, but
execution falls through to msm_dp_ctrl_prepare_stream_on(), which often
returns 0. 

If it returns 0, the caller receives a success status and proceeds to
configure and start the video stream on an uninitialized link.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=8