On 8/13/26 10:18, Liang, Prike wrote:
> AMD General
>
> Regards,
> Prike
>
>> -----Original Message-----
>> From: Koenig, Christian <[email protected]>
>> Sent: Thursday, August 13, 2026 3:54 PM
>> To: Liang, Prike <[email protected]>; [email protected]
>> Cc: Deucher, Alexander <[email protected]>
>> Subject: Re: [PATCH] drm/amdgpu/userq: fix lock missing for userq fence
>> error set
>>
>>
>>
>> On 8/7/26 08:14, Prike Liang wrote:
>>> amdgpu_userq_fence_driver() and amdgpu_userq_fence_driver_destroy()
>>> don't acquire the dma_fence spinlock, so locking the dma_fence lock
>>> before test the signaled state and set error state.
>>>
>>> Signed-off-by: Prike Liang <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 15 ++++++++++-----
>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>> index e8de64f8357d..db5bfd79ec1f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>> @@ -193,12 +193,12 @@ void amdgpu_userq_fence_driver_destroy(struct kref
>> *ref)
>>> spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
>>> list_for_each_entry_safe(fence, tmp, &fence_drv->fences, link) {
>>> f = &fence->base;
>>> -
>>> - if (!dma_fence_is_signaled(f)) {
>>> + spin_lock(dma_fence_spinlock(f));
>>
>> Please use dma_fence_lock_irqsave() here and below, using only spin_lock is
>> completely unsafe!
>
> Since acquiring fence_drv->fence_list_lock with spin_lock_irqsave() already
> disables local IRQ, do we need to disable it again while the lock is held?
In theory we can skip disabling interrupts locally when we know that they
aren't enabled, but this needs some annotation or otherwise lockdep could start
to complain.
As long as it is not some super performance critical path we should not do such
optimizations.
Regards,
Christian.
>
> Regards,
> Prike
>
>> Apart from that looks good to me,
>> Christian.
>>
>>> + if (!dma_fence_is_signaled_locked(f)) {
>>> dma_fence_set_error(f, -ECANCELED);
>>> dma_fence_signal(f);
>>> }
>>> -
>>> + spin_unlock(dma_fence_spinlock(f));
>>> list_del(&fence->link);
>>> dma_fence_put(f);
>>> }
>>> @@ -428,8 +428,13 @@ amdgpu_userq_fence_driver_set_error(struct
>>> amdgpu_userq_fence *fence,
>>>
>>> f = rcu_dereference_protected(&fence->base,
>>>
>>> lockdep_is_held(&fence_drv->fence_list_lock));
>>> - if (f && !dma_fence_is_signaled_locked(f))
>>> - dma_fence_set_error(f, error);
>>> + if (f) {
>>> + /* nest f->lock inside fence_list_lock */
>>> + spin_lock(dma_fence_spinlock(f));
>>> + if (!dma_fence_is_signaled_locked(f))
>>> + dma_fence_set_error(f, error);
>>> + spin_unlock(dma_fence_spinlock(f));
>>> + }
>>> spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags); }
>>>
>
