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
