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

[Chris Wilson]

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.
-Chris