Re: [PATCH v5 03/10] drm/msm/dp: Read DPCD and sink count in bridge detect()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] msm_dp_bridge_detect disables AUX transfers globally on a transient runtime PM resume failure, breaking future transfers permanently.
- [High] Transient AUX errors or PM resume failures incorrectly cause the display to be reported as disconnected, tearing down the display.
--

--- Patch [3]: [PATCH v5 03/10] drm/msm/dp: Read DPCD and sink count in bridge detect() ---
commit 80bf1779215b888b4022742b582789f295f77790
Author: Jessica Zhang <[email protected]>

drm/msm/dp: Read DPCD and sink count in bridge detect()

Instead of relying on the link_ready flag to specify if DP is connected,
read the DPCD bits and get the sink count to accurately detect if DP is
connected.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5997cd28ba11f..a05144de3b93f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -1151,6 +1151,66 @@ static int msm_dp_hpd_event_thread_start(struct msm_dp_display_private *msm_dp_p
[ ... ]
> +enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
> +					       struct drm_connector *connector)
> +{
[ ... ]
> +	if (!dp->link_ready)
> +		return status;
> +
> +	msm_dp_aux_enable_xfers(priv->aux, true);
> +
> +	ret = pm_runtime_resume_and_get(&dp->pdev->dev);
> +	if (ret) {
> +		DRM_ERROR("failed to pm_runtime_resume\n");
> +		msm_dp_aux_enable_xfers(priv->aux, false);

[Severity: High]
If pm_runtime_resume_and_get() fails here, is it correct to call
msm_dp_aux_enable_xfers(priv->aux, false)?

Since enable_xfers is otherwise managed by HPD interrupt handlers (which
don't fire without physical hotplug events), setting it to false on a
transient PM resume failure seems like it would permanently disable the
AUX channel for subsequent transfers (like link training or EDID reads).
Could this render the display unusable until a physical cable replug?

> +		return status;
> +	}
> +
> +	ret = msm_dp_aux_is_link_connected(priv->aux);
> +	if (dp->internal_hpd && !ret)
> +		goto end;
> +
> +	ret = drm_dp_read_dpcd_caps(priv->aux, dpcd);
> +	if (ret)
> +		goto end;
> +
> +	ret = drm_dp_read_desc(priv->aux, &desc, drm_dp_is_branch(dpcd));
> +	if (ret)
> +		goto end;

[Severity: High]
Does returning connector_status_disconnected on transient AUX failures
cause problems?

If drm_dp_read_dpcd_caps() or drm_dp_read_desc() fail due to a transient
error (such as an I2C/AUX timeout), the code jumps to the end and returns
the disconnected status. This could cause userspace to receive a false
display disconnection event and tear down the display modeset abruptly.

> +
> +	status = connector_status_connected;
> +	if (drm_dp_read_sink_count_cap(connector, dpcd, &desc)) {
> +		int sink_count = drm_dp_read_sink_count(priv->aux);
> +
> +		drm_dbg_dp(dp->drm_dev, "sink_count = %d\n", sink_count);
> +
> +		if (sink_count <= 0)

[Severity: High]
Since drm_dp_read_sink_count() can return a negative error code (like
-ETIMEDOUT) on failure, this check evaluates to true for errors.

Would a transient read failure here also result in a false disconnection
status being reported?

> +			status = connector_status_disconnected;
> +	}
> +
> +end:
> +	pm_runtime_put_sync(&dp->pdev->dev);
> +	return status;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3