From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: imx93-mipi-dsi: Fix mode validation Date: Sat, 16 May 2026 09:49:36 +1000 Message-ID: In-Reply-To: <20260515-imx93-mipi-dsi-fix-mode-validation-v3-1-91f7d22b2fe4@nxp.com> References: <20260515-imx93-mipi-dsi-fix-mode-validation-v3-1-91f7d22b2fe4@nxp.com> <20260515-imx93-mipi-dsi-fix-mode-validation-v3-1-91f7d22b2fe4@nxp.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 **Logic change =E2=80=94 correct:** The key change from checking the last bridge to iterating any bridge after = the DSI bridge is correct. The commit message clearly explains the scenario= : the last bridge is typically a display connector bridge without ops, whil= e an intermediate bridge (e.g., an HDMI bridge chip) is the one that sets `= DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID`. **Refcount handling =E2=80=94 correct:** The helper `imx93_dsi_get_next_bridge_in_chain()` correctly drops the refer= ence on the current bridge before returning the next one: ```c static inline struct drm_bridge * imx93_dsi_get_next_bridge_in_chain(struct drm_bridge *bridge) { struct drm_bridge *next =3D drm_bridge_get_next_bridge(bridge); drm_bridge_put(bridge); return next; } ``` Both early-exit paths inside the loop body correctly call `drm_bridge_put(b= ridge)` before returning/breaking: ```c drm_bridge_put(bridge); return MODE_NOCLOCK; } drm_bridge_put(bridge); break; ``` If no bridge matches and the loop exhausts, the last iteration's `imx93_dsi= _get_next_bridge_in_chain()` returns NULL after putting the last bridge =E2= =80=94 also correct. **Locking =E2=80=94 correctly handled:** The call chain is: `drm_bridge_chain_mode_valid()` =E2=86=92 `drm_for_each_= bridge_in_chain_from()` (acquires `bridge_chain_mutex`) =E2=86=92 `dw_mipi_= dsi_bridge_mode_valid()` =E2=86=92 `imx93_dsi_mode_valid()` =E2=86=92 `imx9= 3_dsi_validate_mode()`. Since `bridge_chain_mutex` is already held, using `= drm_for_each_bridge_in_chain_from()` here would deadlock (as discovered in = v2). The manual iteration via `drm_bridge_get_next_bridge()` is safe becaus= e that function doesn't take the mutex =E2=80=94 it just traverses the list= , which is protected by the caller's lock. **Minor observation (non-blocking):** `dw_mipi_dsi_get_bridge()` returns `&dsi->bridge` =E2=80=94 a raw pointer t= o an embedded struct without incrementing the refcount. The `drm_bridge_get= _next_bridge()` API contract states "The caller is responsible of having a = reference to @bridge via drm_bridge_get() or equivalent." In practice this = is safe because the embedded bridge outlives the call, and the pre-existing= code already relied on this same pattern. But a `drm_bridge_get(dmd_bridge= )` / `drm_bridge_put(dmd_bridge)` pair would be more strictly correct. This= is pre-existing and not introduced by this patch, so not a concern for thi= s fix. **Reviewed-by: appropriate.** The patch has Frank Li's R-b tag and the fix = is straightforward and correct. --- Generated by Claude Code Patch Reviewer