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!

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);
>  }
>