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 <[email protected]>
> ---
> 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
