Re: [PATCH 7/9] drm/amdgpu/userq: take reset_domain->sem before userq_mutex in destroy

["Christian König" <christian.koenig@amd.com>]

On 10/13/26 10:24, Prike Liang wrote:
> amdgpu_userq_destroy() takes reset_domain->sem for read in
> amdgpu_userq_detach_doorbell() while holding userq_mutex.

Yeah that is correct behavior.

> GPU recovery
> holds reset_domain->sem for write and then takes clientlist_mutex in
> drm_client_dev_suspend(),

That is completely broken behavior and need to be fixed.

The GPU reset can *NEVER* call drm_client_dev_suspend(), who came up with that nonsense?

Regards,
Christian.

> which already depends on userq_mutex through
> the fbdev reservation and userq restore paths. Lockdep reports:
> 
>   Chain exists of:
>     &dev->clientlist_mutex --> &userq_mgr->userq_mutex --> &reset_domain->sem
> 
> Keep waiting for the last fence under userq_mutex only, as the fence of a
> hung queue is only force-completed by the reset, then take
> reset_domain->sem before re-acquiring userq_mutex for the doorbell
> detach, unmap and fence driver teardown. This also keeps the MES unmap
> from racing with an in-progress GPU reset.
> 
> Fixes: f18719ef4bb7 ("drm/amdgpu: Convert amdgpu userqueue management from IDR to XArray")
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 5fef0cc7703c..a34ea8ecd700 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -415,15 +415,6 @@ static void amdgpu_userq_wait_for_last_fence(struct amdgpu_usermode_queue *queue
>  	dma_fence_wait(f, false);
>  }
>  
> -static void amdgpu_userq_detach_doorbell(struct amdgpu_usermode_queue *queue)
> -{
> -	struct amdgpu_device *adev = queue->userq_mgr->adev;
> -
> -	down_read(&adev->reset_domain->sem);
> -	xa_erase_irq(&adev->userq_doorbell_xa, queue->doorbell_index);
> -	up_read(&adev->reset_domain->sem);
> -}
> -
>  /**
>   * amdgpu_userq_ensure_ev_fence - ensure a valid, unsignaled eviction fence exists
>   * @uq_mgr: the usermode queue manager for this process
> @@ -547,8 +538,12 @@ amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
>  
>  	mutex_lock(&uq_mgr->userq_mutex);
>  	amdgpu_userq_wait_for_last_fence(queue);
> +	mutex_unlock(&uq_mgr->userq_mutex);
>  
> -	amdgpu_userq_detach_doorbell(queue);
> +	/* GPU recovery takes userq_mutex under reset_domain->sem, keep that order */
> +	down_read(&adev->reset_domain->sem);
> +	mutex_lock(&uq_mgr->userq_mutex);
> +	xa_erase_irq(&adev->userq_doorbell_xa, queue->doorbell_index);
>  
>  #if defined(CONFIG_DEBUG_FS)
>  	debugfs_remove_recursive(queue->debugfs_queue);
> @@ -558,6 +553,7 @@ amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
>  	amdgpu_userq_fence_driver_free(queue);
>  	queue->fence_drv = NULL;
>  	mutex_unlock(&uq_mgr->userq_mutex);
> +	up_read(&adev->reset_domain->sem);
>  
>  	/*
>  	 * A failed unmap means MES could not remove the hung queue and is now