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: lt9611uxc: support displays with up to 4 EDID blocks Date: Mon, 18 May 2026 16:00:28 +1000 Message-ID: In-Reply-To: <20260517-lt9611usc_edid34_misc_next-v1-1-5e2fd8c6399b@oss.qualcomm.com> References: <20260517-lt9611usc_edid34_misc_next-v1-1-5e2fd8c6399b@oss.qualcomm.com> <20260517-lt9611usc_edid34_misc_next-v1-1-5e2fd8c6399b@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 **BUG (critical): `memcmp` compares wrong addresses** ```c while (!ret && 0 =3D=3D memcmp(&edid_header, &buf, 8) && retry_cnt-- > 0) { ``` `buf` is a `u8 *` (pointer). Taking `&buf` gives the address of the pointer= variable on the stack, **not** the address of the buffer contents. This me= ans `memcmp` compares 8 bytes of stack memory (the pointer value + whatever= follows) against the EDID header, which will almost never match. The retry= loop will effectively never execute. Similarly, `&edid_header` works by ac= cident here since `edid_header` is an array (so `&edid_header =3D=3D edid_h= eader` address-wise), but for consistency both should just be `edid_header`= and `buf`: ```c while (!ret && memcmp(edid_header, buf, 8) =3D=3D 0 && retry_cnt-- > 0) { ``` Also, the Yoda-style `0 =3D=3D memcmp(...)` is not idiomatic kernel style. **BUG: Missing `edid_data` cleanup in `lt9611uxc_remove`** The patch adds `edid_data` caching but never frees it in `lt9611uxc_remove(= )`. If the device is removed while a display is connected (and EDID is cach= ed), the `drm_edid` allocation leaks. `lt9611uxc_remove` needs: ```c drm_edid_free(lt9611uxc->edid_data); ``` **Issue: Missing `regmap_write(0xb00b, 0x10)` for blocks 2=E2=80=933** The original code writes `regmap_write(lt9611uxc->regmap, 0xb00b, 0x10)` be= fore reading an EDID block. The new `lt9611uxc_read_edid_block()` helper om= its this write entirely. For blocks 0 and 1 this may work because the old c= ode path previously set it, but the intent of that register write is unclea= r and dropping it silently could cause issues. If register `0xb00b` is need= ed to set up EDID reading, it should be included in `lt9611uxc_read_edid_bl= ock`. If it's not needed, its removal from the block 0/1 path should be exp= licitly noted in the commit message. **Issue: Concurrency on `edid_data` cache** `edid_data` is read/written from `lt9611uxc_bridge_edid_read()` and `lt9611= uxc_hpd_work()` without any locking. The HPD work function clears `edid_dat= a` while `edid_read` could be simultaneously accessing it. The `ocm_lock` m= utex is held for `hdmi_connected` access but the new `edid_data` manipulati= on in `lt9611uxc_hpd_work` happens *outside* that lock: ```c mutex_unlock(<9611uxc->ocm_lock); if (!connected) { lt9611uxc->edid_read =3D false; drm_edid_free(lt9611uxc->edid_data); lt9611uxc->edid_data =3D NULL; } ``` This creates a TOCTOU race: `lt9611uxc_bridge_edid_read` could see `edid_da= ta !=3D NULL`, then HPD work frees it, then `edid_read` calls `drm_edid_dup= ` on freed memory. The cache operations need to be protected (either extend= the existing `ocm_lock` scope or add separate locking). **Issue: `edid_read` flag set outside lock** ```c lt9611uxc->edid_read =3D false; ``` This is set in `lt9611uxc_hpd_work` without holding `ocm_lock`, but it's se= t by the IRQ handler under `ocm_lock` (line 150 in the original). Inconsist= ent locking on a shared flag. **Nit: Coding style** Missing space before modulo operator: ```c regmap_write(lt9611uxc->regmap, 0xb00a, (block%2) * EDID_BLOCK_SIZE); ``` Should be `(block % 2)`. **Nit: Unnecessary double-space** ```c u8 *buf, size_t len) ``` Two spaces before `size_t`. **Nit: Return value masking** ```c ret =3D regmap_noinc_read(lt9611uxc->regmap, 0xb0b0, buf, len); if (ret) { dev_err(lt9611uxc->dev, "edid block %d read failed: %d\n", block, ret); lt9611uxc_unlock(lt9611uxc); return -EINVAL; } ``` This replaces `ret` (which is the actual regmap error code) with `-EINVAL`.= It should just `return ret` to propagate the real error. Alternatively, us= e a `goto` with a single unlock path to avoid the duplicated `lt9611uxc_unl= ock` call. **Design: Retry logic is fragile** The retry checks whether block 2 data still looks like block 0 (EDID header= ) as a staleness indicator. This is clever but fragile =E2=80=94 if block 2= legitimately starts with `00 FF FF FF FF FF FF 00` (very unlikely for a CE= A extension but not formally impossible), the retry loop would spin all 10 = iterations pointlessly. A comment explaining the hardware behavior would he= lp. Also, the 10 retries =C3=97 100ms =3D 1 second total, plus the initial = 100ms, gives 1.1 seconds maximum. It would be good to document whether this= is sufficient for the hardware. **Summary of required changes:** 1. Fix `&buf` =E2=86=92 `buf` in the `memcmp` call (critical bug) 2. Add `drm_edid_free(lt9611uxc->edid_data)` in `lt9611uxc_remove` (leak) 3. Add locking around `edid_data` accesses (race condition) 4. Clarify or restore the `0xb00b` register write 5. Minor style fixes --- Generated by Claude Code Patch Reviewer