From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm/dp: add dp_mst_drm to manage DP MST bridge operations
Date: Sun, 12 Apr 2026 10:16:19 +1000
Message-ID: <review-patch34-20260410-msm-dp-mst-v4-34-b20518dea8de@oss.qualcomm.com>
In-Reply-To: <20260410-msm-dp-mst-v4-34-b20518dea8de@oss.qualcomm.com>
References: <20260410-msm-dp-mst-v4-0-b20518dea8de@oss.qualcomm.com> <20260410-msm-dp-mst-v4-34-b20518dea8de@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the largest and most complex patch =E2=80=94 the MST bridge operati=
ons.

**State access bug in atomic_check**:
```c
+	mst_state =3D to_drm_dp_mst_topology_state(mst->mst_mgr.base.state);
+	if (IS_ERR(mst_state))
+		return PTR_ERR(mst_state);
+
+	if (!dfixed_trunc(mst_state->pbn_div)) {
+		mst_state->pbn_div =3D
+			drm_dp_get_vc_payload_bw(mst_conn->dp_panel->link_info.rate,
+						 mst_conn->dp_panel->link_info.num_lanes);
+	}
```

`to_drm_dp_mst_topology_state(mst->mst_mgr.base.state)` reads the *current =
committed* state, not the state being built in this atomic transaction. Thi=
s should use `drm_atomic_get_mst_topology_state(state, &mst->mst_mgr)` to g=
et (or create) the MST state within the current atomic transaction. Modifyi=
ng the current state during atomic_check violates the atomic model and can =
cause races with concurrent modesets. Note that `to_drm_dp_mst_topology_sta=
te` also cannot return an error pointer =E2=80=94 it's a `container_of` mac=
ro =E2=80=94 so the `IS_ERR` check is also dead code.

Additionally, the same function has a similar issue in `_msm_dp_mst_bridge_=
pre_enable_part2()`:
```c
+	mst_state =3D to_drm_dp_mst_topology_state(mst->mst_mgr.base.state);
+	payload =3D drm_atomic_get_mst_payload_state(mst_state, port);
```
In pre_enable this is less dangerous since the state is committed by then, =
but `drm_atomic_get_new_mst_topology_state()` from the atomic_state paramet=
er would be more correct and consistent with `pre_enable_part1` which corre=
ctly uses it.

**Missing error check on slots**:
```c
+	slots =3D drm_dp_atomic_find_time_slots(state, &mst->mst_mgr, mst_conn->m=
st_port, pbn);
+
+	drm_dbg_dp(drm_bridge->dev, "add slots, conn:%d pbn:%d slots:%d rc:%d\n",
+		   connector->base.id, pbn, slots, rc);
```
`slots` is logged but never checked for errors (negative return). The funct=
ion returns 0 unconditionally.

---
Generated by Claude Code Patch Reviewer