From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: gpu: host1x: Allow entries in BO caches to be freed
Date: Sat, 16 May 2026 10:12:28 +1000
Message-ID: <review-patch1-20260515-host1x-bocache-leak-v1-1-a0375f68aeab@nvidia.com>
In-Reply-To: <20260515-host1x-bocache-leak-v1-1-a0375f68aeab@nvidia.com>
References: <20260515-host1x-bocache-leak-v1-0-a0375f68aeab@nvidia.com> <20260515-host1x-bocache-leak-v1-1-a0375f68aeab@nvidia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Refcount centralization (tegra gem.c, submit.c):**

The removal of `host1x_bo_get(bo)` from `tegra_bo_pin` and `gather_bo_pin`,=
 and `host1x_bo_put(map->bo)` from their unpin counterparts, is correct. Th=
e new contract documented in the `host1x_bo_ops` comment makes the expectat=
ion clear:

```c
/**
 * @pin: create a DMA mapping. Implementation must not touch the bo's refco=
unt.
 * @unpin: destroy a DMA mapping. Implementation must not touch the bo's re=
fcount.
 */
```

**host1x_bo_pin() changes (bus.c):**

The new `host1x_bo_get(bo)` calls on both the cache-hit path and the new-ma=
pping path correctly take one user reference per pin:

```c
		if (mapping->bo =3D=3D bo && mapping->direction =3D=3D dir) {
			kref_get(&mapping->ref);
			host1x_bo_get(bo);
			goto unlock;
		}
```
```c
	host1x_bo_get(bo);

	spin_lock(&mapping->bo->lock);
```

The key insight is in the cache-store block =E2=80=94 the mapping refcount =
is bumped for the cache but the BO refcount is intentionally *not* bumped:

```c
		/*
		 * Bump the mapping reference count to track the mapping in the cache,
		 * but do not bump the BO's refcount. This allows the BO to still get fre=
ed,
		 * triggering the release of the cache mapping through
		 * host1x_bo_clear_cached_mappings.
		 */
		kref_get(&mapping->ref);
```

This is the core of the fix and is correct. The comment clearly explains th=
e weak-reference semantics.

**host1x_bo_unpin() changes:**

Saving `bo` before the `kref_put` is necessary since the put may free the m=
apping:

```c
	struct host1x_bo *bo =3D mapping->bo;
	...
	kref_put(&mapping->ref, __host1x_bo_unpin);
	...
	host1x_bo_put(bo);
```

This is correct. On a cache hit unpin, `kref_put` decrements 2=E2=86=921 (n=
o free), then `host1x_bo_put` drops the user's BO ref. On a non-cached unpi=
n, `kref_put` decrements 1=E2=86=920 triggering `__host1x_bo_unpin` (which =
frees the mapping), then `host1x_bo_put` drops the BO ref. The saved `bo` p=
ointer is needed for the latter case since `mapping` is freed.

**host1x_bo_clear_cached_mappings() (new function):**

```c
void host1x_bo_clear_cached_mappings(struct host1x_bo *bo)
{
	struct host1x_bo_mapping *mapping, *tmp;
	struct host1x_bo_cache *cache;

	list_for_each_entry_safe(mapping, tmp, &bo->mappings, list) {
		cache =3D mapping->cache;
		if (WARN_ON(!cache))
			continue;

		mutex_lock(&mapping->cache->lock);
		WARN_ON(kref_read(&mapping->ref) !=3D 1);
		__host1x_bo_unpin(&mapping->ref);
		mutex_unlock(&mapping->cache->lock);
	}
}
```

Two observations:

1. **Direct call to `__host1x_bo_unpin` bypassing `kref_put`**: The ref is =
expected to be 1 (validated by the WARN_ON), but `__host1x_bo_unpin` is cal=
led directly rather than through `kref_put(&mapping->ref, __host1x_bo_unpin=
)`. This means the kref is never decremented to 0 =E2=80=94 the mapping str=
uct (including its kref) is freed inside `ops->unpin()` with the kref still=
 at 1. This works because we're tearing down the object anyway, and is actu=
ally more defensive than `kref_put` for the abnormal case (if ref were unex=
pectedly >1, `kref_put` would just decrement without cleaning up, whereas t=
he direct call ensures cleanup always happens). It's unconventional but def=
ensible for a teardown path. A brief comment explaining the choice would he=
lp future readers.

2. **Locking**: The iteration over `bo->mappings` happens without holding `=
bo->lock`, while `__host1x_bo_unpin` internally acquires `bo->lock` for `li=
st_del`. This is safe because we're in the BO teardown path (refcount is 0,=
 no concurrent access), but worth noting. The `list_for_each_entry_safe` ha=
ndles the in-loop deletion correctly.

The `WARN_ON(!cache)` with `continue` is a good guard. The old code in `teg=
ra_bo_free_object` had the same logic (checked `mapping->cache`, logged err=
or for stale non-cached mappings). Changing from `dev_err` to `WARN_ON` is =
arguably an improvement since a stale non-cached mapping at free time indic=
ates a real bug.

**Replacement in tegra_bo_free_object:**

The old inline cleanup:
```c
	list_for_each_entry_safe(mapping, tmp, &bo->base.mappings, list) {
		if (mapping->cache)
			host1x_bo_unpin(mapping);
		else
			dev_err(gem->dev->dev, "mapping %p stale for device %s\n", ...);
	}
```

Is cleanly replaced with:
```c
	host1x_bo_clear_cached_mappings(&bo->base);
```

Good factoring =E2=80=94 this makes the cleanup callable from any host1x_bo=
 implementer, not just TegraDRM.

**Kdoc comments:** The added documentation for `host1x_bo_pin`, `host1x_bo_=
unpin`, `host1x_bo_ops`, and `host1x_bo_clear_cached_mappings` is clear and=
 accurate. Nice touch.

---
Generated by Claude Code Patch Reviewer