Claude review: drm/msm/dp: rework HPD handling

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

Patch Review

This is the big patch — removes the entire event queue/thread, replaces with threaded IRQ, and restructures all HPD handling. The core design is good: the hardcoded IRQ handler accumulates status bits, the threaded handler dispatches `drm_bridge_hpd_notify()`, and `hpd_notify()` calls the actual plug/unplug/irq handlers.

**Issues:**

1. **Replug handling in threaded IRQ vs hpd_notify:** In the threaded IRQ handler, unplug is processed before plug:
```c
+	if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
+		drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+				      connector_status_disconnected);
+
+	if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
+		drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+				      connector_status_connected);
```
But in `hpd_notify()`, a replug is detected by checking `hpd_link_status == ISR_HPD_REPLUG_COUNT`. This means the replug path depends on reading the HPD state status bits from the register at `hpd_notify()` time, which may have changed from the original interrupt. If the replug completes quickly, the status bits may already show "connected" by the time `hpd_notify()` runs. The old code handled this by queueing both unplug and plug events with a delay, which was more robust. Consider whether this race can actually occur in practice.

2. **IRQ_HPD treated as connected:** The threaded IRQ handler sends `connector_status_connected` for IRQ_HPD:
```c
+	/* Send HPD as connected and distinguish it in the notifier */
+	if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK)
+		drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
+				      connector_status_connected);
```
Then in `hpd_notify()`, the IRQ_HPD case is handled by checking `hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT`. This overloading of `connector_status_connected` to mean both "plug" and "IRQ HPD" is fragile. If an actual plug and IRQ HPD occur at the same time, the two `hpd_notify(connected)` calls cannot be distinguished. The comment acknowledges the hack but doesn't mitigate it.

3. **Missing locking:** The `event_mutex` was removed entirely, but `msm_dp_hpd_plug_handle()`, `msm_dp_hpd_unplug_handle()`, and `msm_dp_irq_hpd_handle()` can now run concurrently (from threaded IRQ and from `hpd_notify()`). Before patch 9 adds `plugged_lock`, there is no synchronization around `link_ready` checks and state transitions. There is a window between patches 7 and 9 where the code has data races.

4. **pm_runtime imbalance in hpd_notify:** `hpd_notify()` does `pm_runtime_resume_and_get()` at the top and `pm_runtime_put_sync()` at the bottom, but `msm_dp_hpd_plug_handle()` also does its own `pm_runtime_resume_and_get()` internally. This means on a plug event, the runtime PM refcount is incremented twice — once in `hpd_notify()` and once in `msm_dp_hpd_plug_handle()`. The `hpd_notify()` then calls `pm_runtime_put_sync()` at the end, but `msm_dp_hpd_plug_handle()` only calls `pm_runtime_put_sync()` on failure. On success, the extra ref from `hpd_notify()` is dropped but the one from `plug_handle()` remains (intentionally, to keep the device powered while connected). This looks correct but is subtle and deserves a comment.

5. **msm_dp_snapshot lost locking:** The `mutex_lock(&msm_dp_display->event_mutex)` around `msm_dp_snapshot()` was removed, but the function reads hardware registers that require the device to be powered. The `power_on` check alone is racy without any lock. Consider whether `pm_runtime_get_if_active()` or similar protection is needed.

6. **`irq_set_status_flags` vs `IRQF_NO_AUTOEN`:** The patch changes from `IRQF_NO_AUTOEN` flag in `devm_request_irq()` to a separate `irq_set_status_flags(dp->irq, IRQ_NOAUTOEN)` call. Using `IRQF_NO_AUTOEN` with `devm_request_threaded_irq()` would be simpler and more conventional.

7. **Default return value changed:**
```c
-	irqreturn_t ret = IRQ_NONE;
+	irqreturn_t ret = IRQ_HANDLED;
```
The hardcoded IRQ handler now returns `IRQ_HANDLED` even when only `msm_dp_ctrl_isr()` fires (or when nothing fires at all if `hpd_isr_status` has no bits and `msm_dp_ctrl_isr` returns `IRQ_NONE`). This suppresses spurious IRQ detection. Should be `IRQ_NONE` by default, with `ret` set per-path.

---
Generated by Claude Code Patch Reviewer