Claude review: drm/i915/display/intel_dp: Drop redundant intel_dp_aux_fini() on init failure

Claude review: drm/i915/display/intel_dp: Drop redundant intel_dp_aux_fini() on init failure

[Claude Code Review Bot]

Patch Review

Correct. The `fail:` label already calls `drm_connector_cleanup()` which triggers encoder destroy paths, and `intel_dp_encoder_flush_work()` (called from both `intel_dp_encoder_destroy()` and `intel_ddi_encoder_destroy()`) already calls `intel_dp_aux_fini()`. The explicit call was redundant.

---
Generated by Claude Code Patch Reviewer

[PATCH 0/3] drm/i915: Fix double cleanup in error paths

[Kory Maincent]

Several error paths in the i915 driver incorrectly invoke cleanup
functions multiple times, potentially causing double-free errors.
This series corrects these paths to ensure cleanup is performed
only once.

Testing note: Only the DisplayPort fix has been hardware tested due
to lack of available hardware for the other components.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Kory Maincent (3):
      drm/i915/display/intel_sdvo: Fix double connector destroy in error paths
      drm/i915/display/intel_lvds: Drop redundant manual cleanup on init failure
      drm/i915/display/intel_dp: Drop redundant intel_dp_aux_fini() on init failure

 drivers/gpu/drm/i915/display/intel_dp.c   |  4 +---
 drivers/gpu/drm/i915/display/intel_lvds.c | 14 ++++----------
 drivers/gpu/drm/i915/display/intel_sdvo.c | 16 ++++------------
 3 files changed, 9 insertions(+), 25 deletions(-)
---
base-commit: e1696f1fc99dc0ff761a012230587b23dec064fb
change-id: 20260603-fix_i915-5c3d8c09662a

Best regards,
--  
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH 1/3] drm/i915/display/intel_sdvo: Fix double connector destroy in error paths

[Kory Maincent]

intel_sdvo_connector_funcs registers intel_connector_destroy() as the
.destroy callback. Once drm_connector_init_with_ddc() succeeds inside
intel_sdvo_connector_init(), the DRM core takes ownership of the
connector object and will call .destroy on teardown.

The error labels in intel_sdvo_tv_init() and intel_sdvo_lvds_init()
call intel_connector_destroy() explicitly before returning false,
causing it to be invoked twice: once in the error path and again by
the DRM core through the registered .destroy callback.

Remove the manual intel_connector_destroy() calls from the error labels
and return false directly instead.

Cc: stable@vger.kernel.org
Fixes: 32aad86fe88e7 ("drm/i915/sdvo: Propagate errors from reading/writing control bus.")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Not tested as I don't have such hardware.
---
 drivers/gpu/drm/i915/display/intel_sdvo.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index d83d350959d88..0f3aa879e39e1 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -2878,16 +2878,12 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, u16 type)
 	}
 
 	if (!intel_sdvo_tv_create_property(intel_sdvo, intel_sdvo_connector, type))
-		goto err;
+		return false;
 
 	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
-		goto err;
+		return false;
 
 	return true;
-
-err:
-	intel_connector_destroy(connector);
-	return false;
 }
 
 static bool
@@ -2950,7 +2946,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, u16 type)
 	}
 
 	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
-		goto err;
+		return false;
 
 	intel_bios_init_panel_late(display, &intel_connector->panel, NULL, NULL);
 
@@ -2972,13 +2968,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, u16 type)
 	intel_panel_init(intel_connector, NULL);
 
 	if (!intel_panel_preferred_fixed_mode(intel_connector))
-		goto err;
+		return false;
 
 	return true;
-
-err:
-	intel_connector_destroy(connector);
-	return false;
 }
 
 static u16 intel_sdvo_filter_output_flags(u16 flags)

-- 
2.43.0

[PATCH 2/3] drm/i915/display/intel_lvds: Drop redundant manual cleanup on init failure

[Kory Maincent]

intel_lvds_init() had a goto-based error path that manually called
drm_connector_cleanup(), drm_encoder_cleanup(), kfree() and
intel_connector_free() when no LVDS panel mode could be found.

Once drm_connector_init_with_ddc() and drm_encoder_init() have been
called, the DRM core takes ownership of these objects and will invoke
their .destroy callbacks (intel_connector_destroy and
intel_encoder_destroy) during device teardown. The manual cleanup in
the failed: label is therefore redundant.

Remove it and replace the goto with a simple early return.

Cc: stable@vger.kernel.org
Fixes: 79e539453b34e ("DRM: i915: add mode setting support")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Not tested as I don't have such hardware.
---
 drivers/gpu/drm/i915/display/intel_lvds.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index c8098104d853a..2a1301eda5fe5 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -990,8 +990,10 @@ void intel_lvds_init(struct intel_display *display)
 	mutex_unlock(&display->drm->mode_config.mutex);
 
 	/* If we still don't have a mode after all that, give up. */
-	if (!intel_panel_preferred_fixed_mode(connector))
-		goto failed;
+	if (!intel_panel_preferred_fixed_mode(connector)) {
+		drm_dbg_kms(display->drm, "No LVDS modes found, disabling.\n");
+		return;
+	}
 
 	intel_panel_init(connector, drm_edid);
 
@@ -1004,12 +1006,4 @@ void intel_lvds_init(struct intel_display *display)
 	lvds_encoder->a3_power = lvds & LVDS_A3_POWER_MASK;
 
 	return;
-
-failed:
-	drm_dbg_kms(display->drm, "No LVDS modes found, disabling.\n");
-	drm_connector_cleanup(&connector->base);
-	drm_encoder_cleanup(&encoder->base);
-	kfree(lvds_encoder);
-	intel_connector_free(connector);
-	return;
 }

-- 
2.43.0

[PATCH 3/3] drm/i915/display/intel_dp: Drop redundant intel_dp_aux_fini() on init failure

[Kory Maincent]

intel_dp_aux_fini() is already invoked via intel_dp_encoder_flush_work()
in the encoder destroy path (intel_dp_encoder_destroy() and
intel_ddi_encoder_destroy()). Calling it explicitly when
intel_edp_init_connector() fails before jumping to the fail label
therefore results in a double invocation. Drop the redundant call.

Cc: stable@vger.kernel.org
Fixes: c191eca110a37 ("drm/i915: Move intel_connector->unregister to connector->early_unregister")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index f01a6eed38395..f4fab568172f4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -7310,10 +7310,8 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
 		connector->get_hw_state = intel_connector_get_hw_state;
 	connector->sync_state = intel_dp_connector_sync_state;
 
-	if (!intel_edp_init_connector(intel_dp, connector)) {
-		intel_dp_aux_fini(intel_dp);
+	if (!intel_edp_init_connector(intel_dp, connector))
 		goto fail;
-	}
 
 	intel_dp_set_source_rates(intel_dp);
 	intel_dp_set_common_rates(intel_dp);

-- 
2.43.0

Re: [PATCH 3/3] drm/i915/display/intel_dp: Drop redundant intel_dp_aux_fini() on init failure

[Imre Deak]

On Wed, Jun 03, 2026 at 10:59:54AM +0200, Kory Maincent wrote:
> intel_dp_aux_fini() is already invoked via intel_dp_encoder_flush_work()
> in the encoder destroy path (intel_dp_encoder_destroy() and
> intel_ddi_encoder_destroy()). Calling it explicitly when
> intel_edp_init_connector() fails before jumping to the fail label
> therefore results in a double invocation. Drop the redundant call.

Could you please describe the exact call chain leading to
intel_dp_encoder_flush_work() if intel_dp_init_connector() fails?

> Cc: stable@vger.kernel.org
> Fixes: c191eca110a37 ("drm/i915: Move intel_connector->unregister to connector->early_unregister")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index f01a6eed38395..f4fab568172f4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7310,10 +7310,8 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  		connector->get_hw_state = intel_connector_get_hw_state;
>  	connector->sync_state = intel_dp_connector_sync_state;
>  
> -	if (!intel_edp_init_connector(intel_dp, connector)) {
> -		intel_dp_aux_fini(intel_dp);
> +	if (!intel_edp_init_connector(intel_dp, connector))
>  		goto fail;
> -	}
>  
>  	intel_dp_set_source_rates(intel_dp);
>  	intel_dp_set_common_rates(intel_dp);
> 
> -- 
> 2.43.0
> 

Re: [PATCH 0/3] drm/i915: Fix double cleanup in error paths

[Kory Maincent]

On Wed, 03 Jun 2026 10:59:51 +0200
Kory Maincent <kory.maincent@bootlin.com> wrote:

> Several error paths in the i915 driver incorrectly invoke cleanup
> functions multiple times, potentially causing double-free errors.
> This series corrects these paths to ensure cleanup is performed
> only once.
> 
> Testing note: Only the DisplayPort fix has been hardware tested due
> to lack of available hardware for the other components.

Don't pay attention to this series.
I missed the point that if drm_encoder/connector_cleanup() are called it will
remove the encoder/connector from the DRM core list, therefore the destroy
callback will be never called. Which means no double cleanup path.
Sorry for the noise.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Claude review: drm/i915: Fix double cleanup in error paths

[Claude Code Review Bot]

Overall Series Review

Subject: drm/i915: Fix double cleanup in error paths
Author: Kory Maincent <kory.maincent@bootlin.com>
Patches: 6
Reviewed: 2026-06-04T11:52:28.153352

---

This is a 3-patch series fixing double-cleanup issues in i915 display connector/encoder error paths. The core claim is correct for patches 1 and 3: once `drm_connector_init_with_ddc()` succeeds, the DRM core owns the object and will call `.destroy` during teardown, so manual cleanup in error paths causes double-free. However, **patch 2 has a significant problem** — it introduces a resource leak by removing cleanup that is actually needed.

The series is Cc: stable, which is appropriate for patches 1 and 3 but patch 2 needs rework. Overall the direction is right but the LVDS patch needs a different approach.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/i915/display/intel_sdvo: Fix double connector destroy in error paths

[Claude Code Review Bot]

Patch Review

**Verdict: Correct.**

The analysis is sound. Looking at the code:

1. `intel_sdvo_connector_init()` calls `drm_connector_init_with_ddc()` with `intel_sdvo_connector_funcs`, which has `.destroy = intel_connector_destroy` (line 2514 of `intel_sdvo.c`).

2. Once `drm_connector_init_with_ddc()` succeeds, the connector is registered with the DRM core.

3. In `intel_sdvo_tv_init()` and `intel_sdvo_lvds_init()`, the `err:` labels call `intel_connector_destroy(connector)` explicitly, but the DRM core will also invoke it via the `.destroy` callback — a genuine double-free.

4. The caller `intel_sdvo_output_setup()` at line 3049 returns false on failure, and the cleanup path at `intel_sdvo_output_cleanup()` (line 3058) iterates all connectors attached to this encoder and calls `intel_connector_destroy()` on each — so the cleanup IS handled properly by the caller.

The fix to just `return false` is correct. No concerns.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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

Claude review: drm/i915/display/intel_dp: Drop redundant intel_dp_aux_fini() on init failure

[Claude Code Review Bot]

Patch Review

**Verdict: Correct.**

The analysis is clear:

1. When `intel_edp_init_connector()` fails, the code calls `intel_dp_aux_fini(intel_dp)` then jumps to `fail:`.

2. The `fail:` label at line 7467 calls `drm_connector_cleanup()`. The connector was initialized with `intel_dp_connector_funcs` which has `.destroy` — but `drm_connector_cleanup()` does NOT call `.destroy`, it just removes from the list and cleans up state.

3. However, the *encoder* will still have its `.destroy` callback called during device teardown — either `intel_dp_encoder_destroy` or `intel_ddi_encoder_destroy`, both of which call `intel_dp_encoder_flush_work()`, which calls `intel_dp_aux_fini()` at line 6811.

4. So `intel_dp_aux_fini()` would be called twice: once explicitly in the error path, and once via the encoder destroy callback.

The fix correctly removes the explicit call, keeping only the one in the encoder destroy path. The `intel_dp_aux_fini()` function unregisters the aux I2C adapter, and calling it twice would indeed be problematic (double unregister/double kfree of adapter name).

One minor note: the patch also changes the code style from a `{ }` block to a bare `if` statement with the `goto`, which is fine since the single statement no longer needs braces.

**Clean and correct fix.**

---
Generated by Claude Code Patch Reviewer