Claude review: drm/amdgpu/amdgpu_connectors: Use struct drm_edid

Claude review: drm/amdgpu/amdgpu_connectors: Use struct drm_edid

[Claude Code Review Bot]

Overall Series Review

Subject: drm/amdgpu/amdgpu_connectors: Use struct drm_edid
Author: Joshua Peisach <jpeisach@ubuntu.com>
Patches: 3
Reviewed: 2026-02-13T16:18:05.671505

---

This two-patch series converts amdgpu connector code from the deprecated `struct edid` type to the newer `struct drm_edid` type, updating all related function calls to use the modern DRM EDID APIs. The conversion is mostly straightforward and follows the established pattern used elsewhere in the DRM subsystem.

The series changes the `amdgpu_connector->edid` field type from `struct edid *` to `const struct drm_edid *`, replaces `drm_get_edid()` calls with `drm_edid_read_ddc()`, replaces `drm_connector_update_edid_property()` with `drm_edid_connector_update()`, and updates audio SAD/speaker allocation code to use `drm_edid_raw()` to access the underlying raw EDID structure. The second patch removes the now-unnecessary `amdgpu_connector_free_edid()` wrapper and calls `drm_edid_free()` directly.

The main concern with this series is a potential use-after-free bug introduced in patch 2. When removing the `amdgpu_connector_free_edid()` wrapper, the patches lose the critical `amdgpu_connector->edid = NULL` assignment that was part of the wrapper. While `drm_edid_free()` safely handles NULL pointers, it does not set the passed pointer to NULL. In several code paths, the EDID is freed but the pointer is left dangling, and the code later checks `if (!amdgpu_connector->edid)` or attempts to use the pointer. This creates a classic use-after-free vulnerability.

---
Generated by Claude Code Patch Reviewer

[PATCH RESEND 0/2] drm/amdgpu/amdgpu_connectors: Use struct drm_edid

[Joshua Peisach]

These patches replace uses of struct edid with struct drm_edid, and
updates function calls appropriately.
                                                                                                                                                                                                      
Additionally, amdgpu_connector_free_edid is no longer needed, and can be
removed.

Joshua Peisach (2):
  drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct
    edid
  drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    | 54 ++++++++-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |  2 +-
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c        |  4 +-
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c         |  4 +-
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c         |  4 +-
 5 files changed, 30 insertions(+), 38 deletions(-)

-- 
2.51.0

[PATCH RESEND 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid

[Joshua Peisach]

Some amdgpu code is still using deprecated edid functions. Switch to
the newer functions and update the amdgpu_connector struct's edid type
to the drm_edid type.

At the same time, use the raw EDID when we need to for speaker
allocations and for determining if the input is digital.

Signed-off-by: Joshua Peisach <jpeisach@ubuntu.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    | 32 +++++++++----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |  2 +-
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c        |  4 +--
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c         |  4 +--
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c         |  4 +--
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index d1bf2e150..6336cadad 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -246,10 +246,10 @@ amdgpu_connector_find_encoder(struct drm_connector *connector,
 	return NULL;
 }
 
-static struct edid *
+static const struct drm_edid *
 amdgpu_connector_get_hardcoded_edid(struct amdgpu_device *adev)
 {
-	return drm_edid_duplicate(drm_edid_raw(adev->mode_info.bios_hardcoded_edid));
+	return drm_edid_dup(adev->mode_info.bios_hardcoded_edid);
 }
 
 static void amdgpu_connector_get_edid(struct drm_connector *connector)
@@ -268,8 +268,8 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
 	if ((amdgpu_connector_encoder_get_dp_bridge_encoder_id(connector) !=
 	     ENCODER_OBJECT_ID_NONE) &&
 	    amdgpu_connector->ddc_bus->has_aux) {
-		amdgpu_connector->edid = drm_get_edid(connector,
-						      &amdgpu_connector->ddc_bus->aux.ddc);
+		amdgpu_connector->edid = drm_edid_read_ddc(connector,
+							  &amdgpu_connector->ddc_bus->aux.ddc);
 	} else if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
 		   (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
 		struct amdgpu_connector_atom_dig *dig = amdgpu_connector->con_priv;
@@ -277,14 +277,14 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
 		if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
 		     dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
 		    amdgpu_connector->ddc_bus->has_aux)
-			amdgpu_connector->edid = drm_get_edid(connector,
-							      &amdgpu_connector->ddc_bus->aux.ddc);
+			amdgpu_connector->edid = drm_edid_read_ddc(connector,
+								  &amdgpu_connector->ddc_bus->aux.ddc);
 		else if (amdgpu_connector->ddc_bus)
-			amdgpu_connector->edid = drm_get_edid(connector,
-							      &amdgpu_connector->ddc_bus->adapter);
+			amdgpu_connector->edid = drm_edid_read_ddc(connector,
+								  &amdgpu_connector->ddc_bus->adapter);
 	} else if (amdgpu_connector->ddc_bus) {
-		amdgpu_connector->edid = drm_get_edid(connector,
-						      &amdgpu_connector->ddc_bus->adapter);
+		amdgpu_connector->edid = drm_edid_read_ddc(connector,
+							  &amdgpu_connector->ddc_bus->adapter);
 	}
 
 	if (!amdgpu_connector->edid) {
@@ -292,7 +292,7 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
 		if (((connector->connector_type == DRM_MODE_CONNECTOR_LVDS) ||
 		     (connector->connector_type == DRM_MODE_CONNECTOR_eDP))) {
 			amdgpu_connector->edid = amdgpu_connector_get_hardcoded_edid(adev);
-			drm_connector_update_edid_property(connector, amdgpu_connector->edid);
+			drm_edid_connector_update(connector, amdgpu_connector->edid);
 		}
 	}
 }
@@ -311,11 +311,11 @@ static int amdgpu_connector_ddc_get_modes(struct drm_connector *connector)
 	int ret;
 
 	if (amdgpu_connector->edid) {
-		drm_connector_update_edid_property(connector, amdgpu_connector->edid);
-		ret = drm_add_edid_modes(connector, amdgpu_connector->edid);
+		drm_edid_connector_update(connector, amdgpu_connector->edid);
+		ret = drm_edid_connector_add_modes(connector);
 		return ret;
 	}
-	drm_connector_update_edid_property(connector, NULL);
+	drm_edid_connector_update(connector, NULL);
 	return 0;
 }
 
@@ -883,7 +883,7 @@ amdgpu_connector_vga_detect(struct drm_connector *connector, bool force)
 			ret = connector_status_connected;
 		} else {
 			amdgpu_connector->use_digital =
-				!!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
+				drm_edid_is_digital(amdgpu_connector->edid);
 
 			/* some oems have boards with separate digital and analog connectors
 			 * with a shared ddc line (often vga + hdmi)
@@ -1063,7 +1063,7 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force)
 			broken_edid = true; /* defer use_digital to later */
 		} else {
 			amdgpu_connector->use_digital =
-				!!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
+				drm_edid_is_digital(amdgpu_connector->edid);
 
 			/* some oems have boards with separate digital and analog connectors
 			 * with a shared ddc line (often vga + hdmi)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index dc8d2f52c..c4e025581 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -624,7 +624,7 @@ struct amdgpu_connector {
 	bool use_digital;
 	/* we need to mind the EDID between detect
 	   and get modes due to analog/digital/tvencoder */
-	struct edid *edid;
+	const struct drm_edid *edid;
 	void *con_priv;
 	bool dac_load_detect;
 	bool detected_by_load; /* if the connection status was determined by load */
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index a7ffe10ee..4aaee1bf9 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1298,7 +1298,7 @@ static void dce_v10_0_audio_write_speaker_allocation(struct drm_encoder *encoder
 		return;
 	}
 
-	sad_count = drm_edid_to_speaker_allocation(amdgpu_connector->edid, &sadb);
+	sad_count = drm_edid_to_speaker_allocation(drm_edid_raw(amdgpu_connector->edid), &sadb);
 	if (sad_count < 0) {
 		DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n", sad_count);
 		sad_count = 0;
@@ -1368,7 +1368,7 @@ static void dce_v10_0_audio_write_sad_regs(struct drm_encoder *encoder)
 		return;
 	}
 
-	sad_count = drm_edid_to_sad(amdgpu_connector->edid, &sads);
+	sad_count = drm_edid_to_sad(drm_edid_raw(amdgpu_connector->edid), &sads);
 	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
 	if (sad_count <= 0)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index a72e20db5..7cb207c7f 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1265,7 +1265,7 @@ static void dce_v6_0_audio_write_speaker_allocation(struct drm_encoder *encoder)
 		return;
 	}
 
-	sad_count = drm_edid_to_speaker_allocation(amdgpu_connector->edid, &sadb);
+	sad_count = drm_edid_to_speaker_allocation(drm_edid_raw(amdgpu_connector->edid), &sadb);
 	if (sad_count < 0) {
 		DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n", sad_count);
 		sad_count = 0;
@@ -1346,7 +1346,7 @@ static void dce_v6_0_audio_write_sad_regs(struct drm_encoder *encoder)
 		return;
 	}
 
-	sad_count = drm_edid_to_sad(amdgpu_connector->edid, &sads);
+	sad_count = drm_edid_to_sad(drm_edid_raw(amdgpu_connector->edid), &sads);
 	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
 	if (sad_count <= 0)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 4221c7b7c..0eff41e94 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1271,7 +1271,7 @@ static void dce_v8_0_audio_write_speaker_allocation(struct drm_encoder *encoder)
 		return;
 	}
 
-	sad_count = drm_edid_to_speaker_allocation(amdgpu_connector->edid, &sadb);
+	sad_count = drm_edid_to_speaker_allocation(drm_edid_raw(amdgpu_connector->edid), &sadb);
 	if (sad_count < 0) {
 		DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n", sad_count);
 		sad_count = 0;
@@ -1339,7 +1339,7 @@ static void dce_v8_0_audio_write_sad_regs(struct drm_encoder *encoder)
 		return;
 	}
 
-	sad_count = drm_edid_to_sad(amdgpu_connector->edid, &sads);
+	sad_count = drm_edid_to_sad(drm_edid_raw(amdgpu_connector->edid), &sads);
 	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
 	if (sad_count <= 0)
-- 
2.51.0

[PATCH RESEND 2/2] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

[Joshua Peisach]

Now that we are using struct drm_edid, we can just call drm_edid_free
directly. Remove the function and update calls to drm_edid_free.

Signed-off-by: Joshua Peisach <jpeisach@ubuntu.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    | 22 ++++++-------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 6336cadad..aabe9d58c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -297,14 +297,6 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
 	}
 }
 
-static void amdgpu_connector_free_edid(struct drm_connector *connector)
-{
-	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
-
-	kfree(amdgpu_connector->edid);
-	amdgpu_connector->edid = NULL;
-}
-
 static int amdgpu_connector_ddc_get_modes(struct drm_connector *connector)
 {
 	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
@@ -754,7 +746,7 @@ static void amdgpu_connector_destroy(struct drm_connector *connector)
 {
 	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
 
-	amdgpu_connector_free_edid(connector);
+	drm_edid_free(amdgpu_connector->edid);
 	kfree(amdgpu_connector->con_priv);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
@@ -873,7 +865,7 @@ amdgpu_connector_vga_detect(struct drm_connector *connector, bool force)
 		dret = amdgpu_display_ddc_probe(amdgpu_connector, false);
 	if (dret) {
 		amdgpu_connector->detected_by_load = false;
-		amdgpu_connector_free_edid(connector);
+		drm_edid_free(amdgpu_connector->edid);
 		amdgpu_connector_get_edid(connector);
 
 		if (!amdgpu_connector->edid) {
@@ -889,7 +881,7 @@ amdgpu_connector_vga_detect(struct drm_connector *connector, bool force)
 			 * with a shared ddc line (often vga + hdmi)
 			 */
 			if (amdgpu_connector->use_digital && amdgpu_connector->shared_ddc) {
-				amdgpu_connector_free_edid(connector);
+				drm_edid_free(amdgpu_connector->edid);
 				ret = connector_status_disconnected;
 			} else {
 				ret = connector_status_connected;
@@ -984,7 +976,7 @@ static void amdgpu_connector_shared_ddc(enum drm_connector_status *status,
 					/* hpd is our only option in this case */
 					if (!amdgpu_display_hpd_sense(adev,
 								      amdgpu_connector->hpd.hpd)) {
-						amdgpu_connector_free_edid(connector);
+						drm_edid_free(amdgpu_connector->edid);
 						*status = connector_status_disconnected;
 					}
 				}
@@ -1053,7 +1045,7 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force)
 	}
 	if (dret) {
 		amdgpu_connector->detected_by_load = false;
-		amdgpu_connector_free_edid(connector);
+		drm_edid_free(amdgpu_connector->edid);
 		amdgpu_connector_get_edid(connector);
 
 		if (!amdgpu_connector->edid) {
@@ -1069,7 +1061,7 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force)
 			 * with a shared ddc line (often vga + hdmi)
 			 */
 			if ((!amdgpu_connector->use_digital) && amdgpu_connector->shared_ddc) {
-				amdgpu_connector_free_edid(connector);
+				drm_edid_free(amdgpu_connector->edid);
 				ret = connector_status_disconnected;
 			} else {
 				ret = connector_status_connected;
@@ -1417,7 +1409,7 @@ amdgpu_connector_dp_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
-	amdgpu_connector_free_edid(connector);
+	drm_edid_free(amdgpu_connector->edid);
 
 	if ((connector->connector_type == DRM_MODE_CONNECTOR_eDP) ||
 	    (connector->connector_type == DRM_MODE_CONNECTOR_LVDS)) {
-- 
2.51.0

Claude review: drm/amdgpu/amdgpu_connectors: Use struct drm_edid

[Claude Code Review Bot]

Overall Series Review

Subject: drm/amdgpu/amdgpu_connectors: Use struct drm_edid
Author: Joshua Peisach <jpeisach@ubuntu.com>
Patches: 3
Reviewed: 2026-03-04T08:35:53.790529

---

This 2-patch series converts the amdgpu connector code from the deprecated `struct edid` / `drm_get_edid()` API to the modern opaque `struct drm_edid` / `drm_edid_read_ddc()` API. The intent is correct and the direction is right — this is a well-understood modernization pattern across DRM drivers.

**However, patch 2 introduces use-after-free bugs at every call site.** The old `amdgpu_connector_free_edid()` both freed the EDID and set `amdgpu_connector->edid = NULL`. The replacement `drm_edid_free()` only frees the memory — it cannot NULL out the caller's pointer. The patch fails to add `amdgpu_connector->edid = NULL` after any of the `drm_edid_free()` calls, leaving dangling pointers that are subsequently dereferenced.

**This series should not be applied as-is.**

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid

[Claude Code Review Bot]

Patch Review

This patch is largely correct. The API migrations are:

- `struct edid *edid` → `const struct drm_edid *edid` in `amdgpu_connector` — **correct**
- `drm_get_edid()` → `drm_edid_read_ddc()` — **correct**
- `drm_edid_duplicate(drm_edid_raw(...))` → `drm_edid_dup()` — **correct**, cleaner
- `drm_connector_update_edid_property()` → `drm_edid_connector_update()` — **correct**
- `drm_add_edid_modes()` → `drm_edid_connector_add_modes()` — **correct**
- `!!(edid->input & DRM_EDID_INPUT_DIGITAL)` → `drm_edid_is_digital()` — **correct**
- `drm_edid_to_speaker_allocation(edid, ...)` → `drm_edid_to_speaker_allocation(drm_edid_raw(edid), ...)` — **correct**, these functions still require `struct edid *`
- `drm_edid_to_sad(edid, ...)` → `drm_edid_to_sad(drm_edid_raw(edid), ...)` — **correct**

One minor observation: the `drm_edid_to_sad()` and `drm_edid_to_speaker_allocation()` calls in dce_v6_0.c, dce_v8_0.c, and dce_v10_0.c still require the raw `struct edid *`, so wrapping with `drm_edid_raw()` is the right approach. Ideally these would eventually get their own `struct drm_edid` versions, but that's out of scope for this series.

**Verdict: Patch 1 looks correct.**

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid

[Claude Code Review Bot]

Patch Review

**This patch has a critical bug.** The removed function was:

```c
static void amdgpu_connector_free_edid(struct drm_connector *connector)
{
	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);

	kfree(amdgpu_connector->edid);
	amdgpu_connector->edid = NULL;   // <-- THIS IS IMPORTANT
}
```

`drm_edid_free()` only frees the memory — it **does not** and **cannot** set the caller's pointer to NULL. Every replacement site leaves `amdgpu_connector->edid` as a dangling pointer.

**Specific use-after-free sites:**

1. **`amdgpu_connector_vga_detect()` line 868:**
   ```c
   drm_edid_free(amdgpu_connector->edid);
   amdgpu_connector_get_edid(connector);
   ```
   `amdgpu_connector_get_edid()` starts with `if (amdgpu_connector->edid) return;` — the dangling non-NULL pointer causes it to return immediately without reading a new EDID. The subsequent `if (!amdgpu_connector->edid)` check at line 871 then sees a non-NULL freed pointer and proceeds to call `drm_edid_is_digital()` on freed memory.

2. **`amdgpu_connector_vga_detect()` line 884** (shared DDC disconnect path): `drm_edid_free()` without NULL leaves a dangling pointer that persists in the struct for future accesses.

3. **`amdgpu_connector_shared_ddc()` line 979:** Same dangling pointer issue.

4. **`amdgpu_connector_dvi_detect()` line 1048:** Same bug as site 1 — `amdgpu_connector_get_edid()` will see the dangling pointer and skip the EDID read.

5. **`amdgpu_connector_dvi_detect()` line 1064** (shared DDC disconnect): Same dangling pointer.

6. **`amdgpu_connector_dp_detect()` line 1412:** Dangling pointer left after free.

7. **`amdgpu_connector_destroy()` line 749:** This is the **only** safe call site — the connector struct is `kfree()`d at line 753, so the dangling pointer is irrelevant.

**Fix:** Every `drm_edid_free(amdgpu_connector->edid)` call (except in `_destroy`) must be followed by `amdgpu_connector->edid = NULL`. Alternatively, keep a helper function:

```c
static void amdgpu_connector_free_edid(struct drm_connector *connector)
{
	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);

	drm_edid_free(amdgpu_connector->edid);
	amdgpu_connector->edid = NULL;
}
```

This would preserve the original behavior while still removing the old `kfree()` call. The commit message claim that "we can just call drm_edid_free directly" is incorrect because the semantics differ — the wrapper provided NULL-clearing that `drm_edid_free()` does not.

**Verdict: Patch 2 must be reworked. It introduces use-after-free bugs at 6 call sites.**

---
Generated by Claude Code Patch Reviewer