On 6/18/26 15:18, Khatri, Sunil wrote:
>
> On 18-06-2026 06:12 pm, Christian König wrote:
>> On 6/18/26 12:51, Zhu Lingshan wrote:
>>> In amdgpu_userq_restore_all(), when failed to reserve
>>> a bo, it should return a meaningful error code other than
>>> "false" that means SUCCESS, which is wrong.
>>>
>>> The caller should not ignore the return code of
>>> amdgpu_userq_restore_all as well
>>>
>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 10 ++++++----
>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 95b680fc88c5..8b14870afbf5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -894,9 +894,9 @@ amdgpu_userq_restore_all(struct amdgpu_userq_mgr
>>> *uq_mgr)
>>> unsigned long queue_id;
>>> int ret = 0, r;
>>>
>>> -
>>> - if (amdgpu_bo_reserve(vm->root.bo, false))
>>> - return false;
>>> + r = amdgpu_bo_reserve(vm->root.bo, false);
>>> + if (r)
>>> + return r;
>> Good catch, but that amdgpu_bo_reserve() is called here is a bug in the
>> first place.
> I think i probably missed that. There is no return value check for
> amdgpu_userq_restore_all and this cant fail. We should wait uninterruptible
> here i.e change false->true and drop the if condition.
No, dropping and re-acquiring the lock is a broken approach to begin with.
>> The call to amdgpu_userq_vm_validate() must be moved into
>> amdgpu_userq_vm_validate(), right before we call drm_exec_fini() and the
>> manual call to amdgpu_bo_reserve() here dropped.
>>
>> Otherwise we have a small windows where we drop the BO locks before starting
>> the queues which could make the VM invalid again and cause all kind of
>> issues.
>
> we need to maintain order of locking First reserve root bo and then take
> mutex else we had mutex deadlocks.... this is why we have added root bo
> locking first before taking mutex in next line.
As long as we do that in amdgpu_userq_vm_validate() that should be
unproblematic. The VM root BO and all other BOs are still locked and acquiring
the userq_mutex shouldn't be much of a problem.
Regards,
Christian.
>
> Regards
>
> Sunil Khatri
>
>> Regards,
>> Christian.
>>
>>>
>>> mutex_lock(&uq_mgr->userq_mutex);
>>> /* Resume all the queues for this process */
>>> @@ -1133,7 +1133,9 @@ static void amdgpu_userq_restore_worker(struct
>>> work_struct *work)
>>> goto put_fence;
>>> }
>>>
>>> - amdgpu_userq_restore_all(uq_mgr);
>>> + ret = amdgpu_userq_restore_all(uq_mgr);
>>> + if (ret)
>>> + drm_file_err(uq_mgr->file, "Failed to restore user queues,
>>> ret=%d\n", ret);
>>>
>>> put_fence:
>>> dma_fence_put(ev_fence);
