public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm/dp: turn link_ready into plugged
Date: Mon, 25 May 2026 17:13:09 +1000	[thread overview]
Message-ID: <review-patch9-20260524-hpd-refactor-v6-9-cf3ab488dd7b@oss.qualcomm.com> (raw)
In-Reply-To: <20260524-hpd-refactor-v6-9-cf3ab488dd7b@oss.qualcomm.com>

Patch Review

Replaces the public `link_ready` boolean with a private `plugged` boolean protected by `plugged_lock` mutex.

**Bug (logic error):** In `msm_dp_bridge_get_modes()`, removing the `link_ready` guard breaks the if/else structure:

```c
-	if (dp->link_ready) {
-		rc = msm_dp_display_get_modes(dp);
-		if (rc <= 0) {
-			DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
-			return rc;
-		}
+	rc = msm_dp_display_get_modes(dp);
+	if (rc <= 0) {
+		DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
+		return rc;
 	} else {
 		drm_dbg_dp(connector->dev, "No sink connected\n");
 	}
```

The `else` now pairs with `if (rc <= 0)`, so when modes ARE successfully retrieved (`rc > 0`), it prints "No sink connected" -- exactly backwards. The `else` branch should be removed entirely since `get_modes` is now always called.

**Bug (PM refcount leak):** In `msm_dp_bridge_detect()`, when the device is plugged, the function does `pm_runtime_resume_and_get()` at the top but the cleanup block only calls `pm_runtime_put_sync()` when `!priv->plugged`:

```c
+	if (!priv->plugged) {
+		if (phy_deinit) {
+			msm_dp_aux_enable_xfers(priv->aux, false);
+			msm_dp_display_host_phy_exit(priv);
+		}
+		pm_runtime_put_sync(&dp->pdev->dev);
+	}
```

Each call to `detect()` when plugged increments the PM refcount without a corresponding put. Since `detect()` can be called multiple times (e.g., during probe, modeset, userspace polling), this will leak PM references and prevent the device from suspending.

**Issue (missing early-out):** `msm_dp_hpd_plug_handle()` removes the `if (dp->msm_dp_display.link_ready) return 0` check and doesn't replace it with a `if (dp->plugged) return 0` equivalent. Now every plug event from HPD does full re-initialization even if already plugged, including `pm_runtime_resume_and_get()` which adds another PM ref.

**Typo:** "initliazed" should be "initialized" in the kerneldoc:

```c
+ * Returns: true if this call has initliazed the PHY
```

**Typo:** "loose" should be "lose" in the comment:

```c
+	 * If we detected the DPRX, leave the controller on so that it doesn't
+	 * loose the state.
```

---

---
Generated by Claude Code Patch Reviewer

  reply	other threads:[~2026-05-25  7:13 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-24 10:33 [PATCH v6 00/10] drm/msm/dp: Drop the HPD state machine Dmitry Baryshkov
2026-05-24 10:33 ` [PATCH v6 01/10] drm/msm/dp: fix HPD state status bit shift value Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 02/10] drm/msm/dp: Fix the ISR_* enum values Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 03/10] drm/msm/dp: Read DPCD and sink count in bridge detect() Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 04/10] drm/msm/dp: Move link training to atomic_enable() Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 05/10] drm/msm/dp: Drop EV_USER_NOTIFICATION Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 06/10] drm/msm/dp: drop event data Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 07/10] drm/msm/dp: rework HPD handling Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 08/10] drm/msm/dp: Add sink_count to debug logs Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 10:33 ` [PATCH v6 09/10] drm/msm/dp: turn link_ready into plugged Dmitry Baryshkov
2026-05-25  7:13   ` Claude Code Review Bot [this message]
2026-05-24 10:33 ` [PATCH v6 10/10] drm/msm/dp: clear EDID on display unplug Dmitry Baryshkov
2026-05-25  7:13   ` Claude review: " Claude Code Review Bot
2026-05-24 21:32 ` [PATCH v6 00/10] drm/msm/dp: Drop the HPD state machine Val Packett
2026-05-25  7:13 ` Claude review: " Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-03-14  1:09 [PATCH v5 00/10] " Dmitry Baryshkov
2026-03-14  1:09 ` [PATCH v5 09/10] drm/msm/dp: turn link_ready into plugged Dmitry Baryshkov
2026-03-16  2:12   ` Claude review: " Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch9-20260524-hpd-refactor-v6-9-cf3ab488dd7b@oss.qualcomm.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox