Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/msm/dp: MST topology state corruption on early return in atomic_enable - [High] drm/msm/dp: sleeping in atomic context via mutex_lock in encoder callbacks - [High] drm/msm/dp: NULL pointer dereference in msm_dp_mst_remove_payload - [High] drm/msm/dp: kernel panic from division by zero in drm_dp_atomic_find_time_slots - [High] drm/msm/dp: hardware desynchronization and crash on atomic_enable failure --
commit b9a18785615c02ba155b20dae0aa027c5d95b815 Author: Yongxing Mou <[email protected]> drm/msm/dp: add atomic stream handling for MST This commit adds the atomic check, enable, and disable operations for MST streams. It reserves VC payload bandwidth based on the negotiated link parameters and updates the MST payload when enabling or disabling streams. The patch also serializes access to the shared MST link state. > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 533109b4365c3..933dbfd30b567 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -2747,16 +2747,29 @@ static const struct drm_encoder_helper_funcs > dpu_encoder_helper_funcs = { > static void dpu_encoder_mst_atomic_enable(struct drm_encoder *enc, > struct drm_atomic_commit *state) > { > + if (msm_dp_mst_stream_enable(enc, state)) > + return; > + [Severity: High] Does this early return cause hardware desynchronization if msm_dp_mst_stream_enable() fails? The DRM core ignores atomic_enable failures, which means the state will transition to active. The enable sequence bails out on error here, skipping the virtual encoder initialization in dpu_encoder_virt_atomic_enable(). > dpu_encoder_virt_atomic_enable(enc, state); > } > > static void dpu_encoder_mst_atomic_disable(struct drm_encoder *enc, > struct drm_atomic_commit *state) > { > + msm_dp_mst_stream_disable(enc, state); > dpu_encoder_virt_atomic_disable(enc, state); [Severity: High] Does this unconditionally access hardware during teardown, assuming dpu_encoder_virt_atomic_enable() had succeeded? Since the virtual encoder might never have been initialized or clocked if the early return in dpu_encoder_mst_atomic_enable() was taken, this call attempts to access unclocked hardware registers (e.g., waiting for TX completion) which can result in a bus error, system hang, or kernel crash. > diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c > b/drivers/gpu/drm/msm/dp/dp_mst_drm.c > index 88687b97ed6e6..fd8fd6fd10bdf 100644 > --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c > @@ -133,6 +136,45 @@ static int msm_dp_mst_connector_atomic_check(struct > drm_connector *connector, [ ... ] > +static void msm_dp_mst_remove_payload(struct msm_dp_mst *mst, > + struct drm_dp_mst_topology_state > *mst_state, > + struct msm_dp_panel *panel) > +{ > + struct msm_dp_panel *dp_panel; > + struct drm_dp_mst_atomic_payload *payload; > + int i; > + > + msm_dp_display_set_stream_info(mst->msm_dp, panel, 0, 0, 0); > + > + /* Apply the slot shift to the remaining payloads. */ > + for (i = 0; i < mst->max_streams; i++) { > + dp_panel = mst->mst_encoders[i].dp_panel; > + if (dp_panel == panel || !dp_panel->connector) > + continue; [Severity: High] Could this dereference a NULL pointer? The mst->mst_encoders array is zero-initialized. If an encoder was never attached to a specific index, dp_panel will be NULL. The || operator evaluates left-to-right. If dp_panel is NULL, the condition dp_panel == panel evaluates to false, and the code immediately evaluates !dp_panel->connector, which will dereference the NULL pointer. > + > + payload = drm_atomic_get_mst_payload_state(mst_state, [ ... ] > +int msm_dp_mst_stream_enable(struct drm_encoder *encoder, > + struct drm_atomic_commit *state) > +{ [ ... ] > + int rc; > + > + guard(mutex)(&mst->mst_lock); > + [Severity: High] Does this acquire a sleeping lock inside an atomic context? The DRM subsystem guidelines state that display encoder atomic_enable and atomic_disable callbacks execute in atomic context where sleeping is strictly forbidden. This function is called from the atomic context via dpu_encoder_mst_atomic_enable() and acquires a sleeping lock, which can cause scheduler violations, kernel warnings, and potential deadlocks. > + drm_connector_get(connector); > + panel->connector = connector; > + > + rc = msm_dp_display_set_mode_helper(dp_display, state, encoder, panel); > + if (rc) { > + drm_err(dp_display->drm_dev, > + "[MST] stream:%u set_mode failed rc=%d\n", > panel->stream_id, rc); > + return rc; > + } > + [Severity: High] Does this early return bypass drm_dp_add_payload_part1()? By returning early here, the MST payload state is left uninitialized for the later disable path. Because the DRM core does not handle atomic_enable failures, the commit completes and the payload's vc_start_slot remains -1. > + rc = msm_dp_display_prepare_link(dp_display); > + if (rc) { > + drm_err(dp_display->drm_dev, > + "[MST] stream:%u prepare_link failed rc=%d\n", > panel->stream_id, rc); > + return rc; > + } > + [Severity: High] Does this early return also bypass the required MST software state updates? > + drm_dp_mst_update_slots(mst_state, DP_CAP_ANSI_8B10B); [ ... ] > +void msm_dp_mst_stream_disable(struct drm_encoder *encoder, > + struct drm_atomic_commit *state) > +{ [ ... ] > + struct drm_dp_mst_atomic_payload *new_payload = > + drm_atomic_get_mst_payload_state(new_mst_state, > mst_conn->mst_port); > + > + guard(mutex)(&mst->mst_lock); > + [Severity: High] Is this mutex acquisition also performed in the atomic disable path? > + drm_dp_remove_payload_part1(&mst->mst_mgr, new_mst_state, new_payload); > + > + drm_dp_remove_payload_part2(&mst->mst_mgr, new_mst_state, old_payload, > new_payload); > + [Severity: High] Does this unconditional cleanup execute during teardown regardless of whether the enable succeeded? If drm_dp_add_payload_part1() was bypassed during enable, drm_dp_remove_payload_part2() will subtract old_payload->time_slots from mgr->next_start_slot and shift all payloads with a vc_start_slot greater than -1. Because the payload count was never incremented, this underflows the payload count and incorrectly shifts the slots of all active payloads, permanently corrupting the shared MST topology manager state. > + msm_dp_mst_remove_payload(mst, new_mst_state, panel); [ ... ] > +void msm_dp_mst_stream_post_disable(struct drm_encoder *encoder, > + struct drm_atomic_commit *state) > +{ [ ... ] > + struct msm_dp_panel *panel = msm_dp_mst_panel_from_encoder(mst, > encoder); > + > + guard(mutex)(&mst->mst_lock); > + [Severity: High] Is this mutex acquisition also performed in the atomic post-disable path? > + msm_dp_display_atomic_post_disable_helper(mst->msm_dp, panel); [ ... ] > +int msm_dp_mst_stream_atomic_check(struct drm_encoder *enc, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ [ ... ] > + mst_state = drm_atomic_get_mst_topology_state(crtc_state->state, > &mst->mst_mgr); > + if (IS_ERR(mst_state)) > + return PTR_ERR(mst_state); > + > + if (!dfixed_trunc(mst_state->pbn_div)) { > + mst_state->pbn_div = > + drm_dp_get_vc_payload_bw(mst->link->link_caps.rate, > + > mst->link->link_caps.num_lanes); > + } > + [Severity: High] Can this calculate the payload bandwidth without verifying if the link rate is valid, passing a zero divisor down to the DRM helpers? If link_caps.rate is 0 (which can occur if the sink is disconnected or link training failed prior to the atomic commit), drm_dp_get_vc_payload_bw() returns 0, setting pbn_div.full to 0. The code then calls drm_dp_atomic_find_time_slots(), which executes DIV_ROUND_UP(..., topology_state->pbn_div.full). Userspace can trigger an atomic commit at any time via the DRM IOCTL, and doing so when the link rate is 0 triggers a division by zero, which results in a kernel panic. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=24
