Claude review: drm/probe-helper: signal hotplug for force-enabled connectors on HPD

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**1. Malformed diff path**

The diff header shows:

```
diff --git a/drm_probe_helper.c b/drm_probe_helper.c
```

This should be `a/drivers/gpu/drm/drm_probe_helper.c`. This means `git am` will fail to apply the patch. The author likely ran `git diff` from inside `drivers/gpu/drm/` rather than from the repository root.

**2. The premise is driver-specific, not universal**

The commit message states:

> Well-behaved drivers (e.g. NVIDIA's open kernel module) check connector->force and return connector_status_connected regardless of actual hardware state.

But this is not a universal DRM convention. The DRM core's own force handling lives in `drm_helper_probe_single_connector_modes()` (the full probe path at `drm_probe_helper.c:589-606`), which explicitly checks `connector->force` and overrides the status. The detect-only path called by `check_connector_changed()` does **not** check `connector->force` — it goes straight to the driver's `detect()` callback via `detect_connector_status()`. Whether a driver respects `connector->force` in its own detect callback is driver-specific behavior, not a DRM convention. Many drivers will return the actual physical status from detect(), meaning the epoch counter **would** change and this patch's code path would never trigger.

This makes the patch's effect inconsistent across drivers — it only fires when the driver happens to check `connector->force` in its detect callback.

**3. Unconditional epoch bump on every HPD is too aggressive**

```c
if (connector->force && old_epoch_counter == connector->epoch_counter) {
    connector->epoch_counter += 1;
    ...
    return true;
}
```

Every HPD event on a forced connector will now trigger a uevent and potentially a full modeset. HPD lines can be noisy — spurious events, signal settling, etc. This could cause unnecessary modesetting storms. There's no debouncing or consideration of whether the link actually needs re-training. The commit message's scenario (TV switching inputs) is valid, but the solution is a blanket that catches all HPD events.

**4. Early return skips existing debug logging**

The patch returns `true` before reaching the existing code at line 999 that logs the epoch counter change:

```c
drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Changed epoch counter %llu => %llu\n", ...);
```

While the patch adds its own debug print, it uses different format/wording, making debug log analysis inconsistent between the forced and non-forced paths.

**5. Alternative approach to consider**

Since this is really about the nvidia-open driver's `detect()` honoring `connector->force`, it might be more appropriate to either:
- Fix the nvidia driver to not blindly return forced status from its `detect()` callback (the DRM core handles forcing elsewhere), or
- Have `detect_connector_status()` itself understand forced connectors if this is truly meant to be a core behavior.

Modifying epoch counters outside of `drm_helper_probe_detect()` creates a second code path for epoch management, which is fragile.

**6. Minor: the fix should probably also apply to `drm_helper_hpd_irq_event()`**

The commit message says "This patch addresses only the hardware HPD IRQ path," but `check_connector_changed()` is used by the per-connector `drm_connector_helper_hpd_irq_event()`. The multi-connector `drm_helper_hpd_irq_event()` (line 807) has its own inline detect+epoch logic and would have the same issue but is not addressed.

---
Generated by Claude Code Patch Reviewer