[PATCH 0/4] drm/panthor: More reset fixes

[PATCH 0/4] drm/panthor: More reset fixes

[Boris Brezillon]

Hello,

This is a collection of fixes for bugs found while chasing an
unrecoverable fault leading to a device unplug (because of some
other bugs that was introduced in my local dev branch).

The first patch makes sure we immediately reset the GPU on an
unrecoverable fault, and following patches are fixing various
NULL/invalid pointer derefs caused by use-after-free situations
following a device unplug.

Regards,

Boris

Boris Brezillon (4):
  drm/panthor: Force an immediate reset on unrecoverable faults
  drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
  drm/panthor: Reset the FW VM to NULL on unplug
  drm/panthor: Call panthor_sched_post_reset() even if the reset failed

 drivers/gpu/drm/panthor/panthor_device.c |  8 ++---
 drivers/gpu/drm/panthor/panthor_device.h |  1 +
 drivers/gpu/drm/panthor/panthor_fw.c     |  5 +--
 drivers/gpu/drm/panthor/panthor_gem.c    |  8 +++--
 drivers/gpu/drm/panthor/panthor_gem.h    |  8 +++--
 drivers/gpu/drm/panthor/panthor_heap.c   |  8 ++---
 drivers/gpu/drm/panthor/panthor_sched.c  | 40 +++++++++++++++++-------
 drivers/gpu/drm/panthor/panthor_sched.h  |  2 +-
 8 files changed, 51 insertions(+), 29 deletions(-)

-- 
2.44.0

[PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults

[Boris Brezillon]

If the FW reports an unrecoverable fault, we need to reset the GPU
before we can start re-using it again.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_device.c |  1 +
 drivers/gpu/drm/panthor/panthor_device.h |  1 +
 drivers/gpu/drm/panthor/panthor_sched.c  | 11 ++++++++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 75276cbeba20..4c5b54e7abb7 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -293,6 +293,7 @@ static const struct panthor_exception_info panthor_exception_infos[] = {
 	PANTHOR_EXCEPTION(ACTIVE),
 	PANTHOR_EXCEPTION(CS_RES_TERM),
 	PANTHOR_EXCEPTION(CS_CONFIG_FAULT),
+	PANTHOR_EXCEPTION(CS_UNRECOVERABLE),
 	PANTHOR_EXCEPTION(CS_ENDPOINT_FAULT),
 	PANTHOR_EXCEPTION(CS_BUS_FAULT),
 	PANTHOR_EXCEPTION(CS_INSTR_INVALID),
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 2fdd671b38fd..e388c0472ba7 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -216,6 +216,7 @@ enum drm_panthor_exception_type {
 	DRM_PANTHOR_EXCEPTION_CS_RES_TERM = 0x0f,
 	DRM_PANTHOR_EXCEPTION_MAX_NON_FAULT = 0x3f,
 	DRM_PANTHOR_EXCEPTION_CS_CONFIG_FAULT = 0x40,
+	DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE = 0x41,
 	DRM_PANTHOR_EXCEPTION_CS_ENDPOINT_FAULT = 0x44,
 	DRM_PANTHOR_EXCEPTION_CS_BUS_FAULT = 0x48,
 	DRM_PANTHOR_EXCEPTION_CS_INSTR_INVALID = 0x49,
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 7f16a4a14e9a..1d2708c3ab0a 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1281,7 +1281,16 @@ cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
 	if (group)
 		group->fatal_queues |= BIT(cs_id);
 
-	sched_queue_delayed_work(sched, tick, 0);
+	if (CS_EXCEPTION_TYPE(fatal) == DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE) {
+		/* If this exception is unrecoverable, queue a reset, and make
+		 * sure we stop scheduling groups until the reset has happened.
+		 */
+		panthor_device_schedule_reset(ptdev);
+		cancel_delayed_work(&sched->tick_work);
+	} else {
+		sched_queue_delayed_work(sched, tick, 0);
+	}
+
 	drm_warn(&ptdev->base,
 		 "CSG slot %d CS slot: %d\n"
 		 "CS_FATAL.EXCEPTION_TYPE: 0x%x (%s)\n"
-- 
2.44.0

[PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level

[Boris Brezillon]

Avoids use-after-free situations when panthor_fw_unplug() is called
and the kernel BO was mapped to the FW VM.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_fw.c    |  4 ++--
 drivers/gpu/drm/panthor/panthor_gem.c   |  8 +++++---
 drivers/gpu/drm/panthor/panthor_gem.h   |  8 ++++++--
 drivers/gpu/drm/panthor/panthor_heap.c  |  8 ++++----
 drivers/gpu/drm/panthor/panthor_sched.c | 11 +++++------
 5 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 181395e2859a..b41685304a83 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -453,7 +453,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
 
 	ret = panthor_kernel_bo_vmap(mem);
 	if (ret) {
-		panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), mem);
+		panthor_kernel_bo_destroy(mem);
 		return ERR_PTR(ret);
 	}
 
@@ -1133,7 +1133,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
 	panthor_fw_stop(ptdev);
 
 	list_for_each_entry(section, &ptdev->fw->sections, node)
-		panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), section->mem);
+		panthor_kernel_bo_destroy(section->mem);
 
 	/* We intentionally don't call panthor_vm_idle() and let
 	 * panthor_mmu_unplug() release the AS we acquired with
diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index d6483266d0c2..38f560864879 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -26,18 +26,18 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
 
 /**
  * panthor_kernel_bo_destroy() - Destroy a kernel buffer object
- * @vm: The VM this BO was mapped to.
  * @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
  * is skipped.
  */
-void panthor_kernel_bo_destroy(struct panthor_vm *vm,
-			       struct panthor_kernel_bo *bo)
+void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
 {
+	struct panthor_vm *vm;
 	int ret;
 
 	if (IS_ERR_OR_NULL(bo))
 		return;
 
+	vm = bo->vm;
 	panthor_kernel_bo_vunmap(bo);
 
 	if (drm_WARN_ON(bo->obj->dev,
@@ -53,6 +53,7 @@ void panthor_kernel_bo_destroy(struct panthor_vm *vm,
 	drm_gem_object_put(bo->obj);
 
 out_free_bo:
+	panthor_vm_put(vm);
 	kfree(bo);
 }
 
@@ -106,6 +107,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
 	if (ret)
 		goto err_free_va;
 
+	kbo->vm = panthor_vm_get(vm);
 	bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
 	drm_gem_object_get(bo->exclusive_vm_root_gem);
 	bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
index 3bccba394d00..e43021cf6d45 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.h
+++ b/drivers/gpu/drm/panthor/panthor_gem.h
@@ -61,6 +61,11 @@ struct panthor_kernel_bo {
 	 */
 	struct drm_gem_object *obj;
 
+	/**
+	 * @vm: VM this private buffer is attached to.
+	 */
+	struct panthor_vm *vm;
+
 	/**
 	 * @va_node: VA space allocated to this GEM.
 	 */
@@ -136,7 +141,6 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
 			 size_t size, u32 bo_flags, u32 vm_map_flags,
 			 u64 gpu_va);
 
-void panthor_kernel_bo_destroy(struct panthor_vm *vm,
-			       struct panthor_kernel_bo *bo);
+void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
 
 #endif /* __PANTHOR_GEM_H__ */
diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 143fa35f2e74..65921296a18c 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -127,7 +127,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
 	heap->chunk_count--;
 	mutex_unlock(&heap->lock);
 
-	panthor_kernel_bo_destroy(vm, chunk->bo);
+	panthor_kernel_bo_destroy(chunk->bo);
 	kfree(chunk);
 }
 
@@ -183,7 +183,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
 	return 0;
 
 err_destroy_bo:
-	panthor_kernel_bo_destroy(vm, chunk->bo);
+	panthor_kernel_bo_destroy(chunk->bo);
 
 err_free_chunk:
 	kfree(chunk);
@@ -391,7 +391,7 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
 	mutex_unlock(&heap->lock);
 
 	if (removed) {
-		panthor_kernel_bo_destroy(pool->vm, chunk->bo);
+		panthor_kernel_bo_destroy(chunk->bo);
 		kfree(chunk);
 		ret = 0;
 	} else {
@@ -587,7 +587,7 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
 		drm_WARN_ON(&pool->ptdev->base, panthor_heap_destroy_locked(pool, i));
 
 	if (!IS_ERR_OR_NULL(pool->gpu_contexts))
-		panthor_kernel_bo_destroy(pool->vm, pool->gpu_contexts);
+		panthor_kernel_bo_destroy(pool->gpu_contexts);
 
 	/* Reflects the fact the pool has been destroyed. */
 	pool->vm = NULL;
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 1d2708c3ab0a..6ea094b00cf9 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -826,8 +826,8 @@ static void group_free_queue(struct panthor_group *group, struct panthor_queue *
 
 	panthor_queue_put_syncwait_obj(queue);
 
-	panthor_kernel_bo_destroy(group->vm, queue->ringbuf);
-	panthor_kernel_bo_destroy(panthor_fw_vm(group->ptdev), queue->iface.mem);
+	panthor_kernel_bo_destroy(queue->ringbuf);
+	panthor_kernel_bo_destroy(queue->iface.mem);
 
 	kfree(queue);
 }
@@ -837,15 +837,14 @@ static void group_release_work(struct work_struct *work)
 	struct panthor_group *group = container_of(work,
 						   struct panthor_group,
 						   release_work);
-	struct panthor_device *ptdev = group->ptdev;
 	u32 i;
 
 	for (i = 0; i < group->queue_count; i++)
 		group_free_queue(group, group->queues[i]);
 
-	panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->suspend_buf);
-	panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->protm_suspend_buf);
-	panthor_kernel_bo_destroy(group->vm, group->syncobjs);
+	panthor_kernel_bo_destroy(group->suspend_buf);
+	panthor_kernel_bo_destroy(group->protm_suspend_buf);
+	panthor_kernel_bo_destroy(group->syncobjs);
 
 	panthor_vm_put(group->vm);
 	kfree(group);
-- 
2.44.0

[PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug

[Boris Brezillon]

This way get NULL derefs instead of use-after-free if the FW VM is
referenced after the device has been unplugged.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_fw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index b41685304a83..93165961a6b5 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -1141,6 +1141,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
 	 * state to keep the active_refcnt balanced.
 	 */
 	panthor_vm_put(ptdev->fw->vm);
+	ptdev->fw->vm = NULL;
 
 	panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present, 20000);
 }
-- 
2.44.0

[PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed

[Boris Brezillon]

We need to undo what was done in panthor_sched_pre_reset() even if the
reset failed. We just flag all previously running groups as terminated
when that happens to unblock things.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_device.c |  7 +------
 drivers/gpu/drm/panthor/panthor_sched.c  | 19 ++++++++++++++-----
 drivers/gpu/drm/panthor/panthor_sched.h  |  2 +-
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 4c5b54e7abb7..4082c8f2951d 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -129,13 +129,8 @@ static void panthor_device_reset_work(struct work_struct *work)
 	panthor_gpu_l2_power_on(ptdev);
 	panthor_mmu_post_reset(ptdev);
 	ret = panthor_fw_post_reset(ptdev);
-	if (ret)
-		goto out_dev_exit;
-
 	atomic_set(&ptdev->reset.pending, 0);
-	panthor_sched_post_reset(ptdev);
-
-out_dev_exit:
+	panthor_sched_post_reset(ptdev, ret != 0);
 	drm_dev_exit(cookie);
 
 	if (ret) {
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 6ea094b00cf9..fc43ff62c77d 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2728,15 +2728,22 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
 	mutex_unlock(&sched->reset.lock);
 }
 
-void panthor_sched_post_reset(struct panthor_device *ptdev)
+void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
 {
 	struct panthor_scheduler *sched = ptdev->scheduler;
 	struct panthor_group *group, *group_tmp;
 
 	mutex_lock(&sched->reset.lock);
 
-	list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node)
+	list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node) {
+		/* Consider all previously running group as terminated if the
+		 * reset failed.
+		 */
+		if (reset_failed)
+			group->state = PANTHOR_CS_GROUP_TERMINATED;
+
 		panthor_group_start(group);
+	}
 
 	/* We're done resetting the GPU, clear the reset.in_progress bit so we can
 	 * kick the scheduler.
@@ -2744,9 +2751,11 @@ void panthor_sched_post_reset(struct panthor_device *ptdev)
 	atomic_set(&sched->reset.in_progress, false);
 	mutex_unlock(&sched->reset.lock);
 
-	sched_queue_delayed_work(sched, tick, 0);
-
-	sched_queue_work(sched, sync_upd);
+	/* No need to queue a tick and update syncs if the reset failed. */
+	if (!reset_failed) {
+		sched_queue_delayed_work(sched, tick, 0);
+		sched_queue_work(sched, sync_upd);
+	}
 }
 
 static void group_sync_upd_work(struct work_struct *work)
diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
index 66438b1f331f..3a30d2328b30 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.h
+++ b/drivers/gpu/drm/panthor/panthor_sched.h
@@ -40,7 +40,7 @@ void panthor_group_pool_destroy(struct panthor_file *pfile);
 int panthor_sched_init(struct panthor_device *ptdev);
 void panthor_sched_unplug(struct panthor_device *ptdev);
 void panthor_sched_pre_reset(struct panthor_device *ptdev);
-void panthor_sched_post_reset(struct panthor_device *ptdev);
+void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed);
 void panthor_sched_suspend(struct panthor_device *ptdev);
 void panthor_sched_resume(struct panthor_device *ptdev);
 
-- 
2.44.0

Re: [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults

[Steven Price]

On 02/05/2024 19:38, Boris Brezillon wrote:
> If the FW reports an unrecoverable fault, we need to reset the GPU
> before we can start re-using it again.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  1 +
>  drivers/gpu/drm/panthor/panthor_device.h |  1 +
>  drivers/gpu/drm/panthor/panthor_sched.c  | 11 ++++++++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 75276cbeba20..4c5b54e7abb7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -293,6 +293,7 @@ static const struct panthor_exception_info panthor_exception_infos[] = {
>  	PANTHOR_EXCEPTION(ACTIVE),
>  	PANTHOR_EXCEPTION(CS_RES_TERM),
>  	PANTHOR_EXCEPTION(CS_CONFIG_FAULT),
> +	PANTHOR_EXCEPTION(CS_UNRECOVERABLE),
>  	PANTHOR_EXCEPTION(CS_ENDPOINT_FAULT),
>  	PANTHOR_EXCEPTION(CS_BUS_FAULT),
>  	PANTHOR_EXCEPTION(CS_INSTR_INVALID),
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 2fdd671b38fd..e388c0472ba7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -216,6 +216,7 @@ enum drm_panthor_exception_type {
>  	DRM_PANTHOR_EXCEPTION_CS_RES_TERM = 0x0f,
>  	DRM_PANTHOR_EXCEPTION_MAX_NON_FAULT = 0x3f,
>  	DRM_PANTHOR_EXCEPTION_CS_CONFIG_FAULT = 0x40,
> +	DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE = 0x41,
>  	DRM_PANTHOR_EXCEPTION_CS_ENDPOINT_FAULT = 0x44,
>  	DRM_PANTHOR_EXCEPTION_CS_BUS_FAULT = 0x48,
>  	DRM_PANTHOR_EXCEPTION_CS_INSTR_INVALID = 0x49,
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 7f16a4a14e9a..1d2708c3ab0a 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1281,7 +1281,16 @@ cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
>  	if (group)
>  		group->fatal_queues |= BIT(cs_id);
>  
> -	sched_queue_delayed_work(sched, tick, 0);
> +	if (CS_EXCEPTION_TYPE(fatal) == DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE) {
> +		/* If this exception is unrecoverable, queue a reset, and make
> +		 * sure we stop scheduling groups until the reset has happened.
> +		 */
> +		panthor_device_schedule_reset(ptdev);
> +		cancel_delayed_work(&sched->tick_work);
> +	} else {
> +		sched_queue_delayed_work(sched, tick, 0);
> +	}
> +
>  	drm_warn(&ptdev->base,
>  		 "CSG slot %d CS slot: %d\n"
>  		 "CS_FATAL.EXCEPTION_TYPE: 0x%x (%s)\n"

Re: [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level

[Steven Price]

On 02/05/2024 19:38, Boris Brezillon wrote:
> Avoids use-after-free situations when panthor_fw_unplug() is called
> and the kernel BO was mapped to the FW VM.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

It makes the code more readable too - I like it.

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_fw.c    |  4 ++--
>  drivers/gpu/drm/panthor/panthor_gem.c   |  8 +++++---
>  drivers/gpu/drm/panthor/panthor_gem.h   |  8 ++++++--
>  drivers/gpu/drm/panthor/panthor_heap.c  |  8 ++++----
>  drivers/gpu/drm/panthor/panthor_sched.c | 11 +++++------
>  5 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 181395e2859a..b41685304a83 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -453,7 +453,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
>  
>  	ret = panthor_kernel_bo_vmap(mem);
>  	if (ret) {
> -		panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), mem);
> +		panthor_kernel_bo_destroy(mem);
>  		return ERR_PTR(ret);
>  	}
>  
> @@ -1133,7 +1133,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>  	panthor_fw_stop(ptdev);
>  
>  	list_for_each_entry(section, &ptdev->fw->sections, node)
> -		panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), section->mem);
> +		panthor_kernel_bo_destroy(section->mem);
>  
>  	/* We intentionally don't call panthor_vm_idle() and let
>  	 * panthor_mmu_unplug() release the AS we acquired with
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index d6483266d0c2..38f560864879 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -26,18 +26,18 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
>  
>  /**
>   * panthor_kernel_bo_destroy() - Destroy a kernel buffer object
> - * @vm: The VM this BO was mapped to.
>   * @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
>   * is skipped.
>   */
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> -			       struct panthor_kernel_bo *bo)
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  {
> +	struct panthor_vm *vm;
>  	int ret;
>  
>  	if (IS_ERR_OR_NULL(bo))
>  		return;
>  
> +	vm = bo->vm;
>  	panthor_kernel_bo_vunmap(bo);
>  
>  	if (drm_WARN_ON(bo->obj->dev,
> @@ -53,6 +53,7 @@ void panthor_kernel_bo_destroy(struct panthor_vm *vm,
>  	drm_gem_object_put(bo->obj);
>  
>  out_free_bo:
> +	panthor_vm_put(vm);
>  	kfree(bo);
>  }
>  
> @@ -106,6 +107,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
>  	if (ret)
>  		goto err_free_va;
>  
> +	kbo->vm = panthor_vm_get(vm);
>  	bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
>  	drm_gem_object_get(bo->exclusive_vm_root_gem);
>  	bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> index 3bccba394d00..e43021cf6d45 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.h
> +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> @@ -61,6 +61,11 @@ struct panthor_kernel_bo {
>  	 */
>  	struct drm_gem_object *obj;
>  
> +	/**
> +	 * @vm: VM this private buffer is attached to.
> +	 */
> +	struct panthor_vm *vm;
> +
>  	/**
>  	 * @va_node: VA space allocated to this GEM.
>  	 */
> @@ -136,7 +141,6 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
>  			 size_t size, u32 bo_flags, u32 vm_map_flags,
>  			 u64 gpu_va);
>  
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> -			       struct panthor_kernel_bo *bo);
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
>  
>  #endif /* __PANTHOR_GEM_H__ */
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 143fa35f2e74..65921296a18c 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -127,7 +127,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
>  	heap->chunk_count--;
>  	mutex_unlock(&heap->lock);
>  
> -	panthor_kernel_bo_destroy(vm, chunk->bo);
> +	panthor_kernel_bo_destroy(chunk->bo);
>  	kfree(chunk);
>  }
>  
> @@ -183,7 +183,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
>  	return 0;
>  
>  err_destroy_bo:
> -	panthor_kernel_bo_destroy(vm, chunk->bo);
> +	panthor_kernel_bo_destroy(chunk->bo);
>  
>  err_free_chunk:
>  	kfree(chunk);
> @@ -391,7 +391,7 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
>  	mutex_unlock(&heap->lock);
>  
>  	if (removed) {
> -		panthor_kernel_bo_destroy(pool->vm, chunk->bo);
> +		panthor_kernel_bo_destroy(chunk->bo);
>  		kfree(chunk);
>  		ret = 0;
>  	} else {
> @@ -587,7 +587,7 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
>  		drm_WARN_ON(&pool->ptdev->base, panthor_heap_destroy_locked(pool, i));
>  
>  	if (!IS_ERR_OR_NULL(pool->gpu_contexts))
> -		panthor_kernel_bo_destroy(pool->vm, pool->gpu_contexts);
> +		panthor_kernel_bo_destroy(pool->gpu_contexts);
>  
>  	/* Reflects the fact the pool has been destroyed. */
>  	pool->vm = NULL;
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 1d2708c3ab0a..6ea094b00cf9 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -826,8 +826,8 @@ static void group_free_queue(struct panthor_group *group, struct panthor_queue *
>  
>  	panthor_queue_put_syncwait_obj(queue);
>  
> -	panthor_kernel_bo_destroy(group->vm, queue->ringbuf);
> -	panthor_kernel_bo_destroy(panthor_fw_vm(group->ptdev), queue->iface.mem);
> +	panthor_kernel_bo_destroy(queue->ringbuf);
> +	panthor_kernel_bo_destroy(queue->iface.mem);
>  
>  	kfree(queue);
>  }
> @@ -837,15 +837,14 @@ static void group_release_work(struct work_struct *work)
>  	struct panthor_group *group = container_of(work,
>  						   struct panthor_group,
>  						   release_work);
> -	struct panthor_device *ptdev = group->ptdev;
>  	u32 i;
>  
>  	for (i = 0; i < group->queue_count; i++)
>  		group_free_queue(group, group->queues[i]);
>  
> -	panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->suspend_buf);
> -	panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->protm_suspend_buf);
> -	panthor_kernel_bo_destroy(group->vm, group->syncobjs);
> +	panthor_kernel_bo_destroy(group->suspend_buf);
> +	panthor_kernel_bo_destroy(group->protm_suspend_buf);
> +	panthor_kernel_bo_destroy(group->syncobjs);
>  
>  	panthor_vm_put(group->vm);
>  	kfree(group);

Re: [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug

[Steven Price]

On 02/05/2024 19:38, Boris Brezillon wrote:
> This way get NULL derefs instead of use-after-free if the FW VM is
> referenced after the device has been unplugged.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_fw.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index b41685304a83..93165961a6b5 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -1141,6 +1141,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>  	 * state to keep the active_refcnt balanced.
>  	 */
>  	panthor_vm_put(ptdev->fw->vm);
> +	ptdev->fw->vm = NULL;
>  
>  	panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present, 20000);
>  }

Re: [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed

[Steven Price]

On 02/05/2024 19:38, Boris Brezillon wrote:
> We need to undo what was done in panthor_sched_pre_reset() even if the
> reset failed. We just flag all previously running groups as terminated
> when that happens to unblock things.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Seems reasonable, although I hope this case doesn't happen in practice ;)

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  7 +------
>  drivers/gpu/drm/panthor/panthor_sched.c  | 19 ++++++++++++++-----
>  drivers/gpu/drm/panthor/panthor_sched.h  |  2 +-
>  3 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 4c5b54e7abb7..4082c8f2951d 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -129,13 +129,8 @@ static void panthor_device_reset_work(struct work_struct *work)
>  	panthor_gpu_l2_power_on(ptdev);
>  	panthor_mmu_post_reset(ptdev);
>  	ret = panthor_fw_post_reset(ptdev);
> -	if (ret)
> -		goto out_dev_exit;
> -
>  	atomic_set(&ptdev->reset.pending, 0);
> -	panthor_sched_post_reset(ptdev);
> -
> -out_dev_exit:
> +	panthor_sched_post_reset(ptdev, ret != 0);
>  	drm_dev_exit(cookie);
>  
>  	if (ret) {
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 6ea094b00cf9..fc43ff62c77d 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2728,15 +2728,22 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
>  	mutex_unlock(&sched->reset.lock);
>  }
>  
> -void panthor_sched_post_reset(struct panthor_device *ptdev)
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
>  {
>  	struct panthor_scheduler *sched = ptdev->scheduler;
>  	struct panthor_group *group, *group_tmp;
>  
>  	mutex_lock(&sched->reset.lock);
>  
> -	list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node)
> +	list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node) {
> +		/* Consider all previously running group as terminated if the
> +		 * reset failed.
> +		 */
> +		if (reset_failed)
> +			group->state = PANTHOR_CS_GROUP_TERMINATED;
> +
>  		panthor_group_start(group);
> +	}
>  
>  	/* We're done resetting the GPU, clear the reset.in_progress bit so we can
>  	 * kick the scheduler.
> @@ -2744,9 +2751,11 @@ void panthor_sched_post_reset(struct panthor_device *ptdev)
>  	atomic_set(&sched->reset.in_progress, false);
>  	mutex_unlock(&sched->reset.lock);
>  
> -	sched_queue_delayed_work(sched, tick, 0);
> -
> -	sched_queue_work(sched, sync_upd);
> +	/* No need to queue a tick and update syncs if the reset failed. */
> +	if (!reset_failed) {
> +		sched_queue_delayed_work(sched, tick, 0);
> +		sched_queue_work(sched, sync_upd);
> +	}
>  }
>  
>  static void group_sync_upd_work(struct work_struct *work)
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
> index 66438b1f331f..3a30d2328b30 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.h
> +++ b/drivers/gpu/drm/panthor/panthor_sched.h
> @@ -40,7 +40,7 @@ void panthor_group_pool_destroy(struct panthor_file *pfile);
>  int panthor_sched_init(struct panthor_device *ptdev);
>  void panthor_sched_unplug(struct panthor_device *ptdev);
>  void panthor_sched_pre_reset(struct panthor_device *ptdev);
> -void panthor_sched_post_reset(struct panthor_device *ptdev);
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed);
>  void panthor_sched_suspend(struct panthor_device *ptdev);
>  void panthor_sched_resume(struct panthor_device *ptdev);
>  

Re: [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults

[Liviu Dudau]

On Thu, May 02, 2024 at 08:38:09PM +0200, Boris Brezillon wrote:
> If the FW reports an unrecoverable fault, we need to reset the GPU
> before we can start re-using it again.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>


> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  1 +
>  drivers/gpu/drm/panthor/panthor_device.h |  1 +
>  drivers/gpu/drm/panthor/panthor_sched.c  | 11 ++++++++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 75276cbeba20..4c5b54e7abb7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -293,6 +293,7 @@ static const struct panthor_exception_info panthor_exception_infos[] = {
>  	PANTHOR_EXCEPTION(ACTIVE),
>  	PANTHOR_EXCEPTION(CS_RES_TERM),
>  	PANTHOR_EXCEPTION(CS_CONFIG_FAULT),
> +	PANTHOR_EXCEPTION(CS_UNRECOVERABLE),
>  	PANTHOR_EXCEPTION(CS_ENDPOINT_FAULT),
>  	PANTHOR_EXCEPTION(CS_BUS_FAULT),
>  	PANTHOR_EXCEPTION(CS_INSTR_INVALID),
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 2fdd671b38fd..e388c0472ba7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -216,6 +216,7 @@ enum drm_panthor_exception_type {
>  	DRM_PANTHOR_EXCEPTION_CS_RES_TERM = 0x0f,
>  	DRM_PANTHOR_EXCEPTION_MAX_NON_FAULT = 0x3f,
>  	DRM_PANTHOR_EXCEPTION_CS_CONFIG_FAULT = 0x40,
> +	DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE = 0x41,
>  	DRM_PANTHOR_EXCEPTION_CS_ENDPOINT_FAULT = 0x44,
>  	DRM_PANTHOR_EXCEPTION_CS_BUS_FAULT = 0x48,
>  	DRM_PANTHOR_EXCEPTION_CS_INSTR_INVALID = 0x49,
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 7f16a4a14e9a..1d2708c3ab0a 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1281,7 +1281,16 @@ cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
>  	if (group)
>  		group->fatal_queues |= BIT(cs_id);
>  
> -	sched_queue_delayed_work(sched, tick, 0);
> +	if (CS_EXCEPTION_TYPE(fatal) == DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE) {
> +		/* If this exception is unrecoverable, queue a reset, and make
> +		 * sure we stop scheduling groups until the reset has happened.
> +		 */
> +		panthor_device_schedule_reset(ptdev);
> +		cancel_delayed_work(&sched->tick_work);
> +	} else {
> +		sched_queue_delayed_work(sched, tick, 0);
> +	}
> +
>  	drm_warn(&ptdev->base,
>  		 "CSG slot %d CS slot: %d\n"
>  		 "CS_FATAL.EXCEPTION_TYPE: 0x%x (%s)\n"
> -- 
> 2.44.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level

[Liviu Dudau]

On Thu, May 02, 2024 at 08:38:10PM +0200, Boris Brezillon wrote:
> Avoids use-after-free situations when panthor_fw_unplug() is called
> and the kernel BO was mapped to the FW VM.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_fw.c    |  4 ++--
>  drivers/gpu/drm/panthor/panthor_gem.c   |  8 +++++---
>  drivers/gpu/drm/panthor/panthor_gem.h   |  8 ++++++--
>  drivers/gpu/drm/panthor/panthor_heap.c  |  8 ++++----
>  drivers/gpu/drm/panthor/panthor_sched.c | 11 +++++------
>  5 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 181395e2859a..b41685304a83 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -453,7 +453,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
>  
>  	ret = panthor_kernel_bo_vmap(mem);
>  	if (ret) {
> -		panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), mem);
> +		panthor_kernel_bo_destroy(mem);
>  		return ERR_PTR(ret);
>  	}
>  
> @@ -1133,7 +1133,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>  	panthor_fw_stop(ptdev);
>  
>  	list_for_each_entry(section, &ptdev->fw->sections, node)
> -		panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), section->mem);
> +		panthor_kernel_bo_destroy(section->mem);
>  
>  	/* We intentionally don't call panthor_vm_idle() and let
>  	 * panthor_mmu_unplug() release the AS we acquired with
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index d6483266d0c2..38f560864879 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -26,18 +26,18 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
>  
>  /**
>   * panthor_kernel_bo_destroy() - Destroy a kernel buffer object
> - * @vm: The VM this BO was mapped to.
>   * @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
>   * is skipped.
>   */
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> -			       struct panthor_kernel_bo *bo)
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  {
> +	struct panthor_vm *vm;
>  	int ret;
>  
>  	if (IS_ERR_OR_NULL(bo))
>  		return;
>  
> +	vm = bo->vm;
>  	panthor_kernel_bo_vunmap(bo);
>  
>  	if (drm_WARN_ON(bo->obj->dev,
> @@ -53,6 +53,7 @@ void panthor_kernel_bo_destroy(struct panthor_vm *vm,
>  	drm_gem_object_put(bo->obj);
>  
>  out_free_bo:
> +	panthor_vm_put(vm);
>  	kfree(bo);
>  }
>  
> @@ -106,6 +107,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
>  	if (ret)
>  		goto err_free_va;
>  
> +	kbo->vm = panthor_vm_get(vm);
>  	bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
>  	drm_gem_object_get(bo->exclusive_vm_root_gem);
>  	bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> index 3bccba394d00..e43021cf6d45 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.h
> +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> @@ -61,6 +61,11 @@ struct panthor_kernel_bo {
>  	 */
>  	struct drm_gem_object *obj;
>  
> +	/**
> +	 * @vm: VM this private buffer is attached to.
> +	 */
> +	struct panthor_vm *vm;
> +
>  	/**
>  	 * @va_node: VA space allocated to this GEM.
>  	 */
> @@ -136,7 +141,6 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
>  			 size_t size, u32 bo_flags, u32 vm_map_flags,
>  			 u64 gpu_va);
>  
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> -			       struct panthor_kernel_bo *bo);
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
>  
>  #endif /* __PANTHOR_GEM_H__ */
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 143fa35f2e74..65921296a18c 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -127,7 +127,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
>  	heap->chunk_count--;
>  	mutex_unlock(&heap->lock);
>  
> -	panthor_kernel_bo_destroy(vm, chunk->bo);
> +	panthor_kernel_bo_destroy(chunk->bo);
>  	kfree(chunk);
>  }
>  
> @@ -183,7 +183,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
>  	return 0;
>  
>  err_destroy_bo:
> -	panthor_kernel_bo_destroy(vm, chunk->bo);
> +	panthor_kernel_bo_destroy(chunk->bo);
>  
>  err_free_chunk:
>  	kfree(chunk);
> @@ -391,7 +391,7 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
>  	mutex_unlock(&heap->lock);
>  
>  	if (removed) {
> -		panthor_kernel_bo_destroy(pool->vm, chunk->bo);
> +		panthor_kernel_bo_destroy(chunk->bo);
>  		kfree(chunk);
>  		ret = 0;
>  	} else {
> @@ -587,7 +587,7 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
>  		drm_WARN_ON(&pool->ptdev->base, panthor_heap_destroy_locked(pool, i));
>  
>  	if (!IS_ERR_OR_NULL(pool->gpu_contexts))
> -		panthor_kernel_bo_destroy(pool->vm, pool->gpu_contexts);
> +		panthor_kernel_bo_destroy(pool->gpu_contexts);
>  
>  	/* Reflects the fact the pool has been destroyed. */
>  	pool->vm = NULL;
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 1d2708c3ab0a..6ea094b00cf9 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -826,8 +826,8 @@ static void group_free_queue(struct panthor_group *group, struct panthor_queue *
>  
>  	panthor_queue_put_syncwait_obj(queue);
>  
> -	panthor_kernel_bo_destroy(group->vm, queue->ringbuf);
> -	panthor_kernel_bo_destroy(panthor_fw_vm(group->ptdev), queue->iface.mem);
> +	panthor_kernel_bo_destroy(queue->ringbuf);
> +	panthor_kernel_bo_destroy(queue->iface.mem);
>  
>  	kfree(queue);
>  }
> @@ -837,15 +837,14 @@ static void group_release_work(struct work_struct *work)
>  	struct panthor_group *group = container_of(work,
>  						   struct panthor_group,
>  						   release_work);
> -	struct panthor_device *ptdev = group->ptdev;
>  	u32 i;
>  
>  	for (i = 0; i < group->queue_count; i++)
>  		group_free_queue(group, group->queues[i]);
>  
> -	panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->suspend_buf);
> -	panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->protm_suspend_buf);
> -	panthor_kernel_bo_destroy(group->vm, group->syncobjs);
> +	panthor_kernel_bo_destroy(group->suspend_buf);
> +	panthor_kernel_bo_destroy(group->protm_suspend_buf);
> +	panthor_kernel_bo_destroy(group->syncobjs);
>  
>  	panthor_vm_put(group->vm);
>  	kfree(group);
> -- 
> 2.44.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug

[Liviu Dudau]

On Fri, May 03, 2024 at 10:22:13AM +0100, Steven Price wrote:
> On 02/05/2024 19:38, Boris Brezillon wrote:
> > This way get NULL derefs instead of use-after-free if the FW VM is
> > referenced after the device has been unplugged.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Reviewed-by: Steven Price <steven.price@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_fw.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index b41685304a83..93165961a6b5 100644
> > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > @@ -1141,6 +1141,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> >  	 * state to keep the active_refcnt balanced.
> >  	 */
> >  	panthor_vm_put(ptdev->fw->vm);
> > +	ptdev->fw->vm = NULL;
> >  
> >  	panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present, 20000);
> >  }
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed

[Liviu Dudau]

On Thu, May 02, 2024 at 08:38:12PM +0200, Boris Brezillon wrote:
> We need to undo what was done in panthor_sched_pre_reset() even if the
> reset failed. We just flag all previously running groups as terminated
> when that happens to unblock things.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  7 +------
>  drivers/gpu/drm/panthor/panthor_sched.c  | 19 ++++++++++++++-----
>  drivers/gpu/drm/panthor/panthor_sched.h  |  2 +-
>  3 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 4c5b54e7abb7..4082c8f2951d 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -129,13 +129,8 @@ static void panthor_device_reset_work(struct work_struct *work)
>  	panthor_gpu_l2_power_on(ptdev);
>  	panthor_mmu_post_reset(ptdev);
>  	ret = panthor_fw_post_reset(ptdev);
> -	if (ret)
> -		goto out_dev_exit;
> -
>  	atomic_set(&ptdev->reset.pending, 0);
> -	panthor_sched_post_reset(ptdev);
> -
> -out_dev_exit:
> +	panthor_sched_post_reset(ptdev, ret != 0);
>  	drm_dev_exit(cookie);
>  
>  	if (ret) {
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 6ea094b00cf9..fc43ff62c77d 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2728,15 +2728,22 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
>  	mutex_unlock(&sched->reset.lock);
>  }
>  
> -void panthor_sched_post_reset(struct panthor_device *ptdev)
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
>  {
>  	struct panthor_scheduler *sched = ptdev->scheduler;
>  	struct panthor_group *group, *group_tmp;
>  
>  	mutex_lock(&sched->reset.lock);
>  
> -	list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node)
> +	list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node) {
> +		/* Consider all previously running group as terminated if the
> +		 * reset failed.
> +		 */
> +		if (reset_failed)
> +			group->state = PANTHOR_CS_GROUP_TERMINATED;
> +
>  		panthor_group_start(group);
> +	}
>  
>  	/* We're done resetting the GPU, clear the reset.in_progress bit so we can
>  	 * kick the scheduler.
> @@ -2744,9 +2751,11 @@ void panthor_sched_post_reset(struct panthor_device *ptdev)
>  	atomic_set(&sched->reset.in_progress, false);
>  	mutex_unlock(&sched->reset.lock);
>  
> -	sched_queue_delayed_work(sched, tick, 0);
> -
> -	sched_queue_work(sched, sync_upd);
> +	/* No need to queue a tick and update syncs if the reset failed. */
> +	if (!reset_failed) {
> +		sched_queue_delayed_work(sched, tick, 0);
> +		sched_queue_work(sched, sync_upd);
> +	}
>  }
>  
>  static void group_sync_upd_work(struct work_struct *work)
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
> index 66438b1f331f..3a30d2328b30 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.h
> +++ b/drivers/gpu/drm/panthor/panthor_sched.h
> @@ -40,7 +40,7 @@ void panthor_group_pool_destroy(struct panthor_file *pfile);
>  int panthor_sched_init(struct panthor_device *ptdev);
>  void panthor_sched_unplug(struct panthor_device *ptdev);
>  void panthor_sched_pre_reset(struct panthor_device *ptdev);
> -void panthor_sched_post_reset(struct panthor_device *ptdev);
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed);
>  void panthor_sched_suspend(struct panthor_device *ptdev);
>  void panthor_sched_resume(struct panthor_device *ptdev);
>  
> -- 
> 2.44.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 0/4] drm/panthor: More reset fixes

[Boris Brezillon]

On Thu,  2 May 2024 20:38:08 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hello,
> 
> This is a collection of fixes for bugs found while chasing an
> unrecoverable fault leading to a device unplug (because of some
> other bugs that was introduced in my local dev branch).
> 
> The first patch makes sure we immediately reset the GPU on an
> unrecoverable fault, and following patches are fixing various
> NULL/invalid pointer derefs caused by use-after-free situations
> following a device unplug.
> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (4):
>   drm/panthor: Force an immediate reset on unrecoverable faults
>   drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
>   drm/panthor: Reset the FW VM to NULL on unplug
>   drm/panthor: Call panthor_sched_post_reset() even if the reset failed

Queued to drm-misc-next-fixes.

> 
>  drivers/gpu/drm/panthor/panthor_device.c |  8 ++---
>  drivers/gpu/drm/panthor/panthor_device.h |  1 +
>  drivers/gpu/drm/panthor/panthor_fw.c     |  5 +--
>  drivers/gpu/drm/panthor/panthor_gem.c    |  8 +++--
>  drivers/gpu/drm/panthor/panthor_gem.h    |  8 +++--
>  drivers/gpu/drm/panthor/panthor_heap.c   |  8 ++---
>  drivers/gpu/drm/panthor/panthor_sched.c  | 40 +++++++++++++++++-------
>  drivers/gpu/drm/panthor/panthor_sched.h  |  2 +-
>  8 files changed, 51 insertions(+), 29 deletions(-)
>