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