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/bridge: samsung-dsim: Fix memory leak in error path Date: Wed, 11 Feb 2026 16:53:11 +1000 Message-ID: In-Reply-To: <20260209184115.10937-1-osama.abdelkader@gmail.com> References: <20260209184115.10937-1-osama.abdelkader@gmail.com> <20260209184115.10937-1-osama.abdelkader@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Mailer: Claude Code Patch Reviewer 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