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/bridge: display-connector: trigger initial HPD event for DP Date: Mon, 16 Mar 2026 12:14:39 +1000 Message-ID: In-Reply-To: <20260314-dp-connector-hpd-v1-2-786044cedc17@oss.qualcomm.com> References: <20260314-dp-connector-hpd-v1-0-786044cedc17@oss.qualcomm.com> <20260314-dp-connector-hpd-v1-2-786044cedc17@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 patch adds a `work_struct` to schedule an initial HPD notification whe= n `hpd_enable` is called for DP connectors. On `hpd_disable`, it cancels th= e work with `cancel_work_sync`. **The use of a workqueue is appropriate** because `hpd_enable` is called un= der `bridge->hpd_mutex` (see `drm_bridge_hpd_enable()` in `drm_bridge.c`), = and `drm_bridge_hpd_notify()` also takes `hpd_mutex`. Calling `drm_bridge_h= pd_notify()` directly from `hpd_enable` would deadlock. The workqueue defer= s the notification outside the mutex. **Observations:** 1. **`INIT_WORK` is conditional but callbacks are not.** The `INIT_WORK` ca= ll at line 661 is guarded by `conn->hpd_irq >=3D 0 && type =3D=3D DRM_MODE_= CONNECTOR_DisplayPort`, and the `schedule_work`/`cancel_work_sync` calls ar= e guarded by `conn->bridge.type =3D=3D DRM_MODE_CONNECTOR_DisplayPort`. Thi= s is consistent =E2=80=93 DP connectors without HPD IRQ won't reach `hpd_en= able` (since `DRM_BRIDGE_OP_HPD` won't be set), and non-DP connectors won't= touch the work. However, if `INIT_WORK` is never called, the `work_struct`= is zero-initialized (from `devm_drm_bridge_alloc` which uses `devm_kzalloc= `), and `cancel_work_sync` on an uninitialized (zero) `work_struct` is tech= nically undefined behavior. In practice, this path is unreachable for non-D= P connectors because the `bridge.type` check guards it. Still, it would be = slightly cleaner to always call `INIT_WORK` when `hpd_irq >=3D 0`, removing= the type check from the init path, since the `schedule_work` is already gu= arded by the DP type check. 2. **Race between work and disable.** The ordering in `hpd_disable` is: `ca= ncel_work_sync` first, then `disable_irq`. This is correct =E2=80=93 it ens= ures the synthetic HPD work is cancelled before the IRQ is disabled. If the= order were reversed, a pending work item could call `display_connector_det= ect` =E2=86=92 `gpiod_get_value_cansleep` after the IRQ was disabled, which= would be fine functionally but inconsistent. 3. **No cleanup on remove.** The `display_connector_remove` function doesn'= t call `cancel_work_sync`. This should be fine because `drm_bridge_hpd_disa= ble` (and thus `hpd_disable`) is called during connector cleanup before bri= dge removal, so the work should already be cancelled. But it's worth consid= ering whether there's a window where `hpd_enable` was called but `hpd_disab= le` was not before `remove`. Given the DRM lifecycle, this shouldn't happen. 4. **Minor typo in commit message:** "externall" should be "external". Overall, this is a well-structured fix for a real problem. The series is co= rrect and ready to merge with perhaps the typo fix in the commit message. --- Generated by Claude Code Patch Reviewer