public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path
@ 2026-04-12 12:06 Guangshuo Li
  2026-04-13  9:12 ` Claude review: " Claude Code Review Bot
  2026-04-13  9:12 ` Claude Code Review Bot
  0 siblings, 2 replies; 5+ messages in thread
From: Guangshuo Li @ 2026-04-12 12:06 UTC (permalink / raw)
  To: Thierry Reding, Mikko Perttunen, David Airlie, Simona Vetter,
	Mark Zhang, Sean Paul, dri-devel, linux-tegra, linux-kernel
  Cc: Guangshuo Li

After device_initialize(), the embedded struct device in struct
host1x_device should be released through the device core with
put_device().

In host1x_device_add(), if host1x_device_parse_dt() fails, the current
error path frees the object directly with kfree(device). That bypasses
the normal device lifetime handling and leaks the reference held on the
embedded struct device.

Fix this by using put_device() in the host1x_device_parse_dt() failure
path.

Fixes: f4c5cf88fbd50 ("gpu: host1x: Provide a proper struct bus_type")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/host1x/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 723a80895cd4..f97567e6ae87 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -452,7 +452,7 @@ static int host1x_device_add(struct host1x *host1x,
 
 	err = host1x_device_parse_dt(device, driver);
 	if (err < 0) {
-		kfree(device);
+		put_device(&device->dev);
 		return err;
 	}
 
-- 
2.43.0


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

* Claude review: gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path
  2026-04-12 12:26 [PATCH v2] " Guangshuo Li
  2026-04-13  9:07 ` Claude review: " Claude Code Review Bot
@ 2026-04-13  9:07 ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-13  9:07 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path
Author: Guangshuo Li <lgs201920130244@gmail.com>
Patches: 1
Reviewed: 2026-04-13T19:07:35.144042

---

This is a single-patch series that fixes a legitimate device reference leak in the host1x driver. The analysis is correct: after `device_initialize()` is called, the proper way to free the device is through `put_device()`, not `kfree()`. The fix itself is sound — `put_device()` will invoke `host1x_device_release()`, which calls `__host1x_device_del()` and then `kfree()`.

The release path is safe in this context: `__host1x_device_del()` iterates `device->active` (empty at this point since no clients have been registered), `device->subdevs` (may contain entries added by `host1x_device_parse_dt()` before it failed — these get properly cleaned up via `host1x_subdev_del()`), `device->clients` (empty), and calls `list_del_init(&device->list)` (safe since `INIT_LIST_HEAD` was called). So `put_device()` correctly tears down any partial state that `host1x_device_parse_dt()` may have built up.

**Verdict: The patch is correct and suitable for merging.**

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path
  2026-04-12 12:26 [PATCH v2] " Guangshuo Li
@ 2026-04-13  9:07 ` Claude Code Review Bot
  2026-04-13  9:07 ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-13  9:07 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.** The commit message accurately describes the problem. After `device_initialize()` at `bus.c:466`, the embedded `struct device` holds a reference count of 1. Using `kfree()` directly bypasses the device core's lifetime management. Switching to `put_device()` is the right fix — it will decrement the refcount to 0 and invoke the `host1x_device_release` callback set at `bus.c:479`, which calls `__host1x_device_del()` followed by `kfree()`.

**Release callback safety:** The `__host1x_device_del()` function (`bus.c:398`) is safe to call in this early failure state:
- `device->active` — empty, since no subdevices have been registered with clients. The loop at line 406 is a no-op.
- `device->subdevs` — may have entries from the partially-completed `host1x_device_parse_dt()`. These are properly cleaned up by `host1x_subdev_del()` at line 427-428, which does `list_del()`, `of_node_put()`, and `kfree()`. This is actually a secondary fix: the old `kfree(device)` code would also leak any subdevices that were added before `host1x_device_parse_dt()` failed.
- `device->clients` — empty, loop at line 436 is a no-op.
- `list_del_init(&device->list)` at line 443 — safe because `INIT_LIST_HEAD(&device->list)` was called at line 473. Calling `list_del_init` on a self-linked list node is a no-op.

**Pre-existing issue worth noting:** `host1x_device_parse_dt()` (`bus.c:88`) and `host1x_subdev_add()` (`bus.c:39`) have their own cleanup gap — when `host1x_subdev_add()` fails mid-recursion at line 62-64 (note the `/* XXX cleanup? */` comment), already-added subdevices from earlier iterations are left on the list without cleanup. However, with this patch applied, those orphaned subdevices are now properly freed through the `put_device()` → `host1x_device_release()` → `__host1x_device_del()` path, which iterates and frees all entries on `device->subdevs`. So this patch incidentally fixes that leak too.

**Minor nits:**
- The commit message title says "in host1x_device_parse_dt() error path" — it would be slightly more precise as "in host1x_device_add() error path" since the fix is in `host1x_device_add()`, not in `host1x_device_parse_dt()` itself. But this is a cosmetic quibble.
- The Fixes tag references `f4c5cf88fbd50`, which introduced `device_initialize()` usage. This is appropriate.

**Reviewed-by recommendation:** This patch is straightforward, correct, and appropriate for stable backport.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path
  2026-04-12 12:06 [PATCH] gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path Guangshuo Li
@ 2026-04-13  9:12 ` Claude Code Review Bot
  2026-04-13  9:12 ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-13  9:12 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path
Author: Guangshuo Li <lgs201920130244@gmail.com>
Patches: 1
Reviewed: 2026-04-13T19:12:54.751886

---

This is a single-patch fix for a device reference handling bug in the host1x bus driver. The analysis is correct and the fix is appropriate. After `device_initialize()` sets up the embedded `struct device` (including refcount = 1 and kobject internals), the proper teardown path is `put_device()`, not a raw `kfree()`. The patch is minimal, well-targeted, and the commit message adequately explains the rationale.

**Verdict: The patch is correct and should be accepted.**

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path
  2026-04-12 12:06 [PATCH] gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path Guangshuo Li
  2026-04-13  9:12 ` Claude review: " Claude Code Review Bot
@ 2026-04-13  9:12 ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-13  9:12 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Analysis of the bug:**

In `host1x_device_add()` (bus.c:454), the allocation and initialization sequence is:

```c
device = kzalloc_obj(*device);           // line 462
device_initialize(&device->dev);          // line 466 — refcount=1, kobject init'd
...
device->dev.release = host1x_device_release;  // line 479

err = host1x_device_parse_dt(device, driver);
if (err < 0) {
    kfree(device);   // BUG: bypasses device core teardown
    return err;
}
```

After `device_initialize()`, the device's kobject and refcount are live. Calling `kfree()` directly skips kobject cleanup and the `.release` callback.

**The fix is correct.** Replacing `kfree(device)` with `put_device(&device->dev)` drops the refcount to zero, which invokes `host1x_device_release()` (bus.c:446). That callback calls `__host1x_device_del(device)` then `kfree(device)`, following the proper device lifecycle.

**Safety of `__host1x_device_del()` in this partially-initialized state:** All the lists and mutexes it touches (`subdevs_lock`, `subdevs`, `active`, `clients_lock`, `clients`, `list`) are initialized at lines 468–474, before `host1x_device_parse_dt()` is called. The `device->list` hasn't been added to `host1x->devices` yet (that happens at line 492, after the error check), but `INIT_LIST_HEAD` makes it self-referential, so `list_del_init()` is a safe no-op.

**Additional benefit:** `host1x_device_parse_dt()` calls `host1x_subdev_add()` which may have already added subdevices to `device->subdevs` before a later `host1x_subdev_add()` call fails (note the existing `/* XXX cleanup? */` comment at bus.c:63). With the old `kfree(device)`, those partially-added subdevices and their `of_node_get()` references would be leaked. With `put_device()`, `__host1x_device_del()` iterates `device->subdevs` and calls `host1x_subdev_del()` on each, properly cleaning them up.

**Minor nit on the commit message:** The description says "leaks the reference held on the embedded struct device," which is slightly imprecise — there's no *external* holder leaking a reference. The real issue is that `kfree()` bypasses the device core's lifetime management (kobject teardown) after `device_initialize()`. This is a cosmetic observation and doesn't affect the correctness of the patch.

**Fixes tag:** References `f4c5cf88fbd50` ("gpu: host1x: Provide a proper struct bus_type"), which is the commit that introduced `device_initialize()` in this code path. Appropriate.

**Reviewed-by worthy:** Yes. Clean, correct, minimal fix.

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-04-13  9:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-12 12:06 [PATCH] gpu: host1x: Fix device reference leak in host1x_device_parse_dt() error path Guangshuo Li
2026-04-13  9:12 ` Claude review: " Claude Code Review Bot
2026-04-13  9:12 ` Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-04-12 12:26 [PATCH v2] " Guangshuo Li
2026-04-13  9:07 ` Claude review: " Claude Code Review Bot
2026-04-13  9:07 ` 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