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/i915/edp: Check supported link rates DPCD read Date: Thu, 04 Jun 2026 16:24:48 +1000 Message-ID: In-Reply-To: <20260529145759.1640646-1-n.zhandarovich@fintech.ru> References: <20260529145759.1640646-1-n.zhandarovich@fintech.ru> <20260529145759.1640646-1-n.zhandarovich@fintech.ru> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good** The switch from `drm_dp_dpcd_read()` (returns bytes read or negative error)= to `drm_dp_dpcd_read_data()` (returns 0 on success, negative on error, als= o handles short reads as `-EPROTO`) is the right call. It catches both outr= ight failures *and* short reads, which the original code did not handle at = all. The `memset` fallback is elegant =E2=80=94 it reuses the existing `rate =3D= =3D 0 =E2=86=92 break` logic at line 4828 of the current tree: ```c rate =3D le16_to_cpu(sink_rates[i]) * 200 / 10; if (rate =3D=3D 0) break; ``` This means `num_sink_rates` will be set to 0, which is the correct fallback. **Minor observation:** The `memset` on failure is technically redundant given `drm_dp_dpcd_read_da= ta()`'s implementation. Looking at the function (drm_dp_helper.h:588-617), = on the primary `drm_dp_dpcd_read()` failure path it falls through to byte-b= y-byte retry. If both paths fail, the buffer contents are indeterminate (pa= rtially written by the failed `drm_dp_dpcd_read()` call). So the `memset` i= s actually **necessary** for correctness =E2=80=94 the buffer could contain= partial/garbage data from a failed read. This is fine as-is. **One nit:** The debug message says `"Unable to read eDP supported link rat= es, using default rates"` but whether "default rates" are actually used dep= ends on what happens after `num_sink_rates` ends up as 0. Looking at the br= oader function (line 4845 in the tree), `num_sink_rates` is set to `i` whic= h will be 0 after the zeroed array causes immediate break. The caller path = does handle `num_sink_rates =3D=3D 0` by falling back to DPCD-based rate ca= lculation. So the message is accurate enough, though "falling back to DPCD-= based rates" would be more precise. This is not worth a respin. **Summary: Patch is correct, addresses a real (if unlikely) bug, and the im= plementation is clean. No blocking issues.** --- Generated by Claude Code Patch Reviewer