Skip to content

Commit f1cc6ac

Browse files
jlawrynojlawryno
committed
accel/ivpu: Fix dev open/close races with unbind
 - Add context_list_lock to synchronize user context addition/removal - Use drm_dev_enter() to prevent unbinding the device during ivpu_open() and vpu address allocation Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> Reviewed-by: Wachowski, Karol <karol.wachowski@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20240122120945.1150728-2-jacek.lawrynowicz@linux.intel.com
1 parent d1b163a commit f1cc6ac

File tree

6 files changed

+85
-65
lines changed

6 files changed

+85
-65
lines changed

‎drivers/accel/ivpu/ivpu_drv.c

Lines changed: 67 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <linux/firmware.h>
77
#include <linux/module.h>
88
#include <linux/pci.h>
9+
#include <linux/pm_runtime.h>
910

1011
#include <drm/drm_accel.h>
1112
#include <drm/drm_file.h>
@@ -66,36 +67,36 @@ struct ivpu_file_priv *ivpu_file_priv_get(struct ivpu_file_priv *file_priv)
6667
return file_priv;
6768
}
6869

69-
struct ivpu_file_priv *ivpu_file_priv_get_by_ctx_id(struct ivpu_device *vdev, unsigned long id)
70+
static void file_priv_unbind(struct ivpu_device *vdev, struct ivpu_file_priv *file_priv)
7071
{
71-
struct ivpu_file_priv *file_priv;
72-
73-
xa_lock_irq(&vdev->context_xa);
74-
file_priv = xa_load(&vdev->context_xa, id);
75-
/* file_priv may still be in context_xa during file_priv_release() */
76-
if (file_priv && !kref_get_unless_zero(&file_priv->ref))
77-
file_priv = NULL;
78-
xa_unlock_irq(&vdev->context_xa);
79-
80-
if (file_priv)
81-
ivpu_dbg(vdev, KREF, "file_priv get by id: ctx %u refcount %u\n",
82-
file_priv->ctx.id, kref_read(&file_priv->ref));
83-
84-
return file_priv;
72+
mutex_lock(&file_priv->lock);
73+
if (file_priv->bound) {
74+
ivpu_dbg(vdev, FILE, "file_priv unbind: ctx %u\n", file_priv->ctx.id);
75+
76+
ivpu_cmdq_release_all_locked(file_priv);
77+
ivpu_jsm_context_release(vdev, file_priv->ctx.id);
78+
ivpu_bo_unbind_all_bos_from_context(vdev, &file_priv->ctx);
79+
ivpu_mmu_user_context_fini(vdev, &file_priv->ctx);
80+
file_priv->bound = false;
81+
drm_WARN_ON(&vdev->drm, !xa_erase_irq(&vdev->context_xa, file_priv->ctx.id));
82+
}
83+
mutex_unlock(&file_priv->lock);
8584
}
8685

8786
static void file_priv_release(struct kref *ref)
8887
{
8988
struct ivpu_file_priv *file_priv = container_of(ref, struct ivpu_file_priv, ref);
9089
struct ivpu_device *vdev = file_priv->vdev;
9190

92-
ivpu_dbg(vdev, FILE, "file_priv release: ctx %u\n", file_priv->ctx.id);
91+
ivpu_dbg(vdev, FILE, "file_priv release: ctx %u bound %d\n",
92+
file_priv->ctx.id, (bool)file_priv->bound);
93+
94+
pm_runtime_get_sync(vdev->drm.dev);
95+
mutex_lock(&vdev->context_list_lock);
96+
file_priv_unbind(vdev, file_priv);
97+
mutex_unlock(&vdev->context_list_lock);
98+
pm_runtime_put_autosuspend(vdev->drm.dev);
9399

94-
ivpu_cmdq_release_all(file_priv);
95-
ivpu_jsm_context_release(vdev, file_priv->ctx.id);
96-
ivpu_bo_remove_all_bos_from_context(vdev, &file_priv->ctx);
97-
ivpu_mmu_user_context_fini(vdev, &file_priv->ctx);
98-
drm_WARN_ON(&vdev->drm, xa_erase_irq(&vdev->context_xa, file_priv->ctx.id) != file_priv);
99100
mutex_destroy(&file_priv->lock);
100101
kfree(file_priv);
101102
}
@@ -232,49 +233,53 @@ static int ivpu_open(struct drm_device *dev, struct drm_file *file)
232233
struct ivpu_device *vdev = to_ivpu_device(dev);
233234
struct ivpu_file_priv *file_priv;
234235
u32 ctx_id;
235-
void *old;
236-
int ret;
236+
int idx, ret;
237237

238-
ret = xa_alloc_irq(&vdev->context_xa, &ctx_id, NULL, vdev->context_xa_limit, GFP_KERNEL);
239-
if (ret) {
240-
ivpu_err(vdev, "Failed to allocate context id: %d\n", ret);
241-
return ret;
242-
}
238+
if (!drm_dev_enter(dev, &idx))
239+
return -ENODEV;
243240

244241
file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
245242
if (!file_priv) {
246243
ret = -ENOMEM;
247-
goto err_xa_erase;
244+
goto err_dev_exit;
248245
}
249246

250247
file_priv->vdev = vdev;
248+
file_priv->bound = true;
251249
kref_init(&file_priv->ref);
252250
mutex_init(&file_priv->lock);
253251

252+
mutex_lock(&vdev->context_list_lock);
253+
254+
ret = xa_alloc_irq(&vdev->context_xa, &ctx_id, file_priv,
255+
vdev->context_xa_limit, GFP_KERNEL);
256+
if (ret) {
257+
ivpu_err(vdev, "Failed to allocate context id: %d\n", ret);
258+
goto err_unlock;
259+
}
260+
254261
ret = ivpu_mmu_user_context_init(vdev, &file_priv->ctx, ctx_id);
255262
if (ret)
256-
goto err_mutex_destroy;
263+
goto err_xa_erase;
257264

258-
old = xa_store_irq(&vdev->context_xa, ctx_id, file_priv, GFP_KERNEL);
259-
if (xa_is_err(old)) {
260-
ret = xa_err(old);
261-
ivpu_err(vdev, "Failed to store context %u: %d\n", ctx_id, ret);
262-
goto err_ctx_fini;
263-
}
265+
mutex_unlock(&vdev->context_list_lock);
266+
drm_dev_exit(idx);
267+
268+
file->driver_priv = file_priv;
264269

265270
ivpu_dbg(vdev, FILE, "file_priv create: ctx %u process %s pid %d\n",
266271
ctx_id, current->comm, task_pid_nr(current));
267272

268-
file->driver_priv = file_priv;
269273
return 0;
270274

271-
err_ctx_fini:
272-
ivpu_mmu_user_context_fini(vdev, &file_priv->ctx);
273-
err_mutex_destroy:
274-
mutex_destroy(&file_priv->lock);
275-
kfree(file_priv);
276275
err_xa_erase:
277276
xa_erase_irq(&vdev->context_xa, ctx_id);
277+
err_unlock:
278+
mutex_unlock(&vdev->context_list_lock);
279+
mutex_destroy(&file_priv->lock);
280+
kfree(file_priv);
281+
err_dev_exit:
282+
drm_dev_exit(idx);
278283
return ret;
279284
}
280285

@@ -531,6 +536,10 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
531536
lockdep_set_class(&vdev->submitted_jobs_xa.xa_lock, &submitted_jobs_xa_lock_class_key);
532537
INIT_LIST_HEAD(&vdev->bo_list);
533538

539+
ret = drmm_mutex_init(&vdev->drm, &vdev->context_list_lock);
540+
if (ret)
541+
goto err_xa_destroy;
542+
534543
ret = drmm_mutex_init(&vdev->drm, &vdev->bo_list_lock);
535544
if (ret)
536545
goto err_xa_destroy;
@@ -602,14 +611,30 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
602611
return ret;
603612
}
604613

614+
static void ivpu_bo_unbind_all_user_contexts(struct ivpu_device *vdev)
615+
{
616+
struct ivpu_file_priv *file_priv;
617+
unsigned long ctx_id;
618+
619+
mutex_lock(&vdev->context_list_lock);
620+
621+
xa_for_each(&vdev->context_xa, ctx_id, file_priv)
622+
file_priv_unbind(vdev, file_priv);
623+
624+
mutex_unlock(&vdev->context_list_lock);
625+
}
626+
605627
static void ivpu_dev_fini(struct ivpu_device *vdev)
606628
{
607629
ivpu_pm_disable(vdev);
608630
ivpu_shutdown(vdev);
609631
if (IVPU_WA(d3hot_after_power_off))
610632
pci_set_power_state(to_pci_dev(vdev->drm.dev), PCI_D3hot);
633+
634+
ivpu_jobs_abort_all(vdev);
611635
ivpu_job_done_consumer_fini(vdev);
612636
ivpu_pm_cancel_recovery(vdev);
637+
ivpu_bo_unbind_all_user_contexts(vdev);
613638

614639
ivpu_ipc_fini(vdev);
615640
ivpu_fw_fini(vdev);

‎drivers/accel/ivpu/ivpu_drv.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ struct ivpu_device {
115115

116116
struct ivpu_mmu_context gctx;
117117
struct ivpu_mmu_context rctx;
118+
struct mutex context_list_lock; /* Protects user context addition/removal */
118119
struct xarray context_xa;
119120
struct xa_limit context_xa_limit;
120121

@@ -147,6 +148,7 @@ struct ivpu_file_priv {
147148
struct ivpu_cmdq *cmdq[IVPU_NUM_ENGINES];
148149
struct ivpu_mmu_context ctx;
149150
bool has_mmu_faults;
151+
bool bound;
150152
};
151153

152154
extern int ivpu_dbg_mask;
@@ -162,7 +164,6 @@ extern bool ivpu_disable_mmu_cont_pages;
162164
extern int ivpu_test_mode;
163165

164166
struct ivpu_file_priv *ivpu_file_priv_get(struct ivpu_file_priv *file_priv);
165-
struct ivpu_file_priv *ivpu_file_priv_get_by_ctx_id(struct ivpu_device *vdev, unsigned long id);
166167
void ivpu_file_priv_put(struct ivpu_file_priv **link);
167168

168169
int ivpu_boot(struct ivpu_device *vdev);

‎drivers/accel/ivpu/ivpu_gem.c

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,10 @@ ivpu_bo_alloc_vpu_addr(struct ivpu_bo *bo, struct ivpu_mmu_context *ctx,
7777
const struct ivpu_addr_range *range)
7878
{
7979
struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);
80-
int ret;
80+
int idx, ret;
81+
82+
if (!drm_dev_enter(&vdev->drm, &idx))
83+
return -ENODEV;
8184

8285
mutex_lock(&bo->lock);
8386

@@ -93,6 +96,8 @@ ivpu_bo_alloc_vpu_addr(struct ivpu_bo *bo, struct ivpu_mmu_context *ctx,
9396

9497
mutex_unlock(&bo->lock);
9598

99+
drm_dev_exit(idx);
100+
96101
return ret;
97102
}
98103

@@ -128,14 +133,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
128133
dma_resv_unlock(bo->base.base.resv);
129134
}
130135

131-
static void ivpu_bo_unbind(struct ivpu_bo *bo)
132-
{
133-
mutex_lock(&bo->lock);
134-
ivpu_bo_unbind_locked(bo);
135-
mutex_unlock(&bo->lock);
136-
}
137-
138-
void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx)
136+
void ivpu_bo_unbind_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx)
139137
{
140138
struct ivpu_bo *bo;
141139

@@ -239,7 +237,7 @@ static void ivpu_bo_free(struct drm_gem_object *obj)
239237

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

242-
ivpu_bo_unbind(bo);
240+
ivpu_bo_unbind_locked(bo);
243241
mutex_destroy(&bo->lock);
244242

245243
drm_WARN_ON(obj->dev, bo->base.pages_use_count > 1);

‎drivers/accel/ivpu/ivpu_gem.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ struct ivpu_bo {
2525
};
2626

2727
int ivpu_bo_pin(struct ivpu_bo *bo);
28-
void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx);
28+
void ivpu_bo_unbind_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx);
2929

3030
struct drm_gem_object *ivpu_gem_create_object(struct drm_device *dev, size_t size);
3131
struct ivpu_bo *ivpu_bo_alloc_internal(struct ivpu_device *vdev, u64 vpu_addr, u64 size, u32 flags);

‎drivers/accel/ivpu/ivpu_job.c

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -112,16 +112,14 @@ static void ivpu_cmdq_release_locked(struct ivpu_file_priv *file_priv, u16 engin
112112
}
113113
}
114114

115-
void ivpu_cmdq_release_all(struct ivpu_file_priv *file_priv)
115+
void ivpu_cmdq_release_all_locked(struct ivpu_file_priv *file_priv)
116116
{
117117
int i;
118118

119-
mutex_lock(&file_priv->lock);
119+
lockdep_assert_held(&file_priv->lock);
120120

121121
for (i = 0; i < IVPU_NUM_ENGINES; i++)
122122
ivpu_cmdq_release_locked(file_priv, i);
123-
124-
mutex_unlock(&file_priv->lock);
125123
}
126124

127125
/*
@@ -161,15 +159,13 @@ void ivpu_cmdq_reset_all_contexts(struct ivpu_device *vdev)
161159
struct ivpu_file_priv *file_priv;
162160
unsigned long ctx_id;
163161

164-
xa_for_each(&vdev->context_xa, ctx_id, file_priv) {
165-
file_priv = ivpu_file_priv_get_by_ctx_id(vdev, ctx_id);
166-
if (!file_priv)
167-
continue;
162+
mutex_lock(&vdev->context_list_lock);
168163

164+
xa_for_each(&vdev->context_xa, ctx_id, file_priv)
169165
ivpu_cmdq_reset_all(file_priv);
170166

171-
ivpu_file_priv_put(&file_priv);
172-
}
167+
mutex_unlock(&vdev->context_list_lock);
168+
173169
}
174170

175171
static int ivpu_cmdq_push_job(struct ivpu_cmdq *cmdq, struct ivpu_job *job)

‎drivers/accel/ivpu/ivpu_job.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ struct ivpu_job {
5656

5757
int ivpu_submit_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
5858

59-
void ivpu_cmdq_release_all(struct ivpu_file_priv *file_priv);
59+
void ivpu_cmdq_release_all_locked(struct ivpu_file_priv *file_priv);
6060
void ivpu_cmdq_reset_all_contexts(struct ivpu_device *vdev);
6161

6262
void ivpu_job_done_consumer_init(struct ivpu_device *vdev);

0 commit comments

Comments
 (0)