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/probe-helper: honour connector->force in drm_helper_probe_detect() Date: Wed, 27 May 2026 14:18:40 +1000 Message-ID: In-Reply-To: <20260526191812.30542-2-pmenzel@molgen.mpg.de> References: <20260526191812.30542-2-pmenzel@molgen.mpg.de> <20260526191812.30542-2-pmenzel@molgen.mpg.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Bug: condition does not match the stated intent (critical)** The patch adds: ```c if (connector->force) { drm_dbg_kms(dev, "[CONNECTOR:%d:%s] forced off, skipping detect\n", connector->base.id, connector->name); return connector_status_disconnected; } ``` `connector->force` is an `enum drm_connector_force` with values: - `DRM_FORCE_UNSPECIFIED` =3D 0 - `DRM_FORCE_OFF` =3D 1 - `DRM_FORCE_ON` =3D 2 - `DRM_FORCE_ON_DIGITAL` =3D 3 Since C treats any non-zero value as truthy, `if (connector->force)` matche= s `DRM_FORCE_OFF`, `DRM_FORCE_ON`, **and** `DRM_FORCE_ON_DIGITAL` =E2=80=94= all three return `connector_status_disconnected`. This directly contradict= s the commit message which says "DRM_FORCE_ON and DRM_FORCE_ON_DIGITAL are = intentionally not short-circuited here" and the v2 changelog "Do not return= early for DRM_FORCE_ON and DRM_FORCE_ON_DIGITAL." The fix should be: ```c if (connector->force =3D=3D DRM_FORCE_OFF) { ``` **Epoch counter not incremented** When `drm_helper_probe_detect()` returns normally (no early return), lines = 413-414 handle bumping the epoch counter: ```c if (ret !=3D connector->status) connector->epoch_counter +=3D 1; ``` The new early return at the top bypasses this. For callers like `check_conn= ector_changed()` (line 988) and the poll work (line 807) that compare epoch= counters to detect changes, the early return means a forced-off connector = whose status was previously `connector_status_unknown` (initial state) will= return `connector_status_disconnected` but never bump the epoch counter. T= he caller will see no change and may not fire the hotplug event to inform u= serspace. The i915 caller `intel_hotplug_detect_connector()` (line 322) similarly use= s the epoch counter. In practice this may be fine since the status will sta= bilize quickly, but it's a behavioral inconsistency worth noting. **The `ctx =3D=3D NULL` path already has its own issues** When `ctx` is NULL, the function delegates to `drm_helper_probe_detect_ctx(= )` which also lacks any force check. The new guard covers both paths, which= is correct. **i915 already partially handles this** Worth noting: the i915 caller at `intel_hotplug.c:323` already has `if (!co= nnector->base.force) connector->base.status =3D status;` =E2=80=94 so it av= oids overwriting the status for forced connectors. The patch eliminates the= unnecessary hardware probe itself, which is the right thing to do, but the= existing guard means the practical impact on i915 was already limited to w= asted I/O (AUX/DDC transactions), not incorrect status reporting. **Debug message is misleading for the buggy condition** The debug message says "forced off, skipping detect" but with the current c= ondition it would also print for `DRM_FORCE_ON` connectors, which would be = confusing. This becomes moot once the condition is fixed to `=3D=3D DRM_FOR= CE_OFF`. **Summary:** Good idea, real problem being solved, but the patch as written= has the condition inverted from its stated intent =E2=80=94 it will break = `DRM_FORCE_ON` and `DRM_FORCE_ON_DIGITAL` connectors by returning disconnec= ted for them. The fix is straightforward: change `if (connector->force)` to= `if (connector->force =3D=3D DRM_FORCE_OFF)`. --- Generated by Claude Code Patch Reviewer