Re: [PATCH v8 25/27] drm/i915/gt: Add a spinlock to prevent starvation of irq_work.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing hardirq disable when acquiring the `signaler_active_sync` spinlock in `intel_context_remove_breadcrumbs()`, leading to a hard deadlock and lockdep splat on non-RT kernels.
--

commit c441a60da28f5ef8bb16a4ea0dbdd666e51caed6
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

drm/i915/gt: Add a spinlock to prevent starvation of irq_work.

This commit adds the signaler_active_sync spinlock to prevent irq_work
starvation and force completion.

> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -209,6 +209,7 @@ static void signal_irq_work(struct irq_work *work)
>  		intel_breadcrumbs_disarm_irq(b);
>  
>  	rcu_read_lock();
> +	spin_lock(&b->signaler_active_sync);

[Severity: High]
Because signal_irq_work() executes as an irq_work, this lock is acquired in
hardirq context on non-RT kernels. Doesn't this establish a requirement that
all other takers must disable hardware interrupts?

>  	atomic_inc(&b->signaler_active);

[ ... ]

> @@ -487,8 +490,11 @@ void intel_context_remove_breadcrumbs(struct intel_context *ce,
>  	if (release)
>  		intel_context_put(ce);
>  
> -	while (atomic_read(&b->signaler_active))
> +	while (atomic_read(&b->signaler_active)) {
> +		spin_lock(&b->signaler_active_sync);

[Severity: High]
Can this cause a deadlock if a hardware interrupt fires while we hold this
lock?

Since intel_context_remove_breadcrumbs() acquires the lock using a plain
spin_lock(), it leaves hardware interrupts enabled.

If a hardware interrupt fires on the same CPU while the lock is held,
and triggers the irq_work, would we see a scenario like this:

CPU0
intel_context_remove_breadcrumbs()
   spin_lock(&b->signaler_active_sync);
   <hardware interrupt fires>
   signal_irq_work()
      spin_lock(&b->signaler_active_sync);  <- deadlock
      
Should this use spin_lock_irq() or spin_lock_irqsave() instead?

> +		spin_unlock(&b->signaler_active_sync);
>  		cpu_relax();
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615201846.307297-1-dev@lankhorst.se?part=25