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/ast: DisplayPort edid supports 256 bytes Date: Sat, 14 Mar 2026 07:11:37 +1000 Message-ID: In-Reply-To: <20260313-upstream_ast_dp_edid-v1-1-2a75b7c091b2@aspeedtech.com> References: <20260313-upstream_ast_dp_edid-v1-1-2a75b7c091b2@aspeedtech.com> <20260313-upstream_ast_dp_edid-v1-1-2a75b7c091b2@aspeedtech.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 **The guard change is correct in isolation:** ```c - if (block > 0) + if (block > 1) return -EIO; /* extension headers not supported */ ``` Changing the limit from 0 to 1 makes sense given the hardware can address u= p to offset 63 (i.e., 256 bytes / 4 =3D 64 dwords). The offset bounds check= at line 110 (`if (offset >=3D 64)`) would correctly allow block 0 and bloc= k 1 (offsets 0=E2=80=9363). **However, there is a critical conflict with the extension-count zeroing lo= gic at lines 157-168:** ```c if (i =3D=3D 31) { /* * For 128-bytes EDID_1.3, * 1. Add the value of Bytes-126 to Bytes-127. * The Bytes-127 is Checksum. Sum of all 128bytes should * equal 0 (mod 256). * 2. Modify Bytes-126 to be 0. * The Bytes-126 indicates the Number of extensions to * follow. 0 represents noextensions. */ ediddata[3] =3D ediddata[3] + ediddata[2]; ediddata[2] =3D 0; } ``` This code runs for **every** call to `ast_astdp_read_edid_block`, including= when `block =3D=3D 0`. It forces the extension count (byte 126) to 0 and a= djusts the checksum accordingly. This means: 1. When the DRM EDID parser reads block 0, it will see extension count =3D = 0 and **never request block 1**. The `block > 1` guard change becomes dead = code. 2. For this patch to actually work, this zeroing logic needs to be conditio= ned on something (e.g., only applied when the hardware EDID truly has no va= lid extension), or removed entirely so the real extension count is reported. **The comment on the guard is now stale:** ```c return -EIO; /* extension headers not supported */ ``` If extension block 1 is now supported, this comment should be updated to so= mething like `/* only blocks 0 and 1 supported */`. **Additional observation:** The `i =3D=3D 31` check is checking the loop va= riable `i` which increments by 4, so `i =3D=3D 31` will never be true =E2= =80=94 `i` takes values 0, 4, 8, ..., 28, 32, ... This appears to be a pre-= existing bug. It seems like the intent was `i =3D=3D 124` (the 32nd dword r= ead, i.e., offset 31 when `block =3D=3D 0`, covering bytes 124=E2=80=93127 = which include the extension count at byte 126 and checksum at byte 127). Th= is needs investigation to confirm whether this code path is actually ever r= eached, and if not, whether the extension-zeroing is happening at all today. **Summary:** The patch as submitted either (a) has no effect because the ex= tension count is zeroed so block 1 is never requested, or (b) the zeroing c= ode is itself dead due to the `i =3D=3D 31` bug, in which case the patch ma= y work but the dead zeroing code should be cleaned up. Either way, addition= al work is needed before this can be merged. The author should clarify the = interaction with the extension-count modification logic and submit a v2 tha= t addresses it. --- Generated by Claude Code Patch Reviewer