[Intel-gfx] [PATCH] gpu/drm/i915: disable interrupt when holding spinlock

[Wang Xiayang]

----- Chris Wilson <chris at chris-wilson.co.uk> wrote:
> Quoting Wang Xiayang (2019-08-07 15:54:37)
> > The irq_lock is acquired in multiple functions:
> > 
> > 1) i915_request_cancel_breadcrumb
> >  <- ... <- panfrost_gpu_irq_handler
> > 2) intel_engine_breadcrumbs_irq
> >  <- ... <- cherryview_irq_handler
> > 3) i915_request_enable_breadcrumb
> > 4) virtual_xfer_breadcrumbs
> > 
> > The former two functions are reachable from IRQ handlers while
> > the latter two functions are not, and they call spin_lock()
> > which do not disable interrupt. Being preempted by an interrupt
> > acquiring the same lock may lead to deadlock.
> > Other functions acquire irq_lock by spin_lock_irq/irqsave().
> > 
> > This patch switches spin_lock() to spin_lock_irq in the two
> > process-context functions.
> > 
> > The issue is identified by a static analyzer based on Coccinelle.
> > 
> > Signed-off-by: Wang Xiayang <xywang.sjtu at sjtu.edu.cn>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 4 ++--
> >  drivers/gpu/drm/i915/gt/intel_lrc.c         | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > index c092bdf5f0bf..e0b46450c2f5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > @@ -301,7 +301,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
> >                 struct intel_context *ce = rq->hw_context;
> >                 struct list_head *pos;
> >  
> > -               spin_lock(&b->irq_lock);
> > +               spin_lock_irq(&b->irq_lock);
> >                 GEM_BUG_ON(test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
> >  
> >                 __intel_breadcrumbs_arm_irq(b);
> > @@ -333,7 +333,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
> >                 GEM_BUG_ON(!check_signal_order(ce, rq));
> >  
> >                 set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> > -               spin_unlock(&b->irq_lock);
> > +               spin_unlock_irq(&b->irq_lock);
> 
> This is very broken, irqs are disabled by the caller and you can't
> unconditionally enable them again here...
> 
> >         return !__request_completed(rq);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 82b7ace62d97..42367aeefcce 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -806,13 +806,13 @@ static void virtual_xfer_breadcrumbs(struct virtual_engine *ve,
> >  
> >         /* All unattached (rq->engine == old) must already be completed */
> >  
> > -       spin_lock(&old->breadcrumbs.irq_lock);
> > +       spin_lock_irq(&old->breadcrumbs.irq_lock);
> >         if (!list_empty(&ve->context.signal_link)) {
> >                 list_move_tail(&ve->context.signal_link,
> >                                &engine->breadcrumbs.signalers);
> >                 intel_engine_queue_breadcrumbs(engine);
> >         }
> > -       spin_unlock(&old->breadcrumbs.irq_lock);
> > +       spin_unlock_irq(&old->breadcrumbs.irq_lock);
> 
> Or here.

I see. Deeply sorry for the false alarming.