From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
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:37:44 +1000
Message-ID: <review-patch1-20260526152025.12530-2-pmenzel@molgen.mpg.de>
In-Reply-To: <20260526152025.12530-2-pmenzel@molgen.mpg.de>
References: <20260526152025.12530-2-pmenzel@molgen.mpg.de> <20260526152025.12530-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

**Correctness =E2=80=94 epoch counter not updated on first call (minor issu=
e)**

When `drm_helper_probe_detect()` is called via the `ctx !=3D NULL` path, th=
e existing code bumps `epoch_counter` when the returned status differs from=
 `connector->status`:

```c
if (ret !=3D connector->status)
    connector->epoch_counter +=3D 1;
```

The new early return skips this entirely. On the very first call (when `con=
nector->status` is `connector_status_unknown` and force is `DRM_FORCE_OFF`)=
, the returned `connector_status_disconnected` differs from the stored stat=
us but the epoch counter is not bumped. This means callers like `check_conn=
ector_changed()` (line 988) won't see the status transition:

```c
connector->status =3D drm_helper_probe_detect(connector, NULL, false);
if (old_epoch_counter =3D=3D connector->epoch_counter) {
    ...
    return false;
}
```

In practice this is likely benign =E2=80=94 the initial status update for f=
orced connectors typically happens through `drm_helper_probe_single_connect=
or_modes()` which sets `connector->status` directly (line 592-594) before e=
ver reaching this path. But it would be more robust to update the epoch cou=
nter in the early-return path too, matching the existing pattern. Consider:

```c
if (connector->force) {
    int status;

    drm_dbg_kms(dev, "[CONNECTOR:%d:%s] force=3D%d, skipping detect\n",
                connector->base.id, connector->name,
                connector->force);
    if (connector->force =3D=3D DRM_FORCE_ON ||
        connector->force =3D=3D DRM_FORCE_ON_DIGITAL)
        status =3D connector_status_connected;
    else
        status =3D connector_status_disconnected;
    if (status !=3D connector->status)
        connector->epoch_counter +=3D 1;
    return status;
}
```

**The `ctx =3D=3D NULL` path =E2=80=94 `drm_helper_probe_detect_ctx()` also=
 has epoch counter logic**

When `ctx` is NULL, the code falls through to `drm_helper_probe_detect_ctx(=
)` which has its own `epoch_counter` bump at line 377-378. The new early re=
turn is placed *before* the `ctx` dispatch, so it correctly covers both pat=
hs. Good.

**Style nit =E2=80=94 `else` after `return`**

The `else` on line 108 is unnecessary since the `if` branch returns:

```c
if (connector->force =3D=3D DRM_FORCE_ON ||
    connector->force =3D=3D DRM_FORCE_ON_DIGITAL)
    return connector_status_connected;
else
    return connector_status_disconnected;
```

This does match the pattern used in `drm_helper_probe_single_connector_mode=
s()` (line 590-594) which also uses if/else, so it's consistent with existi=
ng code style. Not a blocker, but it could be simplified to just:

```c
if (connector->force =3D=3D DRM_FORCE_ON ||
    connector->force =3D=3D DRM_FORCE_ON_DIGITAL)
    return connector_status_connected;
return connector_status_disconnected;
```

**Debug message format =E2=80=94 `force=3D%d`**

Printing the raw enum value (`force=3D%d`) is functional but not immediatel=
y readable in logs. The log output `force=3D1` means `DRM_FORCE_OFF` (since=
 `UNSPECIFIED=3D0, OFF=3D1, ON=3D2, ON_DIGITAL=3D3`). This is fine for deve=
lopers who know the enum, but a human-readable string would be friendlier. =
Minor nit, not a blocker.

**Overall assessment**: The patch is correct and addresses a real issue. Th=
e epoch counter concern is the only substantive point =E2=80=94 it should e=
ither be handled or explicitly called out as intentionally skipped. With th=
at addressed, this is a clean, well-motivated fix.

---
Generated by Claude Code Patch Reviewer