RE: [PATCH] drm/amdgpu/userq: fix lock missing for userq fence error set

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?

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