Re: [PATCH] accel/ivpu: Use dma_resv_lock() instead of a custom mutex

[Lizhi Hou]

On 6/2/25 05:58, Jacek Lawrynowicz wrote:

Hi,

On 5/28/2025 7:50 PM, Lizhi Hou wrote:

On 5/28/25 08:43, Jacek Lawrynowicz wrote:

This fixes a potential race conditions in:
   - ivpu_bo_unbind_locked() where we modified the shmem->sgt without
     holding the dma_resv_lock().
   - ivpu_bo_print_info() where we read the shmem->pages without
     holding the dma_resv_lock().

Using dma_resv_lock() also protects against future syncronisation
issues that may arise when accessing drm_gem_shmem_object or
drm_gem_object members.

Fixes: 42328003ecb6 ("accel/ivpu: Refactor BO creation functions")
Cc: <sta...@vger.kernel.org> # v6.9+
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynow...@linux.intel.com>
---
   drivers/accel/ivpu/ivpu_gem.c | 63 +++++++++++++++++++----------------
   drivers/accel/ivpu/ivpu_gem.h |  1 -
   2 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
index c193a80241f5f..5908268ca45e9 100644
--- a/drivers/accel/ivpu/ivpu_gem.c
+++ b/drivers/accel/ivpu/ivpu_gem.c
@@ -33,6 +33,16 @@ static inline void ivpu_dbg_bo(struct ivpu_device *vdev, 
struct ivpu_bo *bo, con
            (bool)bo->base.base.import_attach);
   }
   +static inline int ivpu_bo_lock(struct ivpu_bo *bo)
+{
+    return dma_resv_lock(bo->base.base.resv, NULL);
+}
+
+static inline void ivpu_bo_unlock(struct ivpu_bo *bo)
+{
+    dma_resv_unlock(bo->base.base.resv);
+}
+
   /*
    * ivpu_bo_pin() - pin the backing physical pages and map them to VPU.
    *
@@ -43,22 +53,22 @@ static inline void ivpu_dbg_bo(struct ivpu_device *vdev, 
struct ivpu_bo *bo, con
   int __must_check ivpu_bo_pin(struct ivpu_bo *bo)
   {
       struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);
+    struct sg_table *sgt;
       int ret = 0;
   -    mutex_lock(&bo->lock);
-
       ivpu_dbg_bo(vdev, bo, "pin");
-    drm_WARN_ON(&vdev->drm, !bo->ctx);
   -    if (!bo->mmu_mapped) {
-        struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(&bo->base);
+    sgt = drm_gem_shmem_get_pages_sgt(&bo->base);
+    if (IS_ERR(sgt)) {
+        ret = PTR_ERR(sgt);
+        ivpu_err(vdev, "Failed to map BO in IOMMU: %d\n", ret);
+        return ret;
+    }
   -        if (IS_ERR(sgt)) {
-            ret = PTR_ERR(sgt);
-            ivpu_err(vdev, "Failed to map BO in IOMMU: %d\n", ret);
-            goto unlock;
-        }
+    ivpu_bo_lock(bo);
   +    if (!bo->mmu_mapped) {
+        drm_WARN_ON(&vdev->drm, !bo->ctx);
           ret = ivpu_mmu_context_map_sgt(vdev, bo->ctx, bo->vpu_addr, sgt,
                              ivpu_bo_is_snooped(bo));
           if (ret) {
@@ -69,7 +79,7 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo)
       }
     unlock:
-    mutex_unlock(&bo->lock);
+    ivpu_bo_unlock(bo);
         return ret;
   }
@@ -84,7 +94,7 @@ ivpu_bo_alloc_vpu_addr(struct ivpu_bo *bo, struct 
ivpu_mmu_context *ctx,
       if (!drm_dev_enter(&vdev->drm, &idx))
           return -ENODEV;
   -    mutex_lock(&bo->lock);
+    ivpu_bo_lock(bo);
         ret = ivpu_mmu_context_insert_node(ctx, range, ivpu_bo_size(bo), 
&bo->mm_node);
       if (!ret) {
@@ -94,7 +104,7 @@ ivpu_bo_alloc_vpu_addr(struct ivpu_bo *bo, struct 
ivpu_mmu_context *ctx,
           ivpu_err(vdev, "Failed to add BO to context %u: %d\n", ctx->id, ret);
       }
   -    mutex_unlock(&bo->lock);
+    ivpu_bo_unlock(bo);
         drm_dev_exit(idx);
   @@ -105,7 +115,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
   {
       struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);
   -    lockdep_assert(lockdep_is_held(&bo->lock) || 
!kref_read(&bo->base.base.refcount));
+    lockdep_assert(dma_resv_held(bo->base.base.resv) || 
!kref_read(&bo->base.base.refcount));
         if (bo->mmu_mapped) {
           drm_WARN_ON(&vdev->drm, !bo->ctx);
@@ -123,14 +133,12 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
       if (bo->base.base.import_attach)
           return;
   -    dma_resv_lock(bo->base.base.resv, NULL);
       if (bo->base.sgt) {
           dma_unmap_sgtable(vdev->drm.dev, bo->base.sgt, DMA_BIDIRECTIONAL, 0);
           sg_free_table(bo->base.sgt);
           kfree(bo->base.sgt);
           bo->base.sgt = NULL;

Maybe not directly modify sgt but use drm_gem_shmem_purge()?

drm_gem_shmem_purge() also removes user mapping and backing pages.
In ivpu_bo_unbind_locked() we only want to unmap the buffer from the device as 
it being turned off.
We don't want to crash user process in this case and this will probably be the 
result of drm_gem_shmem_purge() na mmpapped buffer.

Will it potentially memleak without calling drm_gem_shmem_put_pages()? (if the 
bo is mmap, vmap etc)

There is no memory leak. We are calling drm_gem_shmem_get_pages_sgt() only once 
per object and drm_gem_shmem_free() frees all backing memory.

Reviewed-by: Lizhi Hou <lizhi....@amd.com>

Regards,
Jacek