Commit 48aea7f2 authored by Jacek Lawrynowicz's avatar Jacek Lawrynowicz
Browse files

accel/ivpu: Fix locking in ivpu_bo_remove_all_bos_from_context() 



ivpu_bo_remove_all_bos_from_context() could race with ivpu_bo_free()
when prime buffer was closed after vpu device was closed.

Move the bo_list from context to vdev and use a dedicated lock to
sync it. This list is not modified when BO is added/removed from
a context.

Also rename ivpu_bo_free_vpu_addr() to ivpu_bo_unbind() because this
function does more then just free vpu_addr.

Signed-off-by: default avatarJacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
Reviewed-by: default avatarJeffrey Hugo <quic_jhugo@quicinc.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231031073156.1301669-3-stanislaw.gruszka@linux.intel.com
parent b0352241

drivers/accel/ivpu/ivpu_drv.c

+6 −1
Original line number Diff line number Diff line
@@ -91,8 +91,8 @@ static void file_priv_release(struct kref *ref) 
	ivpu_dbg(vdev, FILE, "file_priv release: ctx %u\n", file_priv->ctx.id);



	ivpu_cmdq_release_all(file_priv);

	ivpu_bo_remove_all_bos_from_context(&file_priv->ctx);

	ivpu_jsm_context_release(vdev, file_priv->ctx.id);

	ivpu_bo_remove_all_bos_from_context(vdev, &file_priv->ctx);

	ivpu_mmu_user_context_fini(vdev, &file_priv->ctx);

	drm_WARN_ON(&vdev->drm, xa_erase_irq(&vdev->context_xa, file_priv->ctx.id) != file_priv);

	mutex_destroy(&file_priv->lock);
@@ -528,6 +528,11 @@ static int ivpu_dev_init(struct ivpu_device *vdev) 
	xa_init_flags(&vdev->context_xa, XA_FLAGS_ALLOC);

	xa_init_flags(&vdev->submitted_jobs_xa, XA_FLAGS_ALLOC1);

	lockdep_set_class(&vdev->submitted_jobs_xa.xa_lock, &submitted_jobs_xa_lock_class_key);

	INIT_LIST_HEAD(&vdev->bo_list);



	ret = drmm_mutex_init(&vdev->drm, &vdev->bo_list_lock);

	if (ret)

		goto err_xa_destroy;



	ret = ivpu_pci_init(vdev);

	if (ret)

drivers/accel/ivpu/ivpu_drv.h

+3 −0
Original line number Diff line number Diff line
@@ -115,6 +115,9 @@ struct ivpu_device { 
	struct xarray context_xa;

	struct xa_limit context_xa_limit;



	struct mutex bo_list_lock; /* Protects bo_list */

	struct list_head bo_list;



	struct xarray submitted_jobs_xa;

	struct task_struct *job_done_thread;

drivers/accel/ivpu/ivpu_gem.c

+68 −63
Original line number Diff line number Diff line
@@ -26,6 +26,17 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs; 


static struct lock_class_key prime_bo_lock_class_key;



static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action)

{

	if (bo->ctx)

		ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u ctx %d vpu_addr 0x%llx mmu_mapped %d\n",

			 action, bo->base.size, (bool)bo->pages, (bool)bo->sgt, bo->handle,

			 bo->ctx->id, bo->vpu_addr, bo->mmu_mapped);

	else

		ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u (not added to context)\n",

			 action, bo->base.size, (bool)bo->pages, (bool)bo->sgt, bo->handle);

}



static int __must_check prime_alloc_pages_locked(struct ivpu_bo *bo)

{

	/* Pages are managed by the underlying dma-buf */
@@ -245,9 +256,8 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo) 


	mutex_lock(&bo->lock);



	if (!bo->vpu_addr) {

		ivpu_err(vdev, "vpu_addr not set for BO ctx_id: %d handle: %d\n",

			 bo->ctx->id, bo->handle);

	if (!bo->ctx) {

		ivpu_err(vdev, "vpu_addr not allocated for BO %d\n", bo->handle);

		ret = -EINVAL;

		goto unlock;

	}
@@ -281,53 +291,68 @@ ivpu_bo_alloc_vpu_addr(struct ivpu_bo *bo, struct ivpu_mmu_context *ctx, 
	struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);

	int ret;



	mutex_lock(&ctx->lock);

	ret = ivpu_mmu_context_insert_node_locked(ctx, range, ivpu_bo_size(bo), &bo->mm_node);

	mutex_lock(&bo->lock);



	ret = ivpu_mmu_context_insert_node(ctx, range, bo->base.size, &bo->mm_node);

	if (!ret) {

		bo->ctx = ctx;

		bo->vpu_addr = bo->mm_node.start;

		list_add_tail(&bo->ctx_node, &ctx->bo_list);

	} else {

		ivpu_err(vdev, "Failed to add BO to context %u: %d\n", ctx->id, ret);

	}

	mutex_unlock(&ctx->lock);

	ivpu_dbg_bo(vdev, bo, "alloc");



	if (ret)

		ivpu_err(vdev, "Failed to add BO to context %u: %d\n", ctx->id, ret);

	mutex_unlock(&bo->lock);



	return ret;

}



static void ivpu_bo_free_vpu_addr(struct ivpu_bo *bo)

static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)

{

	struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);

	struct ivpu_mmu_context *ctx = bo->ctx;



	ivpu_dbg(vdev, BO, "remove from ctx: ctx %d vpu_addr 0x%llx allocated %d mmu_mapped %d\n",

		 ctx->id, bo->vpu_addr, (bool)bo->sgt, bo->mmu_mapped);

	lockdep_assert_held(&bo->lock);



	mutex_lock(&bo->lock);

	ivpu_dbg_bo(vdev, bo, "unbind");



	/* TODO: dma_unmap */

	if (bo->mmu_mapped) {

		drm_WARN_ON(&vdev->drm, !bo->ctx);

		drm_WARN_ON(&vdev->drm, !bo->vpu_addr);

		drm_WARN_ON(&vdev->drm, !bo->sgt);

		ivpu_mmu_context_unmap_sgt(vdev, ctx, bo->vpu_addr, bo->sgt);

		ivpu_mmu_context_unmap_sgt(vdev, bo->ctx, bo->vpu_addr, bo->sgt);

		bo->mmu_mapped = false;

	}



	mutex_lock(&ctx->lock);

	list_del(&bo->ctx_node);

	if (bo->ctx) {

		ivpu_mmu_context_remove_node(bo->ctx, &bo->mm_node);

		bo->vpu_addr = 0;

		bo->ctx = NULL;

	ivpu_mmu_context_remove_node_locked(ctx, &bo->mm_node);

	mutex_unlock(&ctx->lock);

	}

}



static void ivpu_bo_unbind(struct ivpu_bo *bo)

{

	mutex_lock(&bo->lock);

	ivpu_bo_unbind_locked(bo);

	mutex_unlock(&bo->lock);

}



void ivpu_bo_remove_all_bos_from_context(struct ivpu_mmu_context *ctx)

void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx)

{

	struct ivpu_bo *bo, *tmp;

	struct ivpu_bo *bo;



	list_for_each_entry_safe(bo, tmp, &ctx->bo_list, ctx_node)

		ivpu_bo_free_vpu_addr(bo);

	if (drm_WARN_ON(&vdev->drm, !ctx))

		return;



	mutex_lock(&vdev->bo_list_lock);

	list_for_each_entry(bo, &vdev->bo_list, bo_list_node) {

		mutex_lock(&bo->lock);

		if (bo->ctx == ctx)

			ivpu_bo_unbind_locked(bo);

		mutex_unlock(&bo->lock);

	}

	mutex_unlock(&vdev->bo_list_lock);

}



static struct ivpu_bo *
@@ -375,6 +400,10 @@ ivpu_bo_alloc(struct ivpu_device *vdev, u64 size, u32 flags, const struct ivpu_b 
		}

	}



	mutex_lock(&vdev->bo_list_lock);

	list_add_tail(&bo->bo_list_node, &vdev->bo_list);

	mutex_unlock(&vdev->bo_list_lock);



	return bo;



err_release:
@@ -406,19 +435,16 @@ static void ivpu_bo_free(struct drm_gem_object *obj) 
	struct ivpu_bo *bo = to_ivpu_bo(obj);

	struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);



	if (bo->ctx)

		ivpu_dbg(vdev, BO, "free: ctx %d vpu_addr 0x%llx allocated %d mmu_mapped %d\n",

			 bo->ctx->id, bo->vpu_addr, (bool)bo->sgt, bo->mmu_mapped);

	else

		ivpu_dbg(vdev, BO, "free: ctx (released) allocated %d mmu_mapped %d\n",

			 (bool)bo->sgt, bo->mmu_mapped);

	mutex_lock(&vdev->bo_list_lock);

	list_del(&bo->bo_list_node);

	mutex_unlock(&vdev->bo_list_lock);



	drm_WARN_ON(&vdev->drm, !dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_READ));



	vunmap(bo->kvaddr);

	ivpu_dbg_bo(vdev, bo, "free");



	if (bo->ctx)

		ivpu_bo_free_vpu_addr(bo);

	ivpu_bo_unbind(bo);

	vunmap(bo->kvaddr);



	if (bo->sgt)

		ivpu_bo_unmap_and_free_pages(bo);
@@ -435,10 +461,6 @@ static void ivpu_bo_free(struct drm_gem_object *obj) 
static int ivpu_bo_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)

{

	struct ivpu_bo *bo = to_ivpu_bo(obj);

	struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);



	ivpu_dbg(vdev, BO, "mmap: ctx %u handle %u vpu_addr 0x%llx size %zu type %s",

		 bo->ctx->id, bo->handle, bo->vpu_addr, ivpu_bo_size(bo), bo->ops->name);



	if (obj->import_attach) {

		/* Drop the reference drm_gem_mmap_obj() acquired.*/
@@ -553,9 +575,6 @@ ivpu_bo_create_ioctl(struct drm_device *dev, void *data, struct drm_file *file) 


	drm_gem_object_put(&bo->base);



	ivpu_dbg(vdev, BO, "alloc shmem: ctx %u vpu_addr 0x%llx size %zu flags 0x%x\n",

		 file_priv->ctx.id, bo->vpu_addr, ivpu_bo_size(bo), bo->flags);



	return ret;

}
@@ -609,9 +628,6 @@ ivpu_bo_alloc_internal(struct ivpu_device *vdev, u64 vpu_addr, u64 size, u32 fla 
		goto err_put;

	}



	ivpu_dbg(vdev, BO, "alloc internal: ctx 0 vpu_addr 0x%llx size %zu flags 0x%x\n",

		 bo->vpu_addr, ivpu_bo_size(bo), flags);



	return bo;



err_put:
@@ -708,41 +724,30 @@ static void ivpu_bo_print_info(struct ivpu_bo *bo, struct drm_printer *p) 
{

	unsigned long dma_refcount = 0;



	mutex_lock(&bo->lock);



	if (bo->base.dma_buf && bo->base.dma_buf->file)

		dma_refcount = atomic_long_read(&bo->base.dma_buf->file->f_count);



	drm_printf(p, "%5u %6d %16llx %10lu %10u %12lu %14s\n",

		   bo->ctx->id, bo->handle, bo->vpu_addr, ivpu_bo_size(bo),

		   bo->ctx ? bo->ctx->id : -1, bo->handle, bo->vpu_addr, bo->base.size,

		   kref_read(&bo->base.refcount), dma_refcount, bo->ops->name);



	mutex_unlock(&bo->lock);

}



void ivpu_bo_list(struct drm_device *dev, struct drm_printer *p)

{

	struct ivpu_device *vdev = to_ivpu_device(dev);

	struct ivpu_file_priv *file_priv;

	unsigned long ctx_id;

	struct ivpu_bo *bo;



	drm_printf(p, "%5s %6s %16s %10s %10s %12s %14s\n",

		   "ctx", "handle", "vpu_addr", "size", "refcount", "dma_refcount", "type");



	mutex_lock(&vdev->gctx.lock);

	list_for_each_entry(bo, &vdev->gctx.bo_list, ctx_node)

	mutex_lock(&vdev->bo_list_lock);

	list_for_each_entry(bo, &vdev->bo_list, bo_list_node)

		ivpu_bo_print_info(bo, p);

	mutex_unlock(&vdev->gctx.lock);



	xa_for_each(&vdev->context_xa, ctx_id, file_priv) {

		file_priv = ivpu_file_priv_get_by_ctx_id(vdev, ctx_id);

		if (!file_priv)

			continue;



		mutex_lock(&file_priv->ctx.lock);

		list_for_each_entry(bo, &file_priv->ctx.bo_list, ctx_node)

			ivpu_bo_print_info(bo, p);

		mutex_unlock(&file_priv->ctx.lock);



		ivpu_file_priv_put(&file_priv);

	}

	mutex_unlock(&vdev->bo_list_lock);

}



void ivpu_bo_list_print(struct drm_device *dev)

drivers/accel/ivpu/ivpu_gem.h

+3 −3
Original line number Diff line number Diff line
@@ -17,10 +17,10 @@ struct ivpu_bo { 
	const struct ivpu_bo_ops *ops;



	struct ivpu_mmu_context *ctx;

	struct list_head ctx_node;

	struct list_head bo_list_node;

	struct drm_mm_node mm_node;



	struct mutex lock; /* Protects: pages, sgt, mmu_mapped */

	struct mutex lock; /* Protects: pages, sgt, ctx, mmu_mapped, vpu_addr */

	struct sg_table *sgt;

	struct page **pages;

	bool mmu_mapped;
@@ -48,7 +48,7 @@ struct ivpu_bo_ops { 
};



int ivpu_bo_pin(struct ivpu_bo *bo);

void ivpu_bo_remove_all_bos_from_context(struct ivpu_mmu_context *ctx);

void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx);

void ivpu_bo_list(struct drm_device *dev, struct drm_printer *p);

void ivpu_bo_list_print(struct drm_device *dev);

drivers/accel/ivpu/ivpu_mmu.c

+4 −1
Original line number Diff line number Diff line
@@ -775,9 +775,12 @@ int ivpu_mmu_init(struct ivpu_device *vdev) 


	ivpu_dbg(vdev, MMU, "Init..\n");



	drmm_mutex_init(&vdev->drm, &mmu->lock);

	ivpu_mmu_config_check(vdev);



	ret = drmm_mutex_init(&vdev->drm, &mmu->lock);

	if (ret)

		return ret;



	ret = ivpu_mmu_structs_alloc(vdev);

	if (ret)

		return ret;