From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/i915/edp: Check supported link rates DPCD read
Date: Thu, 04 Jun 2026 16:37:24 +1000
Message-ID: <review-patch1-20260529120921.1633346-1-n.zhandarovich@fintech.ru>
In-Reply-To: <20260529120921.1633346-1-n.zhandarovich@fintech.ru>
References: <20260529120921.1633346-1-n.zhandarovich@fintech.ru> <20260529120921.1633346-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 of the core fix =E2=80=94 Good**

The approach of checking the `drm_dp_dpcd_read()` return value is correct. =
Looking at the `drm_dp_dpcd_read()` implementation at `drm_dp_helper.c:748`=
, it returns the number of bytes transferred on success or a negative error=
 code on failure. The docstring also confirms that short reads return `-EPR=
OTO`:

> Returns the number of bytes transferred on success, or a negative error c=
ode on failure. ... If not all bytes were transferred, this function return=
s -EPROTO.

Given that `drm_dp_dpcd_read()` already returns `-EPROTO` for partial reads=
, the comparison `ret !=3D sizeof(sink_rates)` correctly handles both error=
s (negative) and short reads (would be -EPROTO anyway, but belt-and-suspend=
ers is fine here).

**Issue 1 =E2=80=94 `ssize_t` type mismatch with current `drm_dp_dpcd_read(=
)` return type**

The patch declares:
```c
ssize_t ret;
```

But looking at the current `drm_dp_dpcd_read()` definition in `drm_dp_helpe=
r.c:748-751`:
```c
ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
                          void *buffer, size_t size)
{
    int ret;
```

The function signature returns `ssize_t`, so this is technically correct. H=
owever, I note the function internally uses `int ret`. This is fine =E2=80=
=94 `ssize_t` for the caller's variable matches the declared return type.

**Issue 2 =E2=80=94 `goto` label placement creates a subtle semantic change=
**

The `use_default_rates:` label is placed at:
```c
use_default_rates:
	if (intel_dp->num_sink_rates)
		intel_dp->use_rate_select =3D true;
	else
		intel_dp_set_sink_rates(intel_dp);
```

When the `goto` is taken, `intel_dp->num_sink_rates` will be 0 (set at the =
top of the function on line 4680 of the original), so the `else` branch run=
s `intel_dp_set_sink_rates()`. This is correct behavior =E2=80=94 on DPCD r=
ead failure, we fall back to the default rate path. The label name and debu=
g message are clear about the intent.

**Issue 3 =E2=80=94 Patch may not apply to current drm-next**

The patch is based on context showing the code at line 4678, but in the cur=
rent tree this function starts at line 4806. The tree also contains additio=
nal code (the `QUIRK_EDP_LIMIT_RATE_HBR2` block at lines 4834-4841) that is=
n't in the patch context. This means the patch was likely written against a=
n older tree. It should apply with fuzz or can be trivially rebased.

**Issue 4 =E2=80=94 Missing `intel_edp_set_data_override_rates` on error pa=
th**

Looking at the full function in the current tree at `intel_dp.c:4856`:
```c
	intel_edp_set_data_override_rates(intel_dp);
```

This call happens at the end of the function. Since the `goto use_default_r=
ates` jumps past the rate parsing loop but still hits this final call (it's=
 after the label), the data override rates are still applied on the error p=
ath. This is correct =E2=80=94 the `goto` doesn't skip that call.

**Issue 5 =E2=80=94 Uninitialized stack data concern is somewhat overstated=
**

The commit message says the array "may result in bogus sink link rates bein=
g used." While technically true in theory, `drm_dp_dpcd_read()` already ret=
urns `-EPROTO` on partial reads per its documentation, so a short read (ret=
urning less than requested but positive) shouldn't actually happen with thi=
s API. The realistic failure mode is a negative error code. That said, the =
fix is still good defensive programming =E2=80=94 the current code silently=
 ignores read errors and would parse whatever garbage happened to be on the=
 stack.

**Nit =E2=80=94 Commit message title grammar**

The subject line `"drm/i915/edp: Check supported link rates DPCD read"` rea=
ds awkwardly. Something like `"drm/i915/edp: Check return value of supporte=
d link rates DPCD read"` would be clearer.

**Summary**: The patch is a reasonable defensive fix. The code change is co=
rrect and the fallback behavior is appropriate. The main feedback would be =
to rebase onto current drm-next and possibly clarify the commit subject lin=
e.

---
Generated by Claude Code Patch Reviewer