[PATCH] drm/i915/gt: Ensure irqs' status does not change with spin_unlock

[PATCH] drm/i915/gt: Ensure irqs' status does not change with spin_unlock

[Krzysztof Karas]

spin_unlock() function enables irqs regardless of their state
before spin_lock() was called. This might result in an interrupt
while holding a lock further down in the execution, as seen in
GitLab issue #13399.

Try to remedy the problem by saving irq state before spin lock
acquisition.

Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
This issue is hit rarely on CI and I was not able to reproduce
it locally. There might be more places where we should save and
restore irq state, so I am not adding "Closes" label for the
issue yet.

 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 12f1ba7ca9c1..e9102f7246f5 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -4338,10 +4338,11 @@ static void guc_bump_inflight_request_prio(struct i915_request *rq,
 static void guc_retire_inflight_request_prio(struct i915_request *rq)
 {
 	struct intel_context *ce = request_to_scheduling_context(rq);
+	unsigned long flags;
 
-	spin_lock(&ce->guc_state.lock);
+	spin_lock_irqsave(&ce->guc_state.lock, flags);
 	guc_prio_fini(rq, ce);
-	spin_unlock(&ce->guc_state.lock);
+	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 }
 
 static void sanitize_hwsp(struct intel_engine_cs *engine)
-- 
2.34.1

Re: [PATCH] drm/i915/gt: Ensure irqs' status does not change with spin_unlock

[Maciej Patelczyk]

On 10.01.2025 15:08, Krzysztof Karas wrote:

> spin_unlock() function enables irqs regardless of their state
> before spin_lock() was called. This might result in an interrupt
> while holding a lock further down in the execution, as seen in
> GitLab issue #13399.
>
> Try to remedy the problem by saving irq state before spin lock
> acquisition.
>
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
> This issue is hit rarely on CI and I was not able to reproduce
> it locally. There might be more places where we should save and
> restore irq state, so I am not adding "Closes" label for the
> issue yet.
>
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 12f1ba7ca9c1..e9102f7246f5 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -4338,10 +4338,11 @@ static void guc_bump_inflight_request_prio(struct i915_request *rq,
>   static void guc_retire_inflight_request_prio(struct i915_request *rq)
>   {
>   	struct intel_context *ce = request_to_scheduling_context(rq);
> +	unsigned long flags;
>   
> -	spin_lock(&ce->guc_state.lock);
> +	spin_lock_irqsave(&ce->guc_state.lock, flags);
>   	guc_prio_fini(rq, ce);
> -	spin_unlock(&ce->guc_state.lock);
> +	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>   }
>   
>   static void sanitize_hwsp(struct intel_engine_cs *engine)

The guc_retire_inflight_request_prio is called in intel_breadcrumbs.c 
signal_irq_work().

There is a similar situation

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c?h=v6.13-rc7#n255

There is also spin_(un)lock while potentially IRQs are disabled.

Should it also be addressed?

Regards,

Maciej

Re: [PATCH] drm/i915/gt: Ensure irqs' status does not change with spin_unlock

[Krzysztof Karas]

Hi Maciej,

> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 12f1ba7ca9c1..e9102f7246f5 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -4338,10 +4338,11 @@ static void guc_bump_inflight_request_prio(struct i915_request *rq,
> >   static void guc_retire_inflight_request_prio(struct i915_request *rq)
> >   {
> >   	struct intel_context *ce = request_to_scheduling_context(rq);
> > +	unsigned long flags;
> > -	spin_lock(&ce->guc_state.lock);
> > +	spin_lock_irqsave(&ce->guc_state.lock, flags);
> >   	guc_prio_fini(rq, ce);
> > -	spin_unlock(&ce->guc_state.lock);
> > +	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
> >   }
> >   static void sanitize_hwsp(struct intel_engine_cs *engine)
> 
> The guc_retire_inflight_request_prio is called in intel_breadcrumbs.c
> signal_irq_work().
> 
> There is a similar situation
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c?h=v6.13-rc7#n255
> 
> There is also spin_(un)lock while potentially IRQs are disabled.
> 
> Should it also be addressed?
Thanks for spotting this. Yes, I believe we should also address
other spin locks/unlocks on this path
(inside list_for_each_entry_rcu loops for example), as they
might cause similar problems. I will include these in v2.

Krzysztof
> 
> Regards,
> 
> Maciej
>