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/msm/dp: Skip push_idle in atomic_disable if display is not powered on Date: Sat, 16 May 2026 15:42:51 +1000 Message-ID: In-Reply-To: <20260511-edp_suspend-v1-1-128555968d7c@oss.qualcomm.com> References: <20260511-edp_suspend-v1-1-128555968d7c@oss.qualcomm.com> <20260511-edp_suspend-v1-1-128555968d7c@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 **Commit message:** Well-written. It clearly explains the root cause (eDP w= ith no panel =E2=86=92 DPCD read failure =E2=86=92 `ST_DISCONNECTED` =E2=86= =92 `atomic_enable` exits early =E2=86=92 `power_on` stays false =E2=86=92 = `push_idle` writes to unpowered register =E2=86=92 NoC error). Good. **Code change:** ```c + if (!dp->power_on) + return; + msm_dp_ctrl_push_idle(msm_dp_display->ctrl); ``` **Correctness:** The guard is logically correct. If `power_on` is `false`, = the link clocks are not enabled, so writing to `REG_DP_STATE_CTRL` via `msm= _dp_ctrl_push_idle()` would access unpowered hardware. The check is consist= ent with the guard in `msm_dp_display_disable()` which does the same patter= n (`if (!msm_dp_display->power_on) return 0;`). **Minor concern =E2=80=94 early return skips post_disable flow:** The `atom= ic_disable` =E2=86=92 `atomic_post_disable` sequence means that even when w= e return early here, `msm_dp_bridge_atomic_post_disable()` will still be ca= lled. That function calls `msm_dp_display_disable()`, which has its own `po= wer_on` guard, so the early return here is safe =E2=80=94 `atomic_post_disa= ble` won't crash either. The `pm_runtime_put_sync()` at the end of `atomic_= post_disable` could be called without a matching `pm_runtime_get`, but look= ing at the enable path, `pm_runtime_get_sync` is called in `atomic_pre_enab= le`, which would also have been called before `atomic_post_disable` during = the suspend teardown. So this should be balanced correctly. **Nit =E2=80=94 comment length:** The 3-line comment is fine for explaining= a non-obvious guard, though it could be slightly more concise. This is a m= inor style preference and not a blocker. **No other issues found.** The patch is minimal, well-scoped, and addresses= a real crash. It could benefit from a `Fixes:` tag referencing the commit = that introduced the unconditional `msm_dp_ctrl_push_idle()` call in `atomic= _disable`, which would help stable backporting. --- Generated by Claude Code Patch Reviewer