[PATCH] drm/msm: avoid an unmatched fbdev IOVA pin

[PATCH] drm/msm: avoid an unmatched fbdev IOVA pin

[Michael McAllister]

Unbinding the display on the Moto G10 produces a pin-count warning when
the console framebuffer is freed. The fbdev probe takes an IOVA pin,
but normal teardown never drops it.

Use msm_gem_get_iova() to reserve the address without taking that extra
pin. Scanout already pins the buffer while it is in use, and the CPU
mapping holds its own pin. Drop the matching unpin from the error path.

Fixes: 7ad0e8cf6317 ("drm/msm: Count how many times iova memory is pinned")
Signed-off-by: Michael McAllister <me@michaelmcallister.org>
---
Hi,

I found another pin-count warning while testing display teardown on the
Moto G10. This one is in fbdev: it takes a pin when creating the console
framebuffer, but doesn't drop it when the framebuffer is freed.

I tested this alongside the other display fixes on 7.2.9, including
unbind/rebind with the compositor stopped. It is independent of the DSI
command-buffer fix I sent earlier.

Thanks,
Michael

 drivers/gpu/drm/msm/msm_fbdev.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index dd6d6c507d..c3bec6719c 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -137,12 +137,8 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 		goto err_drm_gem_handle_delete;
 	}
 
-	/*
-	 * NOTE: if we can be guaranteed to be able to map buffer
-	 * in panic (ie. lock-safe, etc) we could avoid pinning the
-	 * buffer now:
-	 */
-	ret = msm_gem_get_and_pin_iova(bo, priv->kms->vm, &paddr);
+	/* Scanout pins the buffer; only reserve its address here. */
+	ret = msm_gem_get_iova(bo, priv->kms->vm, &paddr);
 	if (ret) {
 		drm_err(dev, "failed to get buffer obj iova: %d\n", ret);
 		goto err_drm_client_buffer_delete;
@@ -162,7 +158,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 	fbi->screen_buffer = msm_gem_get_vaddr(bo);
 	if (IS_ERR(fbi->screen_buffer)) {
 		ret = PTR_ERR(fbi->screen_buffer);
-		goto err_msm_gem_unpin;
+		goto err_msm_gem_vma_put;
 	}
 	fbi->screen_size = bo->size;
 	fbi->fix.smem_start = paddr;
@@ -179,8 +175,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 
 	return 0;
 
-err_msm_gem_unpin:
-	msm_gem_unpin_iova(bo, priv->kms->vm);
+err_msm_gem_vma_put:
 	msm_gem_vma_put(bo);
 err_drm_client_buffer_delete:
 	drm_client_buffer_delete(buffer);

base-commit: 1e59855738454b14021b64115f0090eafc7526ae
-- 
2.39.5

Re: [PATCH] drm/msm: avoid an unmatched fbdev IOVA pin

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/msm: VMA close and vma_ref underflow in
  msm_fbdev_driver_fbdev_probe()
--

commit a1a08825c16a09f8d3eedf5784d21f5c50d25613
Author: Michael McAllister <me@michaelmcallister.org>

drm/msm: avoid an unmatched fbdev IOVA pin

This commit replaces msm_gem_get_and_pin_iova() with msm_gem_get_iova() in
msm_fbdev_driver_fbdev_probe() and removes the corresponding
msm_gem_unpin_iova() call from the error path.

> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index dd6d6c507d77e..c3bec6719ca04 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -137,12 +137,8 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  		goto err_drm_gem_handle_delete;
>  	}
>  
> -	/*
> -	 * NOTE: if we can be guaranteed to be able to map buffer
> -	 * in panic (ie. lock-safe, etc) we could avoid pinning the
> -	 * buffer now:
> -	 */
> -	ret = msm_gem_get_and_pin_iova(bo, priv->kms->vm, &paddr);
> +	/* Scanout pins the buffer; only reserve its address here. */
> +	ret = msm_gem_get_iova(bo, priv->kms->vm, &paddr);
>  	if (ret) {
>  		drm_err(dev, "failed to get buffer obj iova: %d\n", ret);
>  		goto err_drm_client_buffer_delete;
> @@ -162,7 +158,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  	fbi->screen_buffer = msm_gem_get_vaddr(bo);
>  	if (IS_ERR(fbi->screen_buffer)) {
>  		ret = PTR_ERR(fbi->screen_buffer);
> -		goto err_msm_gem_unpin;
> +		goto err_msm_gem_vma_put;
>  	}
>  	fbi->screen_size = bo->size;
>  	fbi->fix.smem_start = paddr;
> @@ -179,8 +175,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  
>  	return 0;
>  
> -err_msm_gem_unpin:
> -	msm_gem_unpin_iova(bo, priv->kms->vm);
> +err_msm_gem_vma_put:
>  	msm_gem_vma_put(bo);
>  err_drm_client_buffer_delete:
>  	drm_client_buffer_delete(buffer);

[Severity: Medium]
Does calling msm_gem_get_iova() without a matching msm_gem_vma_get() cause
the IOVA reservation to be closed immediately and underflow vma_ref on the
error path?

drm_gem_handle_create() takes the only vma_ref on bo via msm_gem_open(),
and neither drm_client_buffer_create() nor msm_gem_get_iova() increments
vma_ref.  On the normal path, drm_gem_handle_delete() at the end of
msm_fbdev_driver_fbdev_probe() calls msm_gem_close() -> msm_gem_vma_put(),
which drops vma_ref to 0 and calls put_iova_spaces(..., true, "vma_put") to
close the VMA and release the IOVA range stored in fbi->fix.smem_start.

On the error path when msm_gem_get_vaddr() fails, err_msm_gem_vma_put calls
msm_gem_vma_put(bo) and then falls through to drm_gem_handle_delete(),
which calls msm_gem_vma_put(bo) a second time and underflows vma_ref to -1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179424371125.96483.2462884435294600442.msm-fbdev-pin-leak@michaelmcallister.org?part=1

[PATCH v2] drm/msm: avoid an unmatched fbdev IOVA pin

[Michael McAllister]

Unbinding the display on the Moto G10 produces a pin-count warning when
the console framebuffer is freed. The fbdev probe takes an IOVA pin,
but normal teardown never drops it.

Reserve the address with msm_gem_get_iova() instead. Scanout pins the
buffer while it is in use, and the CPU mapping holds its own pin.

Keep a VMA reference for fbdev so deleting the temporary GEM handle does
not release the reserved address. Drop that reference on probe failure
or when the framebuffer is destroyed. This also balances the existing
error-path VMA put, which otherwise drops the handle's reference twice.

Get the CPU mapping before publishing the framebuffer in the helper, so
a mapping failure does not leave helper->fb and helper->buffer pointing
to freed memory.

Fixes: 7ad0e8cf6317 ("drm/msm: Count how many times iova memory is pinned")
Signed-off-by: Michael McAllister <me@michaelmcallister.org>
---
Hi,

Changes in v2:
- Keep the fbdev VMA reference until teardown and balance the error paths.
- Get the CPU mapping before publishing the helper pointers.

Thanks,
Michael

 drivers/gpu/drm/msm/msm_fbdev.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index dd6d6c507d..ed599884bf 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -48,6 +48,7 @@ static void msm_fbdev_fb_destroy(struct fb_info *info)
 	drm_fb_helper_fini(helper);
 
 	msm_gem_put_vaddr(bo);
+	msm_gem_vma_put(bo);
 
 	drm_client_buffer_delete(helper->buffer);
 	drm_client_release(&helper->client);
@@ -137,15 +138,18 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 		goto err_drm_gem_handle_delete;
 	}
 
-	/*
-	 * NOTE: if we can be guaranteed to be able to map buffer
-	 * in panic (ie. lock-safe, etc) we could avoid pinning the
-	 * buffer now:
-	 */
-	ret = msm_gem_get_and_pin_iova(bo, priv->kms->vm, &paddr);
+	/* Keep the IOVA reserved after deleting the temporary GEM handle. */
+	msm_gem_vma_get(bo);
+	ret = msm_gem_get_iova(bo, priv->kms->vm, &paddr);
 	if (ret) {
 		drm_err(dev, "failed to get buffer obj iova: %d\n", ret);
-		goto err_drm_client_buffer_delete;
+		goto err_msm_gem_vma_put;
+	}
+
+	fbi->screen_buffer = msm_gem_get_vaddr(bo);
+	if (IS_ERR(fbi->screen_buffer)) {
+		ret = PTR_ERR(fbi->screen_buffer);
+		goto err_msm_gem_vma_put;
 	}
 
 	DBG("fbi=%p, dev=%p", fbi, dev);
@@ -159,11 +163,6 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 
 	drm_fb_helper_fill_info(fbi, helper, sizes);
 
-	fbi->screen_buffer = msm_gem_get_vaddr(bo);
-	if (IS_ERR(fbi->screen_buffer)) {
-		ret = PTR_ERR(fbi->screen_buffer);
-		goto err_msm_gem_unpin;
-	}
 	fbi->screen_size = bo->size;
 	fbi->fix.smem_start = paddr;
 	fbi->fix.smem_len = bo->size;
@@ -179,10 +178,8 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 
 	return 0;
 
-err_msm_gem_unpin:
-	msm_gem_unpin_iova(bo, priv->kms->vm);
+err_msm_gem_vma_put:
 	msm_gem_vma_put(bo);
-err_drm_client_buffer_delete:
 	drm_client_buffer_delete(buffer);
 err_drm_gem_handle_delete:
 	drm_gem_handle_delete(file, handle);

base-commit: 1e59855738454b14021b64115f0090eafc7526ae
-- 
2.39.5

Re: [PATCH] drm/msm: avoid an unmatched fbdev IOVA pin

[Dmitry Baryshkov]

On Mon, Nov 09, 2026 at 05:01:51PM +0000, Michael McAllister wrote:
> Unbinding the display on the Moto G10 produces a pin-count warning when
> the console framebuffer is freed. The fbdev probe takes an IOVA pin,
> but normal teardown never drops it.
> 

I'd say, it suggests that the teardown path must be fixed instead.

> Use msm_gem_get_iova() to reserve the address without taking that extra
> pin. Scanout already pins the buffer while it is in use, and the CPU
> mapping holds its own pin. Drop the matching unpin from the error path.
> 
> Fixes: 7ad0e8cf6317 ("drm/msm: Count how many times iova memory is pinned")
> Signed-off-by: Michael McAllister <me@michaelmcallister.org>
> ---
> Hi,
> 
> I found another pin-count warning while testing display teardown on the
> Moto G10. This one is in fbdev: it takes a pin when creating the console
> framebuffer, but doesn't drop it when the framebuffer is freed.
> 
> I tested this alongside the other display fixes on 7.2.9, including
> unbind/rebind with the compositor stopped. It is independent of the DSI
> command-buffer fix I sent earlier.
> 
> Thanks,
> Michael
> 
>  drivers/gpu/drm/msm/msm_fbdev.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index dd6d6c507d..c3bec6719c 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -137,12 +137,8 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  		goto err_drm_gem_handle_delete;
>  	}
>  
> -	/*
> -	 * NOTE: if we can be guaranteed to be able to map buffer
> -	 * in panic (ie. lock-safe, etc) we could avoid pinning the
> -	 * buffer now:
> -	 */
> -	ret = msm_gem_get_and_pin_iova(bo, priv->kms->vm, &paddr);
> +	/* Scanout pins the buffer; only reserve its address here. */
> +	ret = msm_gem_get_iova(bo, priv->kms->vm, &paddr);
>  	if (ret) {
>  		drm_err(dev, "failed to get buffer obj iova: %d\n", ret);
>  		goto err_drm_client_buffer_delete;
> @@ -162,7 +158,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  	fbi->screen_buffer = msm_gem_get_vaddr(bo);
>  	if (IS_ERR(fbi->screen_buffer)) {
>  		ret = PTR_ERR(fbi->screen_buffer);
> -		goto err_msm_gem_unpin;
> +		goto err_msm_gem_vma_put;
>  	}
>  	fbi->screen_size = bo->size;
>  	fbi->fix.smem_start = paddr;
> @@ -179,8 +175,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  
>  	return 0;
>  
> -err_msm_gem_unpin:
> -	msm_gem_unpin_iova(bo, priv->kms->vm);
> +err_msm_gem_vma_put:
>  	msm_gem_vma_put(bo);
>  err_drm_client_buffer_delete:
>  	drm_client_buffer_delete(buffer);
> 
> base-commit: 1e59855738454b14021b64115f0090eafc7526ae
> -- 
> 2.39.5
> 

-- 
With best wishes
Dmitry