public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* Claude review: drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid
  2026-02-12 22:20 ` [PATCH 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid Joshua Peisach
@ 2026-02-13  6:24   ` Claude Code Review Bot
  0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-02-13  6:24 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This patch performs the core migration from `struct edid` to `const struct drm_edid`, updating the field type in `struct amdgpu_connector` and converting all associated function calls to use the new API.

> @@ -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);
>  }

The conversion from `drm_edid_duplicate(drm_edid_raw(...))` to `drm_edid_dup()` is correct and cleaner.

> -		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);

The conversion from `drm_get_edid()` to `drm_edid_read_ddc()` is correct throughout.

> @@ -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;
>  	}

The change to `drm_edid_connector_add_modes(connector)` is correct. The new API retrieves the EDID from the connector's internal state that was set by the earlier `drm_edid_connector_update()` call.

> @@ -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);

Good use of the helper function instead of raw field access.

> @@ -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);

Correctly unwraps to the raw EDID for functions that haven't been updated to accept `struct drm_edid`.

Overall, this patch is well-executed with no issues found.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH RESEND 0/2] drm/amdgpu/amdgpu_connectors: Use struct drm_edid
@ 2026-03-03 21:18 Joshua Peisach
  2026-03-03 21:18 ` [PATCH RESEND 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid Joshua Peisach
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Joshua Peisach @ 2026-03-03 21:18 UTC (permalink / raw)
  To: amd-gfx, dri-devel; +Cc: Alex Deucher, Christian König, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH RESEND 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid
  2026-03-03 21:18 [PATCH RESEND 0/2] drm/amdgpu/amdgpu_connectors: Use struct drm_edid Joshua Peisach
@ 2026-03-03 21:18 ` Joshua Peisach
  2026-03-03 22:35   ` Claude review: " Claude Code Review Bot
  2026-03-03 21:18 ` [PATCH RESEND 2/2] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid Joshua Peisach
  2026-03-03 22:35 ` Claude review: drm/amdgpu/amdgpu_connectors: Use struct drm_edid Claude Code Review Bot
  2 siblings, 1 reply; 7+ messages in thread
From: Joshua Peisach @ 2026-03-03 21:18 UTC (permalink / raw)
  To: amd-gfx, dri-devel; +Cc: Alex Deucher, Christian König, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH RESEND 2/2] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid
  2026-03-03 21:18 [PATCH RESEND 0/2] drm/amdgpu/amdgpu_connectors: Use struct drm_edid Joshua Peisach
  2026-03-03 21:18 ` [PATCH RESEND 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid Joshua Peisach
@ 2026-03-03 21:18 ` Joshua Peisach
  2026-03-03 22:35   ` Claude review: " Claude Code Review Bot
  2026-03-03 22:35 ` Claude review: drm/amdgpu/amdgpu_connectors: Use struct drm_edid Claude Code Review Bot
  2 siblings, 1 reply; 7+ messages in thread
From: Joshua Peisach @ 2026-03-03 21:18 UTC (permalink / raw)
  To: amd-gfx, dri-devel; +Cc: Alex Deucher, Christian König, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Claude review: drm/amdgpu/amdgpu_connectors: Use struct drm_edid
  2026-03-03 21:18 [PATCH RESEND 0/2] drm/amdgpu/amdgpu_connectors: Use struct drm_edid Joshua Peisach
  2026-03-03 21:18 ` [PATCH RESEND 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid Joshua Peisach
  2026-03-03 21:18 ` [PATCH RESEND 2/2] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid Joshua Peisach
@ 2026-03-03 22:35 ` Claude Code Review Bot
  2 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 22:35 UTC (permalink / raw)
  To: dri-devel-reviews

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Claude review: drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid
  2026-03-03 21:18 ` [PATCH RESEND 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid Joshua Peisach
@ 2026-03-03 22:35   ` Claude Code Review Bot
  0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 22:35 UTC (permalink / raw)
  To: dri-devel-reviews

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Claude review: drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid
  2026-03-03 21:18 ` [PATCH RESEND 2/2] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid Joshua Peisach
@ 2026-03-03 22:35   ` Claude Code Review Bot
  0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 22:35 UTC (permalink / raw)
  To: dri-devel-reviews

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-03-03 22:35 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-03 21:18 [PATCH RESEND 0/2] drm/amdgpu/amdgpu_connectors: Use struct drm_edid Joshua Peisach
2026-03-03 21:18 ` [PATCH RESEND 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid Joshua Peisach
2026-03-03 22:35   ` Claude review: " Claude Code Review Bot
2026-03-03 21:18 ` [PATCH RESEND 2/2] drm/amdgpu/amdgpu_connectors: remove amdgpu_connector_free_edid Joshua Peisach
2026-03-03 22:35   ` Claude review: " Claude Code Review Bot
2026-03-03 22:35 ` Claude review: drm/amdgpu/amdgpu_connectors: Use struct drm_edid Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-02-12 22:20 [PATCH 0/2] " Joshua Peisach
2026-02-12 22:20 ` [PATCH 1/2] drm/amdgpu/amdgpu_connectors: use struct drm_edid instead of struct edid Joshua Peisach
2026-02-13  6:24   ` Claude review: " Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox