Claude review: drm/i915/display/intel_lvds: Drop redundant manual cleanup on init failure

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Verdict: NACK — this patch introduces a resource leak.**

The commit message claims the manual cleanup in the `failed:` label is "redundant" because the DRM core will invoke `.destroy` callbacks. This is **incorrect** for `intel_lvds_init()`.

`intel_lvds_init()` returns `void`. It is called as a fire-and-forget initialization 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_encoder_init()` have succeeded but before the function completes successfully, the half-initialized connector and encoder objects remain registered with the DRM core.

The key question is: will the DRM core's teardown path call `.destroy` on these objects during device shutdown? Actually, yes — `drm_connector_init_with_ddc()` adds the connector to `mode_config.connector_list`, so `drm_mode_config_cleanup()` will call `.destroy` on it, which is `intel_connector_destroy`. Similarly, `drm_encoder_init()` adds the encoder to the encoder list, so `drm_mode_config_cleanup()` will call `intel_encoder_destroy`.

**However**, there's a subtlety here. `intel_encoder_destroy()` does `drm_encoder_cleanup(encoder); kfree(intel_encoder)`. But `lvds_encoder` is a `struct intel_lvds_encoder` which **wraps** `struct intel_encoder`. The `.destroy` callback kfrees `intel_encoder` (via `to_intel_encoder(encoder)`), which is `&lvds_encoder->base`. Since `intel_encoder` is embedded in `intel_lvds_encoder`, this should free the whole `lvds_encoder` allocation — so the kfree is actually fine.

Wait — let me re-examine. `intel_encoder_destroy()` does:
```c
struct intel_encoder *intel_encoder = 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 = &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 bug. However, `container_of` is typically used, and indeed `to_intel_encoder` casts from `drm_encoder` which is inside `intel_encoder`. The kfree of `intel_encoder` pointer would work correctly if `intel_encoder` is at offset 0 within `intel_lvds_encoder`, but if not, this is undefined behavior / memory corruption.

Actually, looking more carefully at the original code: the `failed:` label calls `kfree(lvds_encoder)` — the **outer** struct — which is the correct pointer. `intel_encoder_destroy()` calls `kfree(intel_encoder)` which is `&lvds_encoder->base` — 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 patch removes a `drm_connector_cleanup()` call and relies on the DRM core teardown to do it, but also removes `intel_connector_free()`.** The original cleanup 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. These 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_cleanup()` + `intel_connector_free()`, and `intel_encoder_destroy()` calls `drm_encoder_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 device removal. But the commit message's claim that this is a "double cleanup" fix is **misleading** — the original code performs cleanup eagerly in the error path and then the objects are already gone by the time the DRM core tries to tear down. The "double" would only happen if the objects remained registered after the manual cleanup (but `drm_connector_cleanup` unregisters 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`. Then `drm_mode_config_cleanup()` won't find it, so `.destroy` won't be called. And `intel_connector_free()` frees the connector memory. The encoder is similar. So the current code is **not a double free** — it's a single, 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-initialized objects around until device teardown, which is tolerable but wasteful compared to the current eager cleanup. The existing code is correct.

**Final verdict: The patch is technically safe but the justification is wrong. The original code is NOT a double-free — it's correct eager cleanup. The patch makes the code rely on deferred cleanup at device teardown instead, which works but is inferior.**

---
Generated by Claude Code Patch Reviewer