Claude review: gpu: host1x: Allow entries in BO caches to be freed

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

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. The new contract documented in the `host1x_bo_ops` comment makes the expectation clear:

```c
/**
 * @pin: create a DMA mapping. Implementation must not touch the bo's refcount.
 * @unpin: destroy a DMA mapping. Implementation must not touch the bo's refcount.
 */
```

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

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

```c
		if (mapping->bo == bo && mapping->direction == 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 — 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 freed,
		 * 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 the weak-reference semantics.

**host1x_bo_unpin() changes:**

Saving `bo` before the `kref_put` is necessary since the put may free the mapping:

```c
	struct host1x_bo *bo = 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→1 (no free), then `host1x_bo_put` drops the user's BO ref. On a non-cached unpin, `kref_put` decrements 1→0 triggering `__host1x_bo_unpin` (which frees the mapping), then `host1x_bo_put` drops the BO ref. The saved `bo` pointer 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 = mapping->cache;
		if (WARN_ON(!cache))
			continue;

		mutex_lock(&mapping->cache->lock);
		WARN_ON(kref_read(&mapping->ref) != 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 called directly rather than through `kref_put(&mapping->ref, __host1x_bo_unpin)`. This means the kref is never decremented to 0 — the mapping struct (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 actually more defensive than `kref_put` for the abnormal case (if ref were unexpectedly >1, `kref_put` would just decrement without cleaning up, whereas the direct call ensures cleanup always happens). It's unconventional but defensible for a teardown path. A brief comment explaining the choice would help future readers.

2. **Locking**: The iteration over `bo->mappings` happens without holding `bo->lock`, while `__host1x_bo_unpin` internally acquires `bo->lock` for `list_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` handles the in-loop deletion correctly.

The `WARN_ON(!cache)` with `continue` is a good guard. The old code in `tegra_bo_free_object` had the same logic (checked `mapping->cache`, logged error 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 indicates 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 — 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