Claude review: drm/bridge: samsung-dsim: Fix memory leak in error path

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

Patch Review

**Commit Message Review:**

The commit message clearly explains the problem and solution. However, there's a formatting issue:

```
v2: 
- Move samsung_dsim_unregister_te_irq() function
- Add Fixes tag
- Add Cc tag
```

**Issue:** Version changelog should appear after the `---` separator (line 117), not before the Signed-off-by tag. This prevents the version notes from being included in the permanent git history.

**Technical Review:**

**1. Function movement (lines 134-140):**
```c
+static void samsung_dsim_unregister_te_irq(struct samsung_dsim *dsi)
+{
+	if (dsi->te_gpio) {
+		free_irq(gpiod_to_irq(dsi->te_gpio), dsi);
+		gpiod_put(dsi->te_gpio);
+	}
+}
```

**Analysis:** Moving this function upward is necessary and correct. The function is unchanged, which is good - it keeps the patch focused on the fix rather than mixing refactoring with bug fixes. This is proper kernel style.

**2. Error path for TE IRQ registration (lines 146-150):**
```c
 	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
 		ret = samsung_dsim_register_te_irq(dsi, &device->dev);
 		if (ret)
-			return ret;
+			goto err_remove_bridge;
 	}
```

**Analysis:** Correct. The bridge was added earlier via `drm_bridge_add()` (line 1950 in original code, not shown in diff), so this error path must remove it.

**3. Error path for host attach (lines 153-157):**
```c
 	if (pdata->host_ops && pdata->host_ops->attach) {
 		ret = pdata->host_ops->attach(dsi, device);
 		if (ret)
-			return ret;
+			goto err_unregister_te_irq;
 	}
```

**Analysis:** Correct. At this point, both the bridge has been added AND the TE IRQ may have been registered (depending on the mode). The error label correctly handles both.

**4. Error unwinding implementation (lines 173-178):**
```c
+err_unregister_te_irq:
+	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO))
+		samsung_dsim_unregister_te_irq(dsi);
+err_remove_bridge:
+	drm_bridge_remove(&dsi->bridge);
+	return ret;
 }
```

**Critical issue to verify:** The conditional check `if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO))` in the error path mirrors the check used when registering the TE IRQ. This is correct because:
- TE IRQ is only registered when NOT in video mode
- The unregister function itself has a null check (`if (dsi->te_gpio)`)
- This provides defense-in-depth

**Potential concern:** The label naming follows reverse-chronological unwinding order (you jump to the LATER operation's label, then fall through to earlier ones). Let me verify the order:

1. `drm_bridge_add()` happens first (not shown in diff)
2. `samsung_dsim_register_te_irq()` happens second (conditionally)
3. `pdata->host_ops->attach()` happens third

Error unwinding should be in reverse order:
1. First we might need to unregister TE IRQ (if it was registered)
2. Then we always need to remove bridge

The implementation is **correct** - `err_unregister_te_irq` cleans up TE IRQ (if needed) then falls through to `err_remove_bridge`.

**5. Symmetry check:**

Looking at the detach path (`samsung_dsim_host_detach`, line 181), it should mirror this cleanup. The original code shows `samsung_dsim_unregister_te_irq(dsi)` was already there, so symmetry is maintained.

**Code Style Review:**
- Indentation: Correct (tabs)
- Goto labels: Properly placed (no indentation for labels, consistent with kernel style)
- Line length: Within acceptable limits
- Error handling pattern: Standard kernel unwinding style

**Fixes Tag Review:**
```
Fixes: e7447128ca4a ("drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge")
```

The Fixes tag appears appropriate. This commit likely introduced the `drm_bridge_add()` call without proper cleanup on error paths. A reviewer with access to git history should verify this is the correct commit.

**Testing Considerations:**

This patch modifies error paths, which are difficult to test without fault injection. The fix is correct by inspection, but ideally should be tested by:
1. Forcing `samsung_dsim_register_te_irq()` to fail
2. Forcing `pdata->host_ops->attach()` to fail
3. Verifying `drm_bridge_remove()` is called and no resources leak

**Minor observations:**

The patch shows version notes at line 118-121 within the commit message body. As mentioned by Luca Ceresoli's review (lines 282-290), these should be below the `---` line. The current location will cause them to be included in the permanent git history.

**Verdict:**

**Acked-by: [Reviewer]** (technical content is correct)

The code changes are sound and properly implement error unwinding. The only issue is the changelog placement, which is a process/formatting issue rather than a technical problem. The patch should be accepted with the understanding that future submissions should use proper version marking in the subject line and place version notes after the `---` separator.

---
Generated by Claude Code Patch Reviewer