Claude review: drm/i915/edp: Check supported link rates DPCD read

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Correctness of the core fix — 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 `-EPROTO`:

> Returns the number of bytes transferred on success, or a negative error code on failure. ... If not all bytes were transferred, this function returns -EPROTO.

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

**Issue 1 — `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_helper.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. However, I note the function internally uses `int ret`. This is fine — `ssize_t` for the caller's variable matches the declared return type.

**Issue 2 — `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 = 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 runs `intel_dp_set_sink_rates()`. This is correct behavior — on DPCD read failure, we fall back to the default rate path. The label name and debug message are clear about the intent.

**Issue 3 — Patch may not apply to current drm-next**

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

**Issue 4 — Missing `intel_edp_set_data_override_rates` on error path**

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_rates` 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 path. This is correct — the `goto` doesn't skip that call.

**Issue 5 — Uninitialized stack data concern is somewhat overstated**

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

**Nit — Commit message title grammar**

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

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

---
Generated by Claude Code Patch Reviewer