From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/radeon/radeon_connectors: use struct drm_edid instead of struct edid Date: Mon, 25 May 2026 17:37:44 +1000 Message-ID: In-Reply-To: <20260523142748.50034-2-jpeisach@ubuntu.com> References: <20260523142748.50034-1-jpeisach@ubuntu.com> <20260523142748.50034-2-jpeisach@ubuntu.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** - The field type change in `radeon_mode.h` from `struct edid *edid` to `con= st struct drm_edid *edid` is correct and matches the return type of `drm_ed= id_read_ddc()` / `drm_edid_read_switcheroo()`. - The `drm_edid_connector_update()` + `drm_edid_connector_add_modes()` repl= acement is the correct modern pattern. - The `drm_edid_is_digital()` replacement for `!!(radeon_connector->edid->i= nput & DRM_EDID_INPUT_DIGITAL)` is correct =E2=80=94 `drm_edid_is_digital()= ` takes `const struct drm_edid *`. - The `radeon_bios_get_hardcoded_edid()` change from `drm_edid_duplicate(dr= m_edid_raw(...))` to `drm_edid_dup(...)` is a clean simplification. **Issue 1 =E2=80=94 `radeon_connector_free_edid` left using `kfree` on `str= uct drm_edid`:** After patch 1, the field type changes to `const struct drm_edid *`, but `ra= deon_connector_free_edid()` still calls `kfree()` on it: ```c static void radeon_connector_free_edid(struct drm_connector *connector) { struct radeon_connector *radeon_connector =3D to_radeon_connector(connecto= r); kfree(radeon_connector->edid); radeon_connector->edid =3D NULL; } ``` After patch 1, `radeon_connector->edid` is now a `const struct drm_edid *`,= but `kfree()` is still used instead of `drm_edid_free()`. This is a **corr= ectness bug between patches** =E2=80=94 `struct drm_edid` is allocated by t= he `drm_edid_read_*` functions and contains an internal `const struct edid = *edid` pointer that also needs freeing. Simply calling `kfree()` on the out= er `struct drm_edid` will leak the inner EDID data. The tree must compile a= nd be correct at every patch boundary, so patch 1 should update `radeon_con= nector_free_edid` to call `drm_edid_free()` or the two patches should be sq= uashed. Additionally, `kfree()` on a `const` pointer will generate a compiler warni= ng/error about discarding the `const` qualifier. **Issue 2 =E2=80=94 `drm_edid_to_sad` / `drm_edid_to_speaker_allocation` ta= ke `const struct edid *`:** The audio code changes are correct. These functions take `const struct edid= *`, so using `drm_edid_raw()` to extract the raw EDID is the right approac= h: ```c sad_count =3D drm_edid_to_sad(drm_edid_raw(radeon_connector->edid), &sads); sad_count =3D drm_edid_to_speaker_allocation(drm_edid_raw(radeon_connector-= >edid), &sadb); ``` No issue here, just confirming it's correct. **Issue 3 =E2=80=94 `drm_edid_connector_update` return value ignored:** In `radeon_ddc_get_modes`, the return value of `drm_edid_connector_update()= ` is silently discarded: ```c if (radeon_connector->edid) { drm_edid_connector_update(connector, radeon_connector->edid); ret =3D drm_edid_connector_add_modes(connector); return ret; } drm_edid_connector_update(connector, NULL); ``` The old `drm_connector_update_edid_property()` also returned int and was al= so ignored, so this is pre-existing behavior and not a regression. Just not= ing it. **Nit =E2=80=94 `drm_edid_read_ddc` vs `drm_edid_read`:** For the non-DP, non-switcheroo, plain i2c adapter case, `drm_edid_read()` c= ould be used instead of `drm_edid_read_ddc()` since `drm_edid_read()` is ju= st `drm_edid_read_ddc(connector, connector->ddc)`. However, radeon sets the= ddc_bus separately from the connector's `ddc` field, so using `drm_edid_re= ad_ddc` with the explicit adapter is correct here. --- --- Generated by Claude Code Patch Reviewer