[PATCH v2] drm/i915/guc: Add synchronization on interrupt enable flag

[PATCH v2] drm/i915/guc: Add synchronization on interrupt enable flag

[Zhanjun Dong]

Boolean flag access from interrupt context might have synchronous issueis
on multiple processor platform, flags modified by one core might be read
as an old value by another core. This issue on interrupt enable flag might
causes interrupt misses or leakage.
Change the interrupts.enable type to atomic to ensure memory
synchronization.

Fixes: a187f13d51fa0 ("drm/i915/guc: handle interrupts from media GuC")
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14834
Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
Cc: Andi Shyti <andi.shyti@kernel.org>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

---
Change history:
v2: Add skip on disabled case for tasklet
    Add memory barrier after flag changed
    Add Closes tag and typo fix
---
 drivers/gpu/drm/i915/gt/intel_gt_irq.c    |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc.c    | 17 ++++++++++++-----
 drivers/gpu/drm/i915/gt/uc/intel_guc.h    |  4 ++--
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c |  3 +++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c     |  8 ++++++--
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index 75e802e10be2..21804eec8320 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -20,7 +20,7 @@
 
 static void guc_irq_handler(struct intel_guc *guc, u16 iir)
 {
-	if (unlikely(!guc->interrupts.enabled))
+	if (unlikely(!atomic_read(&guc->interrupts.enabled)))
 		return;
 
 	if (iir & GUC_INTR_GUC2HOST)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index f360f020d8f1..48148cb6cba0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -100,8 +100,9 @@ static void gen9_enable_guc_interrupts(struct intel_guc *guc)
 			 gt->pm_guc_events);
 	gen6_gt_pm_enable_irq(gt, gt->pm_guc_events);
 	spin_unlock_irq(gt->irq_lock);
-
-	guc->interrupts.enabled = true;
+	atomic_set(&guc->interrupts.enabled, true);
+	/* make sure interrupt handler will see changes */
+	smp_mb();
 }
 
 static void gen9_disable_guc_interrupts(struct intel_guc *guc)
@@ -109,7 +110,9 @@ static void gen9_disable_guc_interrupts(struct intel_guc *guc)
 	struct intel_gt *gt = guc_to_gt(guc);
 
 	assert_rpm_wakelock_held(&gt->i915->runtime_pm);
-	guc->interrupts.enabled = false;
+	atomic_set(&guc->interrupts.enabled, false);
+	/* make sure interrupt handler will see changes */
+	smp_mb();
 
 	spin_lock_irq(gt->irq_lock);
 
@@ -146,14 +149,18 @@ static void gen11_enable_guc_interrupts(struct intel_guc *guc)
 	__gen11_reset_guc_interrupts(gt);
 	spin_unlock_irq(gt->irq_lock);
 
-	guc->interrupts.enabled = true;
+	atomic_set(&guc->interrupts.enabled, true);
+	/* make sure interrupt handler will see changes */
+	smp_mb();
 }
 
 static void gen11_disable_guc_interrupts(struct intel_guc *guc)
 {
 	struct intel_gt *gt = guc_to_gt(guc);
 
-	guc->interrupts.enabled = false;
+	atomic_set(&guc->interrupts.enabled, false);
+	/* make sure interrupt handler will see changes */
+	smp_mb();
 	intel_synchronize_irq(gt->i915);
 
 	gen11_reset_guc_interrupts(guc);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 053780f562c1..40242bbb166e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -93,7 +93,7 @@ struct intel_guc {
 
 	/** @interrupts: pointers to GuC interrupt-managing functions. */
 	struct {
-		bool enabled;
+		atomic_t enabled;
 		void (*reset)(struct intel_guc *guc);
 		void (*enable)(struct intel_guc *guc);
 		void (*disable)(struct intel_guc *guc);
@@ -393,7 +393,7 @@ static inline int intel_guc_send_busy_loop(struct intel_guc *guc,
 /* Only call this from the interrupt handler code */
 static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
 {
-	if (guc->interrupts.enabled)
+	if (atomic_read(&guc->interrupts.enabled))
 		intel_guc_ct_event_handler(&guc->ct);
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 380a11c92d63..f0ee3f1235d4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -1326,6 +1326,9 @@ static void ct_try_receive_message(struct intel_guc_ct *ct)
 {
 	int ret;
 
+	if (!atomic_read(&ct_to_guc(ct)->interrupts.enabled))
+		return;
+
 	if (GEM_WARN_ON(!ct->enabled))
 		return;
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 4a3493e8d433..050ea821bb60 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -659,7 +659,9 @@ void intel_uc_runtime_suspend(struct intel_uc *uc)
 	struct intel_guc *guc = &uc->guc;
 
 	if (!intel_guc_is_ready(guc)) {
-		guc->interrupts.enabled = false;
+		atomic_set(&guc->interrupts.enabled, false);
+		/* make sure interrupt handler will see changes */
+		smp_mb();
 		return;
 	}
 
@@ -687,7 +689,9 @@ void intel_uc_suspend(struct intel_uc *uc)
 	wake_up_all_tlb_invalidate(guc);
 
 	if (!intel_guc_is_ready(guc)) {
-		guc->interrupts.enabled = false;
+		atomic_set(&guc->interrupts.enabled, false);
+		/* make sure interrupt handler will see changes */
+		smp_mb();
 		return;
 	}
 
-- 
2.34.1

Re: [PATCH v2] drm/i915/guc: Add synchronization on interrupt enable flag

[Andi Shyti]

Hi Zhanjun,

> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index 75e802e10be2..21804eec8320 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -20,7 +20,7 @@
>  
>  static void guc_irq_handler(struct intel_guc *guc, u16 iir)
>  {
> -	if (unlikely(!guc->interrupts.enabled))
> +	if (unlikely(!atomic_read(&guc->interrupts.enabled)))
>  		return;
>  
>  	if (iir & GUC_INTR_GUC2HOST)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index f360f020d8f1..48148cb6cba0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -100,8 +100,9 @@ static void gen9_enable_guc_interrupts(struct intel_guc *guc)
>  			 gt->pm_guc_events);
>  	gen6_gt_pm_enable_irq(gt, gt->pm_guc_events);
>  	spin_unlock_irq(gt->irq_lock);
> -
> -	guc->interrupts.enabled = true;
> +	atomic_set(&guc->interrupts.enabled, true);
> +	/* make sure interrupt handler will see changes */
> +	smp_mb();

Are we sure we need the barriers here? Can you please explain
better what you are trying to achieve?

My idea of barriers was to use in order to avoid converting
everything into atomic, which doesn't necessarily mean that it's
the best solution, it was just a thought.

But maybe I misunderstood your intention.

Andi

Re: [PATCH v2] drm/i915/guc: Add synchronization on interrupt enable flag

[Dong, Zhanjun]

On 2025-08-25 6:00 a.m., Andi Shyti wrote:
> Hi Zhanjun,
> 
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> index 75e802e10be2..21804eec8320 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> @@ -20,7 +20,7 @@
>>   
>>   static void guc_irq_handler(struct intel_guc *guc, u16 iir)
>>   {
>> -	if (unlikely(!guc->interrupts.enabled))
>> +	if (unlikely(!atomic_read(&guc->interrupts.enabled)))
>>   		return;
>>   
>>   	if (iir & GUC_INTR_GUC2HOST)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> index f360f020d8f1..48148cb6cba0 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -100,8 +100,9 @@ static void gen9_enable_guc_interrupts(struct intel_guc *guc)
>>   			 gt->pm_guc_events);
>>   	gen6_gt_pm_enable_irq(gt, gt->pm_guc_events);
>>   	spin_unlock_irq(gt->irq_lock);
>> -
>> -	guc->interrupts.enabled = true;
>> +	atomic_set(&guc->interrupts.enabled, true);
>> +	/* make sure interrupt handler will see changes */
>> +	smp_mb();
> 
> Are we sure we need the barriers here? Can you please explain
> better what you are trying to achieve?
> 
> My idea of barriers was to use in order to avoid converting
> everything into atomic, which doesn't necessarily mean that it's
> the best solution, it was just a thought.
> 
> But maybe I misunderstood your intention.
> 
> Andi

The barriers seems not needed, the synchronization issue is for 
interrupt.enable only, no extra memory barrier is needed, to be removed 
in next rev.


Regards,
Zhanjun Dong