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/hisilicon/hibmc: fix no showing when no connectors connected Date: Fri, 13 Mar 2026 14:23:18 +1000 Message-ID: In-Reply-To: <20260312074202.1491504-3-shiyongbang@huawei.com> References: <20260312074202.1491504-1-shiyongbang@huawei.com> <20260312074202.1491504-3-shiyongbang@huawei.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 most complex patch in the series and has several issues: **1. Comment style violation:** The multi-line comment in `hibmc_vdac_detec= t()` uses the wrong style for the kernel: ```c /* If the DP connectors are disconnected, the hibmc_vdac_detect function * must return a connected state to ensure KVM display functionality. ``` This should start with `/*` on its own line per kernel coding style (though= this is a minor nit =E2=80=94 some subsystems accept this style). **2. Race / ordering dependency between DP and VDAC detect:** `hibmc_vdac_d= etect()` reads `priv->dp.phys_state` to decide whether to force VGA connect= ed. But there's no guarantee about the order in which connectors are polled= . If the VGA connector is polled/detected *before* the DP connector, `dp.ph= ys_state` could still hold a stale value from a previous cycle (or the init= ial `connector_status_disconnected`). On initial boot with DP connected, `d= p.phys_state` starts as `connector_status_disconnected`, so the first VGA d= etect would force-return connected even if DP is actually plugged in but no= t yet detected. **3. `drm_connector_helper_get_modes()` usage:** The `get_modes` callback n= ow calls `drm_connector_helper_get_modes()` which reads the EDID via the co= nnector's DDC adapter. The old code explicitly used `drm_edid_read_ddc(conn= ector, &vdac->adapter)`. This should work since the connector was initializ= ed with `drm_connector_init_with_ddc(..., &vdac->adapter)`, but it's worth = confirming that `drm_connector_helper_get_modes` will use this DDC adapter = path. **4. Clearing EDID when falling back:** The `drm_edid_connector_update(conn= ector, NULL)` call when falling back to noedid modes is good =E2=80=94 it c= lears stale EDID data. **5. `drm_dp_read_sink_count` return value reuse:** In `hibmc_dp_detect()`,= the variable `ret` is reused to hold both the `drm_dp_read_sink_count()` r= eturn value (which can be a negative error or a positive count) and the con= nector status enum. The `goto exit` path then assigns this to `dp->phys_sta= te`. If `drm_dp_read_sink_count()` returns a negative error, the code falls= through to `exit:` with `ret` set to a negative errno, which gets stored i= n `dp->phys_state`. While the original code had the same issue (it would re= turn `connector_status_disconnected` by falling through), the new code stor= es a negative errno in `phys_state`, which is neither `connector_status_con= nected` nor `connector_status_disconnected`. This is a bug =E2=80=94 the er= ror case from `drm_dp_read_sink_count` should explicitly set `ret =3D conne= ctor_status_disconnected` before the goto. --- Generated by Claude Code Patch Reviewer