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/i915/display/intel_lvds: Drop redundant manual cleanup on init failure Date: Thu, 04 Jun 2026 11:52:28 +1000 Message-ID: In-Reply-To: <20260603-fix_i915-v1-2-7479ff64e705@bootlin.com> References: <20260603-fix_i915-v1-0-7479ff64e705@bootlin.com> <20260603-fix_i915-v1-2-7479ff64e705@bootlin.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 **Verdict: NACK =E2=80=94 this patch introduces a resource leak.** The commit message claims the manual cleanup in the `failed:` label is "red= undant" because the DRM core will invoke `.destroy` callbacks. This is **in= correct** for `intel_lvds_init()`. `intel_lvds_init()` returns `void`. It is called as a fire-and-forget initi= alization from `intel_display.c` (lines 7872, 7949, 7955, 7997). The caller= does **not** check a return value and does **not** perform any cleanup. If= `intel_lvds_init()` fails after `drm_connector_init_with_ddc()` and `drm_e= ncoder_init()` have succeeded but before the function completes successfull= y, the half-initialized connector and encoder objects remain registered wit= h the DRM core. The key question is: will the DRM core's teardown path call `.destroy` on t= hese objects during device shutdown? Actually, yes =E2=80=94 `drm_connector= _init_with_ddc()` adds the connector to `mode_config.connector_list`, so `d= rm_mode_config_cleanup()` will call `.destroy` on it, which is `intel_conne= ctor_destroy`. Similarly, `drm_encoder_init()` adds the encoder to the enco= der list, so `drm_mode_config_cleanup()` will call `intel_encoder_destroy`. **However**, there's a subtlety here. `intel_encoder_destroy()` does `drm_e= ncoder_cleanup(encoder); kfree(intel_encoder)`. But `lvds_encoder` is a `st= ruct intel_lvds_encoder` which **wraps** `struct intel_encoder`. The `.dest= roy` callback kfrees `intel_encoder` (via `to_intel_encoder(encoder)`), whi= ch is `&lvds_encoder->base`. Since `intel_encoder` is embedded in `intel_lv= ds_encoder`, this should free the whole `lvds_encoder` allocation =E2=80=94= so the kfree is actually fine. Wait =E2=80=94 let me re-examine. `intel_encoder_destroy()` does: ```c struct intel_encoder *intel_encoder =3D to_intel_encoder(encoder); drm_encoder_cleanup(encoder); kfree(intel_encoder); ``` And `lvds_encoder->base` is `struct intel_encoder`, and `intel_encoder` was= allocated as part of `kzalloc_obj(*lvds_encoder)`. So `kfree(intel_encoder= )` where `intel_encoder =3D &lvds_encoder->base` would free the containing = struct only if `base` is at offset 0. Looking at the struct definition: The `kfree()` with a pointer to a member at a non-zero offset would be a bu= g. However, `container_of` is typically used, and indeed `to_intel_encoder`= casts from `drm_encoder` which is inside `intel_encoder`. The kfree of `in= tel_encoder` pointer would work correctly if `intel_encoder` is at offset 0= within `intel_lvds_encoder`, but if not, this is undefined behavior / memo= ry corruption. Actually, looking more carefully at the original code: the `failed:` label = calls `kfree(lvds_encoder)` =E2=80=94 the **outer** struct =E2=80=94 which = is the correct pointer. `intel_encoder_destroy()` calls `kfree(intel_encode= r)` which is `&lvds_encoder->base` =E2=80=94 this is only correct if `base`= is at offset 0, which is a common but fragile assumption. Regardless of that subtlety, the bigger issue with this patch is: **the pat= ch removes a `drm_connector_cleanup()` call and relies on the DRM core tear= down to do it, but also removes `intel_connector_free()`.** The original cl= eanup calls: ```c drm_connector_cleanup(&connector->base); drm_encoder_cleanup(&encoder->base); kfree(lvds_encoder); intel_connector_free(connector); ``` The DRM core teardown path will call `.destroy` callbacks: `intel_connector= _destroy` for the connector and `intel_encoder_destroy` for the encoder. Th= ese should handle everything. **So the patch may actually be safe**, since = the `.destroy` callbacks handle the same cleanup. On closer analysis: `intel_connector_destroy()` calls `drm_connector_cleanu= p()` + `intel_connector_free()`, and `intel_encoder_destroy()` calls `drm_e= ncoder_cleanup()` + `kfree(intel_encoder)`. So the DRM core teardown **does= ** cover this. **Revised verdict: The patch is probably safe**, since the half-initialized= objects will remain on the DRM mode_config lists and get cleaned up at dev= ice removal. But the commit message's claim that this is a "double cleanup"= fix is **misleading** =E2=80=94 the original code performs cleanup eagerly= in the error path and then the objects are already gone by the time the DR= M core tries to tear down. The "double" would only happen if the objects re= mained registered after the manual cleanup (but `drm_connector_cleanup` unr= egisters them from the list, so the `.destroy` callback wouldn't fire again= ). Actually, now I see the real issue: the existing code calls `drm_connector_= cleanup()` which removes the connector from `mode_config.connector_list`. T= hen `drm_mode_config_cleanup()` won't find it, so `.destroy` won't be calle= d. And `intel_connector_free()` frees the connector memory. The encoder is = similar. So the current code is **not a double free** =E2=80=94 it's a sing= le, correct, eager cleanup. **The patch therefore removes a valid cleanup path and claims it's a double= -free fix, when it isn't.** The code without the patch will leave half-init= ialized objects around until device teardown, which is tolerable but wastef= ul compared to the current eager cleanup. The existing code is correct. **Final verdict: The patch is technically safe but the justification is wro= ng. The original code is NOT a double-free =E2=80=94 it's correct eager cle= anup. The patch makes the code rely on deferred cleanup at device teardown = instead, which works but is inferior.** --- Generated by Claude Code Patch Reviewer