[PATCH 0/3] virtgpu: fix memory leak on device removal

[PATCH 0/3] virtgpu: fix memory leak on device removal

[Manos Pitsidianakis]

When a VIRTIO GPU device is removed, it cleans up any command buffers
that the VIRTIO frontend has responded to. It however ignores commands
that have yet to be replied to, which still reside in the avail rings of
the virt queues. This leaks two type of objects:

- VIRTIO command buffers
- Fences

Furthermore, if the virtio config has num_capsets > 0, the capsets field
of the device is also leaked.

These memory leaks are reported by:

- /sys/kernel/debug/kmemleak
- slab debug options "BUG virtio-gpu-vbufs: Objects remaining in
  virtio-gpu-vbufs on __kmem_cache_shutdown()"
- drm:drm_mm_takedown "Memory manager not clean during takedown."

This patch series adds cleanup logic in virtio_gpu_deinit(), after
calling virtio_reset_device(), to free any such allocations.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
Manos Pitsidianakis (3):
      virtgpu: add virtio_gpu_queue_cleanup()
      virtgpu: add virtio_gpu_fence_cleanup()
      virtgpu: deallocate capsets on device deinit

 drivers/gpu/drm/virtio/virtgpu_drv.h   |  2 ++
 drivers/gpu/drm/virtio/virtgpu_fence.c | 12 ++++++++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  6 ++++
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 55 ++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+)
---
base-commit: ad10b82c2bcac7f87ac6eaecfca33378b43425ee
change-id: 20250505-virtgpu-queue-cleanup-v1-3392995cab5f

--
γαῖα πυρί μιχθήτω

[PATCH 1/3] virtgpu: add virtio_gpu_queue_cleanup()

[Manos Pitsidianakis]

When virtio_gpu_remove() is called, the queues are flushed and used
buffers from the virtqueues are freed. However, the VIRTIO device might
have left unused buffers in the avail rings, resulting in memory leaks.
KASAN, slab debug and drm_mm_takedown all report the errors:

 BUG virtio-gpu-vbufs: Objects remaining in virtio-gpu-vbufs on
 __kmem_cache_shutdown()
 <- Snipped backtrace ->
 Object 0xffffff801b07c008 @offset=8
 Allocated in virtio_gpu_get_vbuf.isra.0+0x38/0xb0 age=4314 cpu=3
 pid=540
  kmem_cache_alloc+0x330/0x3a8
  virtio_gpu_get_vbuf.isra.0+0x38/0xb0
  virtio_gpu_cmd_resource_create_3d+0x60/0x1f0
  virtio_gpu_object_create+0x388/0x468
  virtio_gpu_resource_create_ioctl+0x1f0/0x420
  drm_ioctl_kernel+0x170/0x248
  drm_ioctl+0x33c/0x680
  __arm64_sys_ioctl+0xdc/0x128
  invoke_syscall+0x84/0x1c8
  el0_svc_common.constprop.0+0x11c/0x150
  do_el0_svc+0x38/0x50
  el0_svc+0x38/0x70
  el0t_64_sync_handler+0x120/0x130
  el0t_64_sync+0x190/0x198

 ------------[ cut here ]------------
 kmem_cache_destroy virtio-gpu-vbufs: Slab cache still has objects when
 called from virtio_gpu_free_vbufs+0x48/0x70
 WARNING: CPU: 0 PID: 483 at mm/slab_common.c:498
 kmem_cache_destroy+0x114/0x178
 <- Snipped info ->

 ------------[ cut here ]------------
 Memory manager not clean during takedown.
 <- Snipped info ->
 ---[ end trace 0000000000000000 ]---
 [drm:drm_mm_takedown] *ERROR* node [001000eb + 00000080]: inserted at
  drm_mm_insert_node_in_range+0x48c/0x6a8
  drm_vma_offset_add+0x84/0xb0
  drm_gem_create_mmap_offset+0x50/0x70
  __drm_gem_shmem_create+0x94/0x1d8
  drm_gem_shmem_create+0x1c/0x30
  virtio_gpu_object_create+0x68/0x468
  virtio_gpu_resource_create_ioctl+0x1f0/0x420
  drm_ioctl_kernel+0x170/0x248
  drm_ioctl+0x33c/0x680
  __arm64_sys_ioctl+0xdc/0x128
  invoke_syscall+0x84/0x1c8
  el0_svc_common.constprop.0+0x11c/0x150
  do_el0_svc+0x38/0x50
  el0_svc+0x38/0x70
  el0t_64_sync_handler+0x120/0x130
  el0t_64_sync+0x190/0x198
 [drm:drm_mm_takedown] *ERROR* node [0010016b + 000000eb]: inserted at
 <- Snipped info ->

The leaked objects are also reported in /sys/kernel/debug/kmemleak.

This commit adds a cleanup function that is called from
virtio_gpu_deinit().

The function cleans up any unused buffers from the virtqueues and calls
the appropriate freeing functions. This is safe to do so because
virtio_gpu_deinit() calls virtio_reset_device() before calling the
cleanup function, ensuring no one is going to read from the virtqueues.

The cleanup function checks for used buffers on the queues, and
additionally calls virtqueue_detach_unused_buf on each queue to get any
buffers that did not have time to be processed by the VIRTIO backend.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h |  1 +
 drivers/gpu/drm/virtio/virtgpu_kms.c |  1 +
 drivers/gpu/drm/virtio/virtgpu_vq.c  | 55 ++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index f17660a71a3e7a22b5d4fefa6b754c227a294037..b3d367be6f204dbc98bf1c6e5c43a37ac8c0d8b3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -419,6 +419,7 @@ void virtio_gpu_cursor_ack(struct virtqueue *vq);
 void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
 void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
 void virtio_gpu_panic_notify(struct virtio_gpu_device *vgdev);
+void virtio_gpu_queue_cleanup(struct virtio_gpu_device *vgdev);
 void virtio_gpu_notify(struct virtio_gpu_device *vgdev);
 
 int
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 7dfb2006c561ca13b15d979ddb8bf2d753e35dad..da70d9248072b64786a5d48b71bccaa80b8aae8f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -286,6 +286,7 @@ void virtio_gpu_deinit(struct drm_device *dev)
 	flush_work(&vgdev->cursorq.dequeue_work);
 	flush_work(&vgdev->config_changed_work);
 	virtio_reset_device(vgdev->vdev);
+	virtio_gpu_queue_cleanup(vgdev);
 	vgdev->vdev->config->del_vqs(vgdev->vdev);
 }
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 55a15e247dd1ad53a2b43b19fca8879b956f0e1a..fd150827e413cedcec4d82b0da8d792cb67e243f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -299,6 +299,61 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
 	wake_up(&vgdev->cursorq.ack_queue);
 }
 
+/* deallocate all in-flight virtqueue elements */
+void virtio_gpu_queue_cleanup(struct virtio_gpu_device *vgdev)
+{
+	struct list_head reclaim_list;
+	struct virtio_gpu_vbuffer *entry, *tmp;
+
+	INIT_LIST_HEAD(&reclaim_list);
+	spin_lock(&vgdev->ctrlq.qlock);
+	do {
+		virtqueue_disable_cb(vgdev->ctrlq.vq);
+		reclaim_vbufs(vgdev->ctrlq.vq, &reclaim_list);
+	} while (!virtqueue_enable_cb(vgdev->ctrlq.vq));
+	/* detach unused buffers */
+	while ((entry = virtqueue_detach_unused_buf(vgdev->ctrlq.vq)) != NULL) {
+		if (entry->resp_cb)
+			entry->resp_cb(vgdev, entry);
+		if (entry->objs)
+			virtio_gpu_array_put_free(entry->objs);
+		free_vbuf(vgdev, entry);
+	}
+	spin_unlock(&vgdev->ctrlq.qlock);
+
+	list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
+		if (entry->resp_cb)
+			entry->resp_cb(vgdev, entry);
+		if (entry->objs)
+			virtio_gpu_array_put_free(entry->objs);
+		list_del(&entry->list);
+		free_vbuf(vgdev, entry);
+	}
+
+	spin_lock(&vgdev->cursorq.qlock);
+	do {
+		virtqueue_disable_cb(vgdev->cursorq.vq);
+		reclaim_vbufs(vgdev->cursorq.vq, &reclaim_list);
+	} while (!virtqueue_enable_cb(vgdev->cursorq.vq));
+	spin_unlock(&vgdev->cursorq.qlock);
+	while ((entry = virtqueue_detach_unused_buf(vgdev->cursorq.vq)) != NULL) {
+		if (entry->resp_cb)
+			entry->resp_cb(vgdev, entry);
+		if (entry->objs)
+			virtio_gpu_array_put_free(entry->objs);
+		free_vbuf(vgdev, entry);
+	}
+
+	list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
+		if (entry->resp_cb)
+			entry->resp_cb(vgdev, entry);
+		if (entry->objs)
+			virtio_gpu_array_put_free(entry->objs);
+		list_del(&entry->list);
+		free_vbuf(vgdev, entry);
+	}
+}
+
 /* Create sg_table from a vmalloc'd buffer. */
 static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size, int *sg_ents)
 {

-- 
2.47.2

[PATCH 2/3] virtgpu: add virtio_gpu_fence_cleanup()

[Manos Pitsidianakis]

When virtio_gpu_remove() is called, there might be in-flight command
objects in the virtqueues that the VIRTIO device hasn't processed. These
commands might use fences, which end up being leaked, as reported by
/sys/kernel/debug/kmemleak.

This commit adds a cleanup function that lowers the reference count of
all in-flight fences, resulting in their de-allocation.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  1 +
 drivers/gpu/drm/virtio/virtgpu_fence.c | 12 ++++++++++++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  1 +
 3 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index b3d367be6f204dbc98bf1c6e5c43a37ac8c0d8b3..c94b5edb2aec42fe5cd6416e243cf40e4e2b060f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -465,6 +465,7 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 			  struct virtio_gpu_fence *fence);
 void virtio_gpu_fence_event_process(struct virtio_gpu_device *vdev,
 				    u64 fence_id);
+void virtio_gpu_fence_cleanup(struct virtio_gpu_device *vdev);
 
 /* virtgpu_object.c */
 void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo);
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 44c1d8ef3c4d07881e2c4c92cc67f6aba7a5df4f..3e536d190c0464f4db8955605bbf0aa4aa3612bd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -157,3 +157,15 @@ void virtio_gpu_fence_event_process(struct virtio_gpu_device *vgdev,
 	}
 	spin_unlock_irqrestore(&drv->lock, irq_flags);
 }
+
+void virtio_gpu_fence_cleanup(struct virtio_gpu_device *vgdev)
+{
+	struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
+	struct virtio_gpu_fence *curr, *tmp;
+
+	list_for_each_entry_safe(curr, tmp, &drv->fences, node) {
+		dma_fence_signal_locked(&curr->f);
+		list_del(&curr->node);
+		dma_fence_put(&curr->f);
+	}
+}
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index da70d9248072b64786a5d48b71bccaa80b8aae8f..7b3c4d314f8eee692e2842a7056d6dc64936fc2f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -286,6 +286,7 @@ void virtio_gpu_deinit(struct drm_device *dev)
 	flush_work(&vgdev->cursorq.dequeue_work);
 	flush_work(&vgdev->config_changed_work);
 	virtio_reset_device(vgdev->vdev);
+	virtio_gpu_fence_cleanup(vgdev);
 	virtio_gpu_queue_cleanup(vgdev);
 	vgdev->vdev->config->del_vqs(vgdev->vdev);
 }

-- 
2.47.2

[PATCH 3/3] virtgpu: deallocate capsets on device deinit

[Manos Pitsidianakis]

virtio_gpu_device's capsets field is allocated with the DRM memory
allocator but never freed. Add the appropriate freeing call inside
virtio_gpu_deinit.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 drivers/gpu/drm/virtio/virtgpu_kms.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 7b3c4d314f8eee692e2842a7056d6dc64936fc2f..a8b751179332b9ec2fbba1392a6ee0e638a5192e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -286,6 +286,10 @@ void virtio_gpu_deinit(struct drm_device *dev)
 	flush_work(&vgdev->cursorq.dequeue_work);
 	flush_work(&vgdev->config_changed_work);
 	virtio_reset_device(vgdev->vdev);
+	spin_lock(&vgdev->display_info_lock);
+	drmm_kfree(dev, vgdev->capsets);
+	vgdev->capsets = NULL;
+	spin_unlock(&vgdev->display_info_lock);
 	virtio_gpu_fence_cleanup(vgdev);
 	virtio_gpu_queue_cleanup(vgdev);
 	vgdev->vdev->config->del_vqs(vgdev->vdev);

-- 
2.47.2

Re: [PATCH 3/3] virtgpu: deallocate capsets on device deinit

[Dmitry Osipenko]

On 5/5/25 11:59, Manos Pitsidianakis wrote:
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 7b3c4d314f8eee692e2842a7056d6dc64936fc2f..a8b751179332b9ec2fbba1392a6ee0e638a5192e 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -286,6 +286,10 @@ void virtio_gpu_deinit(struct drm_device *dev)
>  	flush_work(&vgdev->cursorq.dequeue_work);
>  	flush_work(&vgdev->config_changed_work);
>  	virtio_reset_device(vgdev->vdev);
> +	spin_lock(&vgdev->display_info_lock);
> +	drmm_kfree(dev, vgdev->capsets);
> +	vgdev->capsets = NULL;
> +	spin_unlock(&vgdev->display_info_lock);

Isn't this lock superfluous?

-- 
Best regards,
Dmitry

Re: [PATCH 3/3] virtgpu: deallocate capsets on device deinit

[Dmitry Osipenko]

On 5/5/25 18:58, Dmitry Osipenko wrote:
> On 5/5/25 11:59, Manos Pitsidianakis wrote:
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
>> index 7b3c4d314f8eee692e2842a7056d6dc64936fc2f..a8b751179332b9ec2fbba1392a6ee0e638a5192e 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
>> @@ -286,6 +286,10 @@ void virtio_gpu_deinit(struct drm_device *dev)
>>  	flush_work(&vgdev->cursorq.dequeue_work);
>>  	flush_work(&vgdev->config_changed_work);
>>  	virtio_reset_device(vgdev->vdev);
>> +	spin_lock(&vgdev->display_info_lock);
>> +	drmm_kfree(dev, vgdev->capsets);
>> +	vgdev->capsets = NULL;
>> +	spin_unlock(&vgdev->display_info_lock);
> 
> Isn't this lock superfluous?

Wait a minute, vgdev->capsets is allocated using drmm, hence it's
auto-freed when DRM device is freed. This patch shouldn't be needed.

-- 
Best regards,
Dmitry

Re: [PATCH 1/3] virtgpu: add virtio_gpu_queue_cleanup()

[Stefano Garzarella]

On Mon, May 05, 2025 at 11:59:14AM +0300, Manos Pitsidianakis wrote:
>When virtio_gpu_remove() is called, the queues are flushed and used
>buffers from the virtqueues are freed. However, the VIRTIO device might
>have left unused buffers in the avail rings, resulting in memory leaks.
>KASAN, slab debug and drm_mm_takedown all report the errors:
>
> BUG virtio-gpu-vbufs: Objects remaining in virtio-gpu-vbufs on
> __kmem_cache_shutdown()
> <- Snipped backtrace ->
> Object 0xffffff801b07c008 @offset=8
> Allocated in virtio_gpu_get_vbuf.isra.0+0x38/0xb0 age=4314 cpu=3
> pid=540
>  kmem_cache_alloc+0x330/0x3a8
>  virtio_gpu_get_vbuf.isra.0+0x38/0xb0
>  virtio_gpu_cmd_resource_create_3d+0x60/0x1f0
>  virtio_gpu_object_create+0x388/0x468
>  virtio_gpu_resource_create_ioctl+0x1f0/0x420
>  drm_ioctl_kernel+0x170/0x248
>  drm_ioctl+0x33c/0x680
>  __arm64_sys_ioctl+0xdc/0x128
>  invoke_syscall+0x84/0x1c8
>  el0_svc_common.constprop.0+0x11c/0x150
>  do_el0_svc+0x38/0x50
>  el0_svc+0x38/0x70
>  el0t_64_sync_handler+0x120/0x130
>  el0t_64_sync+0x190/0x198
>
> ------------[ cut here ]------------
> kmem_cache_destroy virtio-gpu-vbufs: Slab cache still has objects when
> called from virtio_gpu_free_vbufs+0x48/0x70
> WARNING: CPU: 0 PID: 483 at mm/slab_common.c:498
> kmem_cache_destroy+0x114/0x178
> <- Snipped info ->
>
> ------------[ cut here ]------------
> Memory manager not clean during takedown.
> <- Snipped info ->
> ---[ end trace 0000000000000000 ]---
> [drm:drm_mm_takedown] *ERROR* node [001000eb + 00000080]: inserted at
>  drm_mm_insert_node_in_range+0x48c/0x6a8
>  drm_vma_offset_add+0x84/0xb0
>  drm_gem_create_mmap_offset+0x50/0x70
>  __drm_gem_shmem_create+0x94/0x1d8
>  drm_gem_shmem_create+0x1c/0x30
>  virtio_gpu_object_create+0x68/0x468
>  virtio_gpu_resource_create_ioctl+0x1f0/0x420
>  drm_ioctl_kernel+0x170/0x248
>  drm_ioctl+0x33c/0x680
>  __arm64_sys_ioctl+0xdc/0x128
>  invoke_syscall+0x84/0x1c8
>  el0_svc_common.constprop.0+0x11c/0x150
>  do_el0_svc+0x38/0x50
>  el0_svc+0x38/0x70
>  el0t_64_sync_handler+0x120/0x130
>  el0t_64_sync+0x190/0x198
> [drm:drm_mm_takedown] *ERROR* node [0010016b + 000000eb]: inserted at
> <- Snipped info ->
>
>The leaked objects are also reported in /sys/kernel/debug/kmemleak.
>
>This commit adds a cleanup function that is called from
>virtio_gpu_deinit().
>
>The function cleans up any unused buffers from the virtqueues and calls
>the appropriate freeing functions. This is safe to do so because
>virtio_gpu_deinit() calls virtio_reset_device() before calling the
>cleanup function, ensuring no one is going to read from the virtqueues.
>
>The cleanup function checks for used buffers on the queues, and
>additionally calls virtqueue_detach_unused_buf on each queue to get any
>buffers that did not have time to be processed by the VIRTIO backend.
>
>Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>---
> drivers/gpu/drm/virtio/virtgpu_drv.h |  1 +
> drivers/gpu/drm/virtio/virtgpu_kms.c |  1 +
> drivers/gpu/drm/virtio/virtgpu_vq.c  | 55 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 57 insertions(+)
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>index f17660a71a3e7a22b5d4fefa6b754c227a294037..b3d367be6f204dbc98bf1c6e5c43a37ac8c0d8b3 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>@@ -419,6 +419,7 @@ void virtio_gpu_cursor_ack(struct virtqueue *vq);
> void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
> void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
> void virtio_gpu_panic_notify(struct virtio_gpu_device *vgdev);
>+void virtio_gpu_queue_cleanup(struct virtio_gpu_device *vgdev);
> void virtio_gpu_notify(struct virtio_gpu_device *vgdev);
>
> int
>diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
>index 7dfb2006c561ca13b15d979ddb8bf2d753e35dad..da70d9248072b64786a5d48b71bccaa80b8aae8f 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
>@@ -286,6 +286,7 @@ void virtio_gpu_deinit(struct drm_device *dev)
> 	flush_work(&vgdev->cursorq.dequeue_work);
> 	flush_work(&vgdev->config_changed_work);
> 	virtio_reset_device(vgdev->vdev);
>+	virtio_gpu_queue_cleanup(vgdev);
> 	vgdev->vdev->config->del_vqs(vgdev->vdev);
> }
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
>index 
>55a15e247dd1ad53a2b43b19fca8879b956f0e1a..fd150827e413cedcec4d82b0da8d792cb67e243f 
>100644
>--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
>@@ -299,6 +299,61 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
> 	wake_up(&vgdev->cursorq.ack_queue);
> }
>
>+/* deallocate all in-flight virtqueue elements */
>+void virtio_gpu_queue_cleanup(struct virtio_gpu_device *vgdev)
>+{
>+	struct list_head reclaim_list;
>+	struct virtio_gpu_vbuffer *entry, *tmp;
>+
>+	INIT_LIST_HEAD(&reclaim_list);
>+	spin_lock(&vgdev->ctrlq.qlock);
>+	do {
>+		virtqueue_disable_cb(vgdev->ctrlq.vq);
>+		reclaim_vbufs(vgdev->ctrlq.vq, &reclaim_list);
>+	} while (!virtqueue_enable_cb(vgdev->ctrlq.vq));

IIUC this function (virtio_gpu_queue_cleanup) is called after device is 
reset, so do we really need this cycle to disable notification and 
double check if the device queued new stuff?

I mean, maybe we can just call reclaim_vbufs(), since the device may not 
queue new elements anymore.

>+	/* detach unused buffers */
>+	while ((entry = virtqueue_detach_unused_buf(vgdev->ctrlq.vq)) != NULL) {

What about implementing a new reclaim_unused_vbufs(), similar to 
reclaim_vbufs() but calling virtqueue_detach_unused_buf() and queuing on 
reclaim_list, so we can do a single loop to clean all buffer in the same 
way?

>+		if (entry->resp_cb)
>+			entry->resp_cb(vgdev, entry);

Is it fine to call `entry->resp_cb` on an "unused" buffer?

>+		if (entry->objs)
>+			virtio_gpu_array_put_free(entry->objs);
>+		free_vbuf(vgdev, entry);
>+	}
>+	spin_unlock(&vgdev->ctrlq.qlock);
>+
>+	list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
>+		if (entry->resp_cb)
>+			entry->resp_cb(vgdev, entry);
>+		if (entry->objs)
>+			virtio_gpu_array_put_free(entry->objs);
>+		list_del(&entry->list);
>+		free_vbuf(vgdev, entry);
>+	}
>+

Similar comments also on the next section.
They looks very similar, so not sure if we can avoid code duplication 
adding a new function (e.g. renaming this function in 
`virtio_gpu_queues_cleanup()` and add a new `virtio_gpu_queue_cleanup()` 
to be called on the 2 virtqueues). Just an idea, not a strong opinion.

Thanks,
Stefano

>+	spin_lock(&vgdev->cursorq.qlock);
>+	do {
>+		virtqueue_disable_cb(vgdev->cursorq.vq);
>+		reclaim_vbufs(vgdev->cursorq.vq, &reclaim_list);
>+	} while (!virtqueue_enable_cb(vgdev->cursorq.vq));
>+	spin_unlock(&vgdev->cursorq.qlock);
>+	while ((entry = virtqueue_detach_unused_buf(vgdev->cursorq.vq)) != NULL) {
>+		if (entry->resp_cb)
>+			entry->resp_cb(vgdev, entry);
>+		if (entry->objs)
>+			virtio_gpu_array_put_free(entry->objs);
>+		free_vbuf(vgdev, entry);
>+	}
>+
>+	list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
>+		if (entry->resp_cb)
>+			entry->resp_cb(vgdev, entry);
>+		if (entry->objs)
>+			virtio_gpu_array_put_free(entry->objs);
>+		list_del(&entry->list);
>+		free_vbuf(vgdev, entry);
>+	}
>+}
>+
> /* Create sg_table from a vmalloc'd buffer. */
> static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size, int *sg_ents)
> {
>
>-- 
>2.47.2
>
>

Re: [PATCH 2/3] virtgpu: add virtio_gpu_fence_cleanup()

[Stefano Garzarella]

On Mon, May 05, 2025 at 11:59:15AM +0300, Manos Pitsidianakis wrote:
>When virtio_gpu_remove() is called, there might be in-flight command
>objects in the virtqueues that the VIRTIO device hasn't processed. These
>commands might use fences, which end up being leaked, as reported by
>/sys/kernel/debug/kmemleak.
>
>This commit adds a cleanup function that lowers the reference count of
>all in-flight fences, resulting in their de-allocation.
>
>Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>---
> drivers/gpu/drm/virtio/virtgpu_drv.h   |  1 +
> drivers/gpu/drm/virtio/virtgpu_fence.c | 12 ++++++++++++
> drivers/gpu/drm/virtio/virtgpu_kms.c   |  1 +
> 3 files changed, 14 insertions(+)
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>index b3d367be6f204dbc98bf1c6e5c43a37ac8c0d8b3..c94b5edb2aec42fe5cd6416e243cf40e4e2b060f 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>@@ -465,6 +465,7 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
> 			  struct virtio_gpu_fence *fence);
> void virtio_gpu_fence_event_process(struct virtio_gpu_device *vdev,
> 				    u64 fence_id);
>+void virtio_gpu_fence_cleanup(struct virtio_gpu_device *vdev);
>
> /* virtgpu_object.c */
> void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo);
>diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
>index 44c1d8ef3c4d07881e2c4c92cc67f6aba7a5df4f..3e536d190c0464f4db8955605bbf0aa4aa3612bd 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>@@ -157,3 +157,15 @@ void virtio_gpu_fence_event_process(struct virtio_gpu_device *vgdev,
> 	}
> 	spin_unlock_irqrestore(&drv->lock, irq_flags);
> }
>+
>+void virtio_gpu_fence_cleanup(struct virtio_gpu_device *vgdev)
>+{
>+	struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
>+	struct virtio_gpu_fence *curr, *tmp;
>+
>+	list_for_each_entry_safe(curr, tmp, &drv->fences, node) {

I don't know this code, but I see that when we access `drv->fences` we 
hold `drv->lock`, should we do the same here? (or it isn't needed since 
we are in the cleaning phase?)

The rest LGTM!

Thanks,
Stefano


>+		dma_fence_signal_locked(&curr->f);
>+		list_del(&curr->node);
>+		dma_fence_put(&curr->f);
>+	}
>+}
>diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
>index da70d9248072b64786a5d48b71bccaa80b8aae8f..7b3c4d314f8eee692e2842a7056d6dc64936fc2f 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
>@@ -286,6 +286,7 @@ void virtio_gpu_deinit(struct drm_device *dev)
> 	flush_work(&vgdev->cursorq.dequeue_work);
> 	flush_work(&vgdev->config_changed_work);
> 	virtio_reset_device(vgdev->vdev);
>+	virtio_gpu_fence_cleanup(vgdev);
> 	virtio_gpu_queue_cleanup(vgdev);
> 	vgdev->vdev->config->del_vqs(vgdev->vdev);
> }
>
>-- 
>2.47.2
>
>

Re: [PATCH 3/3] virtgpu: deallocate capsets on device deinit

[Stefano Garzarella]

On Mon, May 05, 2025 at 07:22:35PM +0300, Dmitry Osipenko wrote:
>On 5/5/25 18:58, Dmitry Osipenko wrote:
>> On 5/5/25 11:59, Manos Pitsidianakis wrote:
>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
>>> index 7b3c4d314f8eee692e2842a7056d6dc64936fc2f..a8b751179332b9ec2fbba1392a6ee0e638a5192e 100644
>>> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
>>> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
>>> @@ -286,6 +286,10 @@ void virtio_gpu_deinit(struct drm_device *dev)
>>>  	flush_work(&vgdev->cursorq.dequeue_work);
>>>  	flush_work(&vgdev->config_changed_work);
>>>  	virtio_reset_device(vgdev->vdev);
>>> +	spin_lock(&vgdev->display_info_lock);
>>> +	drmm_kfree(dev, vgdev->capsets);
>>> +	vgdev->capsets = NULL;
>>> +	spin_unlock(&vgdev->display_info_lock);
>>
>> Isn't this lock superfluous?
>
>Wait a minute, vgdev->capsets is allocated using drmm, hence it's
>auto-freed when DRM device is freed. This patch shouldn't be needed.

Yep, good point. I mean the patch is not wrong, but I think we can avoid 
it.

Thanks,
Stefano