Re: [PATCH] drm/amdgpu: Harden against potential deadlock

[Christian König]

On 10/26/26 19:31, Felix Kuehling wrote:
> On 2026-10-26 10:15, Philipp Stanner wrote:
>> amdgpu's KFD eviction fence's dma_fence ops->enable_signaling() contains
>> a dma_fence_is_signaled() check.
>>
>> ops->enable_signaling() is currently consistently being called with the
>> dma_fence lock held. dma_fence_is_signaled() might try to take this lock
>> in its dma_fence_signal() call.
>>
>> This deadlock can currently not occur because the amdkfd_fence_ops do
>> not implement ops->signaled. Should that ever be added, however,
>> deadlocks might occur occasionally once the fastpath check in
>> dma_fence_is_signaled() is skipped.
>>
>> Replace dma_fence_is_signaled() with dma_fence_is_signaled_locked().
>>
>> Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
>> ---
>> As detailed in my recent discussions with Christian [1], I also think
>> that drivers should be very tightly constrained as to what they do in
>> the fence_ops. I think at the very least they shouldn't use any
>> dma_fence functions there.
>>
>> So we could also remove this check here completely;
>> __dma_fence_enable_signaling() already checks the signaled-flag, after
>> all, but I'm not sure whether that might break amdgpu.
> 
> Looking at the code, I think it should be safe to remove the check. There is another one in kgd2kfd_schedule_evict_and_restore_process, which is called from amdkfd_fence_enable_signaling, which is also redundant.
> 
> We don't implement ops->signaled, and we explicitly signal the fence from SW with dma_fence_signal or dma_fence_check_and_signal. So the signaled flag should be the only thing that needs to be checked. The flag is only set while holding the fence lock, and checked while holding the fence lock. So in theory, relying on the check in __dma_fence_enable_signaling should be safe.

I actually had a patch to remove that one send out month ago already, it was just never merged.

The call absolutely doesn't make sense since the ops->signaled callback isn't implemented and the bit tested before the enable signaling callback called.

I can take care and push the removal to amd-staging-drm-next when I find time.

Regards,
Christian.

> 
> Regards,
>   Felix
> 
> 
>>
>>
>> P.
>>
>> [1] https://lore.kernel.org/dri-devel/320b6e7c75da6f05c015e89df0e2c7d11f20d70a.camel@xxxxxxxxxxx/
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>> index 9b10d015671c..61621eebb2b0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>> @@ -123,7 +123,8 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
>>       if (!fence)
>>           return false;
>>   -    if (dma_fence_is_signaled(f))
>> +    /* The enable_signaling() ops is called under lock protection. */
>> +    if (dma_fence_is_signaled_locked(f))
>>           return true;
>>         if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, fence->context_id, f))