[PATCH v2] drm/i915/edp: Check supported link rates DPCD read

[PATCH v2] drm/i915/edp: Check supported link rates DPCD read

[Nikita Zhandarovich]

intel_edp_set_sink_rates() reads DP_SUPPORTED_LINK_RATES into a local
stack array and then parses the array unconditionally. If the read
fails, the array contents are not valid and may result in bogus sink
link rates being used.

Use drm_dp_dpcd_read_data() and clear the sink rate array on failure,
so the existing parser falls back to the default sink rate handling.

Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.

Fixes: 68f357cb7347 ("drm/i915/dp: generate and cache sink rate array for all DP, not just eDP 1.4")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
v1 -> v2:
- Use drm_dp_dpcd_read_data() instead of drm_dp_dpcd_read().
- Avoid the goto by clearing sink_rates on read failure, as suggested by
  Jani Nikula.
- Adjust patch description.

 drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 6ef2a0043cda..5c3e816b0135 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4678,10 +4678,17 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
 
 	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
 		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
+		int ret;
 		int i;
 
-		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
-				 sink_rates, sizeof(sink_rates));
+		ret = drm_dp_dpcd_read_data(&intel_dp->aux,
+					    DP_SUPPORTED_LINK_RATES,
+					    sink_rates, sizeof(sink_rates));
+		if (ret < 0) {
+			drm_dbg_kms(display->drm,
+				    "Unable to read eDP supported link rates, using default rates\n");
+			memset(sink_rates, 0, sizeof(sink_rates));
+		}
 
 		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
 			int rate;

Re: [PATCH v2] drm/i915/edp: Check supported link rates DPCD read

[Jani Nikula]

On Fri, 29 May 2026, Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote:
> intel_edp_set_sink_rates() reads DP_SUPPORTED_LINK_RATES into a local
> stack array and then parses the array unconditionally. If the read
> fails, the array contents are not valid and may result in bogus sink
> link rates being used.
>
> Use drm_dp_dpcd_read_data() and clear the sink rate array on failure,
> so the existing parser falls back to the default sink rate handling.
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
>
> Fixes: 68f357cb7347 ("drm/i915/dp: generate and cache sink rate array for all DP, not just eDP 1.4")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
> v1 -> v2:
> - Use drm_dp_dpcd_read_data() instead of drm_dp_dpcd_read().
> - Avoid the goto by clearing sink_rates on read failure, as suggested by
>   Jani Nikula.
> - Adjust patch description.
>
>  drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6ef2a0043cda..5c3e816b0135 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4678,10 +4678,17 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>  
>  	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
>  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> +		int ret;
>  		int i;
>  
> -		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> -				 sink_rates, sizeof(sink_rates));
> +		ret = drm_dp_dpcd_read_data(&intel_dp->aux,
> +					    DP_SUPPORTED_LINK_RATES,
> +					    sink_rates, sizeof(sink_rates));
> +		if (ret < 0) {
> +			drm_dbg_kms(display->drm,
> +				    "Unable to read eDP supported link rates, using default rates\n");
> +			memset(sink_rates, 0, sizeof(sink_rates));
> +		}
>  
>  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
>  			int rate;

-- 
Jani Nikula, Intel

Re: [PATCH v2] drm/i915/edp: Check supported link rates DPCD read

[Jani Nikula]

On Fri, 29 May 2026, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 29 May 2026, Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote:
>> intel_edp_set_sink_rates() reads DP_SUPPORTED_LINK_RATES into a local
>> stack array and then parses the array unconditionally. If the read
>> fails, the array contents are not valid and may result in bogus sink
>> link rates being used.
>>
>> Use drm_dp_dpcd_read_data() and clear the sink rate array on failure,
>> so the existing parser falls back to the default sink rate handling.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 68f357cb7347 ("drm/i915/dp: generate and cache sink rate array for all DP, not just eDP 1.4")
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

And pushed to drm-intel-next, thanks for the patch.

>
>> ---
>> v1 -> v2:
>> - Use drm_dp_dpcd_read_data() instead of drm_dp_dpcd_read().
>> - Avoid the goto by clearing sink_rates on read failure, as suggested by
>>   Jani Nikula.
>> - Adjust patch description.
>>
>>  drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 6ef2a0043cda..5c3e816b0135 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4678,10 +4678,17 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>>  
>>  	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
>>  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
>> +		int ret;
>>  		int i;
>>  
>> -		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
>> -				 sink_rates, sizeof(sink_rates));
>> +		ret = drm_dp_dpcd_read_data(&intel_dp->aux,
>> +					    DP_SUPPORTED_LINK_RATES,
>> +					    sink_rates, sizeof(sink_rates));
>> +		if (ret < 0) {
>> +			drm_dbg_kms(display->drm,
>> +				    "Unable to read eDP supported link rates, using default rates\n");
>> +			memset(sink_rates, 0, sizeof(sink_rates));
>> +		}
>>  
>>  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
>>  			int rate;

-- 
Jani Nikula, Intel

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

[Claude Code Review Bot]

Overall Series Review

Subject: drm/i915/edp: Check supported link rates DPCD read
Author: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Patches: 3
Reviewed: 2026-06-04T16:24:48.707404

---

This is a single-patch series (v2) that adds error checking to the `drm_dp_dpcd_read()` call in `intel_edp_set_sink_rates()`. The patch addresses a real static-analysis finding: the original code unconditionally parsed the `sink_rates[]` array even if the DPCD read failed, potentially using uninitialized stack data.

The fix is reasonable and follows the v2 feedback from Jani Nikula to use `drm_dp_dpcd_read_data()` instead of `drm_dp_dpcd_read()` and to avoid a goto. The approach of zeroing the array on failure is safe because the existing loop already treats a zero rate as a terminator (`if (rate == 0) break`), so `memset(sink_rates, 0, sizeof(sink_rates))` will cause the loop to immediately break and set `num_sink_rates = 0`, falling through to default handling.

**Verdict: The patch is correct and suitable for merging, with one minor observation below.**

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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, also handles short reads as `-EPROTO`) is the right call. It catches both outright failures *and* short reads, which the original code did not handle at all.

The `memset` fallback is elegant — it reuses the existing `rate == 0 → break` logic at line 4828 of the current tree:

```c
rate = le16_to_cpu(sink_rates[i]) * 200 / 10;
if (rate == 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_data()`'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-by-byte retry. If both paths fail, the buffer contents are indeterminate (partially written by the failed `drm_dp_dpcd_read()` call). So the `memset` is actually **necessary** for correctness — 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 rates, using default rates"` but whether "default rates" are actually used depends on what happens after `num_sink_rates` ends up as 0. Looking at the broader function (line 4845 in the tree), `num_sink_rates` is set to `i` which will be 0 after the zeroed array causes immediate break. The caller path does handle `num_sink_rates == 0` by falling back to DPCD-based rate calculation. 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 implementation is clean. No blocking issues.**

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Overall Series Review

Subject: drm/i915/edp: Check supported link rates DPCD read
Author: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Patches: 2
Reviewed: 2026-06-04T16:37:23.909532

---

This is a single-patch series that adds error checking to the `drm_dp_dpcd_read()` call in `intel_edp_set_sink_rates()`. The motivation is reasonable — the existing code reads DPCD supported link rates into a stack-allocated array and parses it unconditionally, which could use uninitialized stack data if the read fails or is short. The fix approach is sensible: check the return value and fall back to default rate handling on failure.

However, the patch has a notable mismatch with the current upstream code and some minor issues worth addressing.

**Verdict**: Reasonable defensive fix with minor issues. Needs a small update to match the current kernel tree.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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