public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/bridge: samsung-dsim: fix TE GPIO ownership
@ 2026-06-04 11:46 Guangshuo Li
  2026-06-04 20:32 ` Claude review: " Claude Code Review Bot
  2026-06-04 20:32 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Guangshuo Li @ 2026-06-04 11:46 UTC (permalink / raw)
  To: Inki Dae, Jagan Teki, Marek Szyprowski, Andrzej Hajda,
	Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Marek Vasut,
	dri-devel, linux-kernel
  Cc: Guangshuo Li

samsung_dsim_register_te_irq() acquires the TE GPIO with
devm_gpiod_get_optional(), but the descriptor is released manually in the
request_threaded_irq() error path and in samsung_dsim_unregister_te_irq().
The devres entry remains registered and will release the same descriptor
again when the device is detached.

Use the non-managed gpiod_get_optional() helper instead, so the GPIO
descriptor lifetime matches the existing manual cleanup paths. Also clear
dsi->te_gpio after each gpiod_put() to avoid leaving a stale descriptor
pointer around if the host is attached again.

Fixes: e7447128ca4a ("drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index c3eb437ef1b0..2299b7dc0c04 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1862,7 +1862,7 @@ static int samsung_dsim_register_te_irq(struct samsung_dsim *dsi, struct device
 	int te_gpio_irq;
 	int ret;
 
-	dsi->te_gpio = devm_gpiod_get_optional(dev, "te", GPIOD_IN);
+	dsi->te_gpio = gpiod_get_optional(dev, "te", GPIOD_IN);
 	if (!dsi->te_gpio)
 		return 0;
 	else if (IS_ERR(dsi->te_gpio))
@@ -1875,6 +1875,7 @@ static int samsung_dsim_register_te_irq(struct samsung_dsim *dsi, struct device
 	if (ret) {
 		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
 		gpiod_put(dsi->te_gpio);
+		dsi->te_gpio = NULL;
 		return ret;
 	}
 
@@ -1886,6 +1887,7 @@ 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);
+		dsi->te_gpio = NULL;
 	}
 }
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/bridge: samsung-dsim: fix TE GPIO ownership
  2026-06-04 11:46 [PATCH] drm/bridge: samsung-dsim: fix TE GPIO ownership Guangshuo Li
@ 2026-06-04 20:32 ` Claude Code Review Bot
  2026-06-04 20:32 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 20:32 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/bridge: samsung-dsim: fix TE GPIO ownership
Author: Guangshuo Li <lgs201920130244@gmail.com>
Patches: 1
Reviewed: 2026-06-05T06:32:39.803290

---

This is a single-patch fix for a real resource management bug in the Samsung DSIM bridge driver. The analysis in the commit message is correct: `devm_gpiod_get_optional()` registers a devres cleanup action that will call `gpiod_put()` automatically on device detach, but the driver also manually calls `gpiod_put()` in both the error path of `samsung_dsim_register_te_irq()` and in `samsung_dsim_unregister_te_irq()`. This results in a double-free of the GPIO descriptor — once via the manual `gpiod_put()` and again when devres runs during device detach.

The fix is correct and minimal: switch to non-managed `gpiod_get_optional()` so that the manual `gpiod_put()` calls are the sole owners of the descriptor's lifetime. The addition of `dsi->te_gpio = NULL` after each `gpiod_put()` is a good defensive practice.

**Verdict: Looks good. Correct fix for a genuine double-free bug.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/bridge: samsung-dsim: fix TE GPIO ownership
  2026-06-04 11:46 [PATCH] drm/bridge: samsung-dsim: fix TE GPIO ownership Guangshuo Li
  2026-06-04 20:32 ` Claude review: " Claude Code Review Bot
@ 2026-06-04 20:32 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 20:32 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**The bug is real and well-described.** The original code at line 1865 used `devm_gpiod_get_optional()`, but both error and teardown paths manually call `gpiod_put()`. On device detach, devres would fire and double-free the descriptor.

**The fix is correct:**

1. **`devm_gpiod_get_optional()` → `gpiod_get_optional()`** (line 1865): This removes the devres registration, making the manual `gpiod_put()` calls the sole cleanup mechanism. Correct.

2. **`dsi->te_gpio = NULL` after `gpiod_put()` in the error path** (line 1878): Good. Without this, if `samsung_dsim_register_te_irq()` fails and the caller (or a later re-attach) checks `dsi->te_gpio`, it would see a dangling pointer. This is especially relevant since `samsung_dsim_unregister_te_irq()` is called unconditionally in `samsung_dsim_host_detach()` (line 2012) — with a stale pointer that would trigger a use-after-free.

3. **`dsi->te_gpio = NULL` after `gpiod_put()` in the unregister path** (line 1890): Good defensive practice. Protects against double-unregister if `samsung_dsim_host_detach()` were ever called twice, and makes re-attach safe.

**One minor pre-existing nit (not introduced by this patch):** In the `IS_ERR()` path (line 1868-1869), the function returns an error but leaves `dsi->te_gpio` set to an error pointer. If `samsung_dsim_enable_irq()` or `samsung_dsim_disable_irq()` were called after a failed `register_te_irq()`, the `if (dsi->te_gpio)` check at lines 1606/1612 would be true (error pointers are non-NULL), leading to `gpiod_to_irq()` on an invalid descriptor. In the current call flow this doesn't happen because `register_te_irq` failure causes `samsung_dsim_host_attach` to bail out at `err_remove_bridge` before the bridge is usable, but it's still a latent hazard. A `dsi->te_gpio = NULL` or wrapping the return in a local variable would tighten it up. But that's pre-existing and out of scope for this patch.

**Reviewed-by worthy.** The commit message is clear, the Fixes tag is appropriate, the change is minimal and correct.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-06-04 20:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-04 11:46 [PATCH] drm/bridge: samsung-dsim: fix TE GPIO ownership Guangshuo Li
2026-06-04 20:32 ` Claude review: " Claude Code Review Bot
2026-06-04 20:32 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox