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: turn link_ready into plugged Date: Mon, 16 Mar 2026 12:12:57 +1000 Message-ID: In-Reply-To: <20260314-hpd-refactor-v5-9-0c8450737d64@oss.qualcomm.com> References: <20260314-hpd-refactor-v5-0-0c8450737d64@oss.qualcomm.com> <20260314-hpd-refactor-v5-9-0c8450737d64@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 Replaces the public `link_ready` flag with a private `plugged` boolean prot= ected by `plugged_lock` mutex. Also removes `msm_dp_bridge_atomic_check()` = which returned `-ENOTCONN` when link was not ready. **Issues:** 1. **msm_dp_bridge_get_modes logic broken:** The patch changes: ```c - if (dp->link_ready) { - rc =3D msm_dp_display_get_modes(dp); - if (rc <=3D 0) { - DRM_ERROR("failed to get DP sink modes, rc=3D%d\n", rc); - return rc; - } - } else { - drm_dbg_dp(connector->dev, "No sink connected\n"); - } + rc =3D msm_dp_display_get_modes(dp); + if (rc <=3D 0) { + DRM_ERROR("failed to get DP sink modes, rc=3D%d\n", rc); + return rc; + } else { + drm_dbg_dp(connector->dev, "No sink connected\n"); + } ``` The `else` branch is now unreachable =E2=80=94 if `rc <=3D 0` the function = returns, and if `rc > 0` it falls into the `else` printing "No sink connect= ed" which is wrong (it means modes were successfully found). This should be= `if (rc > 0)` to reach the else, or the else should be removed. 2. **plug_handle unconditional pm_runtime_get:** After removing the `link_r= eady` guard: ```c - if (dp->msm_dp_display.link_ready) - return 0; + mutex_lock(&dp->plugged_lock); ``` Now `msm_dp_hpd_plug_handle()` always calls `pm_runtime_resume_and_get()` e= ven if already plugged. On repeated plug events, this will increment the pm= _runtime refcount without matching puts. The old code had `if (link_ready) = return 0` which prevented this. 3. **Error handling regression in plug_handle:** After the rework: ```c ret =3D msm_dp_display_process_hpd_high(dp); ... dp->plugged =3D true; mutex_unlock(&dp->plugged_lock); return ret; ``` Even on failure (`ret !=3D 0`), `dp->plugged` is set to `true`. This means = a failed plug will leave the device in a state where it thinks it's plugged= , preventing future plug attempts from working and leaving a stale pm_runti= me ref. 4. **Removal of atomic_check:** Removing `msm_dp_bridge_atomic_check()` tha= t returned `-ENOTCONN` when link was down means the DRM core can now attemp= t to enable a disconnected display. The commit message mentions "After remo= ving the link_ready flag, DP bridge doesn't have a distinction between link= trained and display plugged in" but doesn't explain how the "display enabl= ed while disconnected" case is handled. The `sink_count =3D=3D 0` check in = `atomic_enable()` returns early but doesn't prevent the mode set. 5. **detect() leaves device powered if plugged:** The new detect logic call= s `pm_runtime_resume_and_get()` and only calls `pm_runtime_put_sync()` if `= !priv->plugged`. This means every detect() call that finds the device plugg= ed increments pm_runtime without a put. Multiple detect() calls would stack= refs. The comment says "leave the controller on so that it doesn't lose th= e state" but this ref stacking is problematic. 6. **Typo:** "loose" should be "lose" in: ```c + * If we detected the DPRX, leave the controller on so that it doesn't + * loose the state. ``` --- Generated by Claude Code Patch Reviewer