[PATCH 0/2] Fix leaking of cached Host1x buffer objects

[PATCH 0/2] Fix leaking of cached Host1x buffer objects

[Mikko Perttunen]

Host1x implements a mechanism to cache buffer object mappings to allow 
skipping costly map/unmap cycles for buffers that where that commonly 
happens (buffers used with display). The intention was that once the 
user frees the buffer, the cache mapping also goes away.

However, the cached mapping was also keeping a refcount on the buffer, 
so the code freeing the buffer -- and releasing the cached mapping -- 
would never run, hence leaking any buffer used with the cache.

Fix by making cache's reference to the buffer weak.

Merging notes:

The change to the crypto driver is safe to merge independently. The 
driver keeps its own refcount regardless so the buffer won't get freed 
incorrectly.

---
Mikko Perttunen (2):
      gpu: host1x: Allow entries in BO caches to be freed
      crypto: tegra - Don't touch bo refcount in host1x bo pin/unpin

 drivers/crypto/tegra/tegra-se-main.c |  3 +-
 drivers/gpu/drm/tegra/gem.c          | 13 ++------
 drivers/gpu/drm/tegra/submit.c       |  3 +-
 drivers/gpu/host1x/bus.c             | 60 +++++++++++++++++++++++++++++++++++-
 include/linux/host1x.h               |  7 +++++
 5 files changed, 70 insertions(+), 16 deletions(-)
---
base-commit: 028ef9c96e96197026887c0f092424679298aae8
change-id: 20260513-host1x-bocache-leak-4759384eb792

[PATCH 1/2] gpu: host1x: Allow entries in BO caches to be freed

[Mikko Perttunen]

When a buffer object is pinned via host1x_bo_pin() with a cache, the
resulting mapping is kept in the cache so it can be reused on subsequent
pins. Each mapping held a reference to the underlying host1x_bo (taken
in tegra_bo_pin / gather_bo_pin), so as long as a mapping was cached,
the bo itself could not be freed.

However, the only way to remove the cached mapping was through the free
path of the buffer object. This meant that if a bo got cached, it could
never get freed again.

Resolve the circularity by holding a weak reference to the bo from the
cache side. This is done by having the .pin callbacks not bump the bo's
refcount -- instead the common Host1x bo code does so, except for the
cache reference.

Also move the remove-cache-mapping-on-free code into a common function
inside Host1x code. This is only called from the TegraDRM GEM buffers
since those are the only ones that can be cached at the moment.

Reported-by: Aaron Kling <webgeek1234@gmail.com>
Fixes: 1f39b1dfa53c ("drm/tegra: Implement buffer object cache")
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/drm/tegra/gem.c    | 13 ++-------
 drivers/gpu/drm/tegra/submit.c |  3 +--
 drivers/gpu/host1x/bus.c       | 60 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/host1x.h         |  7 +++++
 4 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index d2bae88ad545..2377e2b76397 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -69,7 +69,7 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct device *dev, struct host1x_
 		return ERR_PTR(-ENOMEM);
 
 	kref_init(&map->ref);
-	map->bo = host1x_bo_get(bo);
+	map->bo = bo;
 	map->direction = direction;
 	map->dev = dev;
 
@@ -170,7 +170,6 @@ static void tegra_bo_unpin(struct host1x_bo_mapping *map)
 		kfree(map->sgt);
 	}
 
-	host1x_bo_put(map->bo);
 	kfree(map);
 }
 
@@ -509,17 +508,9 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 void tegra_bo_free_object(struct drm_gem_object *gem)
 {
 	struct tegra_drm *tegra = gem->dev->dev_private;
-	struct host1x_bo_mapping *mapping, *tmp;
 	struct tegra_bo *bo = to_tegra_bo(gem);
 
-	/* remove all mappings of this buffer object from any caches */
-	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", mapping,
-				dev_name(mapping->dev));
-	}
+	host1x_bo_clear_cached_mappings(&bo->base);
 
 	if (tegra->domain) {
 		tegra_bo_iommu_unmap(tegra, bo);
diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
index 3009b8b9e619..e5841857c937 100644
--- a/drivers/gpu/drm/tegra/submit.c
+++ b/drivers/gpu/drm/tegra/submit.c
@@ -76,7 +76,7 @@ gather_bo_pin(struct device *dev, struct host1x_bo *bo, enum dma_data_direction
 		return ERR_PTR(-ENOMEM);
 
 	kref_init(&map->ref);
-	map->bo = host1x_bo_get(bo);
+	map->bo = bo;
 	map->direction = direction;
 	map->dev = dev;
 
@@ -117,7 +117,6 @@ static void gather_bo_unpin(struct host1x_bo_mapping *map)
 	dma_unmap_sgtable(map->dev, map->sgt, map->direction, 0);
 	sg_free_table(map->sgt);
 	kfree(map->sgt);
-	host1x_bo_put(map->bo);
 
 	kfree(map);
 }
diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index f814eb4941c0..772e05a7b45b 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -887,6 +887,20 @@ int host1x_client_resume(struct host1x_client *client)
 }
 EXPORT_SYMBOL(host1x_client_resume);
 
+/**
+ * host1x_bo_pin() - Create a DMA mapping for the buffer object
+ * @dev: Device onto which DMA map to
+ * @bo: Buffer object to map
+ * @dir: DMA direction
+ * @cache: Cache in which to store mapping, or NULL
+ *
+ * Creates a DMA mapping pointing to @bo for @dev. The refcount of @bo is incremented
+ * until host1x_bo_unpin is called.
+ *
+ * If @cache is specified, the mapping is also stored in the cache and not released
+ * until @bo is freed (refcount drops to zero). This improves performance when a buffer
+ * is pinned and unpinned frequently as in the case of display use.
+ */
 struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo,
 					enum dma_data_direction dir,
 					struct host1x_bo_cache *cache)
@@ -899,6 +913,7 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
 		list_for_each_entry(mapping, &cache->mappings, entry) {
 			if (mapping->bo == bo && mapping->direction == dir) {
 				kref_get(&mapping->ref);
+				host1x_bo_get(bo);
 				goto unlock;
 			}
 		}
@@ -908,6 +923,8 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
 	if (IS_ERR(mapping))
 		goto unlock;
 
+	host1x_bo_get(bo);
+
 	spin_lock(&mapping->bo->lock);
 	list_add_tail(&mapping->list, &bo->mappings);
 	spin_unlock(&mapping->bo->lock);
@@ -918,7 +935,12 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
 
 		list_add_tail(&mapping->entry, &cache->mappings);
 
-		/* bump reference count to track the copy in the cache */
+		/*
+		 * 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);
 	}
 
@@ -948,9 +970,17 @@ static void __host1x_bo_unpin(struct kref *ref)
 	mapping->bo->ops->unpin(mapping);
 }
 
+/**
+ * host1x_bo_unpin() - Release an established DMA mapping of a buffer object
+ * @mapping: Mapping to release
+ *
+ * Unmaps the given @mapping, unless it is cached. Decreases the refcount on
+ * the underlying buffer object.
+ */
 void host1x_bo_unpin(struct host1x_bo_mapping *mapping)
 {
 	struct host1x_bo_cache *cache = mapping->cache;
+	struct host1x_bo *bo = mapping->bo;
 
 	if (cache)
 		mutex_lock(&cache->lock);
@@ -959,5 +989,33 @@ void host1x_bo_unpin(struct host1x_bo_mapping *mapping)
 
 	if (cache)
 		mutex_unlock(&cache->lock);
+
+	host1x_bo_put(bo);
 }
 EXPORT_SYMBOL(host1x_bo_unpin);
+
+/**
+ * host1x_bo_clear_cached_mappings() - Remove all cached mappings pointing at a bo
+ * @bo: Buffer object to release mappings of
+ *
+ * Drops references to any mappings pointing to @bo left in any caches. This must
+ * be called by any host1x_bo implementers that may be pinned with caching enabled
+ * before freeing the bo.
+ */
+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);
+	}
+}
+EXPORT_SYMBOL(host1x_bo_clear_cached_mappings);
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 5e7a63143a4a..d8f052a85b75 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -143,6 +143,12 @@ static inline struct host1x_bo_mapping *to_host1x_bo_mapping(struct kref *ref)
 	return container_of(ref, struct host1x_bo_mapping, ref);
 }
 
+/**
+ * struct host1x_bo_ops - operations implemented by a host1x_bo provider
+ *
+ * @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.
+ */
 struct host1x_bo_ops {
 	struct host1x_bo *(*get)(struct host1x_bo *bo);
 	void (*put)(struct host1x_bo *bo);
@@ -181,6 +187,7 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
 					enum dma_data_direction dir,
 					struct host1x_bo_cache *cache);
 void host1x_bo_unpin(struct host1x_bo_mapping *map);
+void host1x_bo_clear_cached_mappings(struct host1x_bo *bo);
 
 static inline void *host1x_bo_mmap(struct host1x_bo *bo)
 {

-- 
2.53.0

[PATCH 2/2] crypto: tegra - Don't touch bo refcount in host1x bo pin/unpin

[Mikko Perttunen]

Since commit "gpu: host1x: Allow entries in BO caches to be freed",
host1x_bo_pin() and host1x_bo_unpin() handle the bo's refcount
themselves. .pin/.unpin callbacks should not adjust it.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/crypto/tegra/tegra-se-main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/tegra/tegra-se-main.c b/drivers/crypto/tegra/tegra-se-main.c
index eb71113ed146..d7541a9c0a92 100644
--- a/drivers/crypto/tegra/tegra-se-main.c
+++ b/drivers/crypto/tegra/tegra-se-main.c
@@ -52,7 +52,7 @@ tegra_se_cmdbuf_pin(struct device *dev, struct host1x_bo *bo, enum dma_data_dire
 		return ERR_PTR(-ENOMEM);
 
 	kref_init(&map->ref);
-	map->bo = host1x_bo_get(bo);
+	map->bo = bo;
 	map->direction = direction;
 	map->dev = dev;
 
@@ -93,7 +93,6 @@ static void tegra_se_cmdbuf_unpin(struct host1x_bo_mapping *map)
 	dma_unmap_sgtable(map->dev, map->sgt, map->direction, 0);
 	sg_free_table(map->sgt);
 	kfree(map->sgt);
-	host1x_bo_put(map->bo);
 
 	kfree(map);
 }

-- 
2.53.0

Claude review: Fix leaking of cached Host1x buffer objects

[Claude Code Review Bot]

Overall Series Review

Subject: Fix leaking of cached Host1x buffer objects
Author: Mikko Perttunen <mperttunen@nvidia.com>
Patches: 3
Reviewed: 2026-05-16T10:12:28.032839

---

This is a well-reasoned 2-patch series from Mikko Perttunen fixing a real memory leak in the Host1x buffer object cache. The core problem is a classic circular reference: cached mappings hold a refcount on the BO, but the only way to release a cached mapping is through the BO's free path — which never runs because the cache keeps the BO alive.

The fix is clean: centralize BO refcounting in the common `host1x_bo_pin()`/`host1x_bo_unpin()` functions rather than in each driver's `.pin`/`.unpin` callbacks, and make the cache's reference to the BO "weak" (no refcount bump). The BO free path is then responsible for tearing down cached mappings via a new `host1x_bo_clear_cached_mappings()` helper.

The series is correct and the approach is sound. I have one concern about the direct invocation of `__host1x_bo_unpin` bypassing `kref_put` in the teardown path, and a minor locking observation, but nothing blocking.

**Verdict: Looks good overall. Two minor observations below, neither blocking.**

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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

Claude review: crypto: tegra - Don't touch bo refcount in host1x bo pin/unpin

[Claude Code Review Bot]

Patch Review

Straightforward follow-up aligning the crypto driver with the new contract:

```c
-	map->bo = host1x_bo_get(bo);
+	map->bo = bo;
```

```c
-	host1x_bo_put(map->bo);
```

The crypto driver (tegra-se) never uses caching (the `host1x_bo_pin` calls pass `NULL` for cache), so no `host1x_bo_clear_cached_mappings` call is needed. The cmdbuf manages its own lifetime via `cmdbuf->ref`, independent of the host1x_bo refcount.

The cover letter's claim that this is safe to merge independently is correct: without Patch 1, the old `host1x_bo_pin`/`host1x_bo_unpin` don't touch the BO refcount either (it was all in the callbacks), so removing the ref manipulation from both pin and unpin is a no-op from a lifecycle perspective — the crypto driver's own kref on the cmdbuf ensures the BO outlives the mapping.

**No issues with this patch.**

---
Generated by Claude Code Patch Reviewer