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/msm/dp: rework HPD handling Date: Mon, 16 Mar 2026 12:12:57 +1000 Message-ID: In-Reply-To: <20260314-hpd-refactor-v5-7-0c8450737d64@oss.qualcomm.com> References: <20260314-hpd-refactor-v5-0-0c8450737d64@oss.qualcomm.com> <20260314-hpd-refactor-v5-7-0c8450737d64@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the big patch =E2=80=94 removes the entire event queue/thread, repl= aces with threaded IRQ, and restructures all HPD handling. The core design = is good: the hardcoded IRQ handler accumulates status bits, the threaded ha= ndler dispatches `drm_bridge_hpd_notify()`, and `hpd_notify()` calls the ac= tual plug/unplug/irq handlers. **Issues:** 1. **Replug handling in threaded IRQ vs hpd_notify:** In the threaded IRQ h= andler, 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 = =3D=3D 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 quick= ly, 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 wi= th 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 `connec= tor_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_s= tatus =3D=3D ISR_IRQ_HPD_PULSE_COUNT`. This overloading of `connector_statu= s_connected` to mean both "plug" and "IRQ HPD" is fragile. If an actual plu= g 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 mit= igate 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_hand= le()` can now run concurrently (from threaded IRQ and from `hpd_notify()`).= Before patch 9 adds `plugged_lock`, there is no synchronization around `li= nk_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 increme= nted twice =E2=80=94 once in `hpd_notify()` and once in `msm_dp_hpd_plug_ha= ndle()`. 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 failur= e. On success, the extra ref from `hpd_notify()` is dropped but the one fro= m `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 har= dware 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_reque= st_threaded_irq()` would be simpler and more conventional. 7. **Default return value changed:** ```c - irqreturn_t ret =3D IRQ_NONE; + irqreturn_t ret =3D 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 IR= Q detection. Should be `IRQ_NONE` by default, with `ret` set per-path. --- Generated by Claude Code Patch Reviewer