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

[PATCH v3] 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:
v3: Drop skip on disabled case for tasklet
    Drop memory barrier
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 | 11 +++++++----
 drivers/gpu/drm/i915/gt/uc/intel_guc.h |  4 ++--
 drivers/gpu/drm/i915/gt/uc/intel_uc.c  |  5 +++--
 4 files changed, 13 insertions(+), 9 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..1b8d3bbfa16d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -100,8 +100,8 @@ 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);
+	atomic_set(&guc->interrupts.enabled, true);
 
-	guc->interrupts.enabled = true;
 }
 
 static void gen9_disable_guc_interrupts(struct intel_guc *guc)
@@ -109,7 +109,8 @@ 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);
+
 
 	spin_lock_irq(gt->irq_lock);
 
@@ -146,14 +147,16 @@ 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);
+
 }
 
 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);
+
 	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_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 4a3493e8d433..9d01c3c3d504 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -659,7 +659,8 @@ 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);
+
 		return;
 	}
 
@@ -687,7 +688,7 @@ 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);
 		return;
 	}
 
-- 
2.34.1

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

[Dong, Zhanjun]

The skip on disabled case for tasklet add in v2, effected part:
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;

This part was dropped, caller intel_guc_to_host_event_handler already 
did that check.

Regards,
Zhanjun Dong

On 2025-08-25 5:19 p.m., Zhanjun Dong wrote:
> 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:
> v3: Drop skip on disabled case for tasklet
>      Drop memory barrier
> 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 | 11 +++++++----
>   drivers/gpu/drm/i915/gt/uc/intel_guc.h |  4 ++--
>   drivers/gpu/drm/i915/gt/uc/intel_uc.c  |  5 +++--
>   4 files changed, 13 insertions(+), 9 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..1b8d3bbfa16d 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -100,8 +100,8 @@ 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);
> +	atomic_set(&guc->interrupts.enabled, true);
>   
> -	guc->interrupts.enabled = true;
>   }
>   
>   static void gen9_disable_guc_interrupts(struct intel_guc *guc)
> @@ -109,7 +109,8 @@ 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);
> +
>   
>   	spin_lock_irq(gt->irq_lock);
>   
> @@ -146,14 +147,16 @@ 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);
> +
>   }
>   
>   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);
> +
>   	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_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 4a3493e8d433..9d01c3c3d504 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -659,7 +659,8 @@ 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);
> +
>   		return;
>   	}
>   
> @@ -687,7 +688,7 @@ 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);
>   		return;
>   	}
>   

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

[Dong, Zhanjun]

For ct_try_receive_message, issue happens on tasklet path, not the
caller intel_guc_to_host_event_handler, so sounds like need to add it back.
Due to the low reproduce rate, I Will test it for few days before send 
next rev.

Regards,
Zhanjun Dong

On 2025-08-25 5:33 p.m., Dong, Zhanjun wrote:
> The skip on disabled case for tasklet add in v2, effected part:
> 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;
> 
> This part was dropped, caller intel_guc_to_host_event_handler already 
> did that check.
> 
> Regards,
> Zhanjun Dong
> 
> On 2025-08-25 5:19 p.m., Zhanjun Dong wrote:
>> 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:
>> v3: Drop skip on disabled case for tasklet
>>      Drop memory barrier
>> 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 | 11 +++++++----
>>   drivers/gpu/drm/i915/gt/uc/intel_guc.h |  4 ++--
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c  |  5 +++--
>>   4 files changed, 13 insertions(+), 9 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..1b8d3bbfa16d 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -100,8 +100,8 @@ 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);
>> +    atomic_set(&guc->interrupts.enabled, true);
>> -    guc->interrupts.enabled = true;
>>   }
>>   static void gen9_disable_guc_interrupts(struct intel_guc *guc)
>> @@ -109,7 +109,8 @@ 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);
>> +
>>       spin_lock_irq(gt->irq_lock);
>> @@ -146,14 +147,16 @@ 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);
>> +
>>   }
>>   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);
>> +
>>       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_uc.c b/drivers/gpu/drm/ 
>> i915/gt/uc/intel_uc.c
>> index 4a3493e8d433..9d01c3c3d504 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -659,7 +659,8 @@ 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);
>> +
>>           return;
>>       }
>> @@ -687,7 +688,7 @@ 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);
>>           return;
>>       }
> 

✗ Fi.CI.BUILD: failure for drm/i915/guc: Add synchronization on interrupt enable flag (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Add synchronization on interrupt enable flag (rev4)
URL   : https://patchwork.freedesktop.org/series/153158/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/153158/revisions/4/mbox/ not applied
Applying: drm/i915/guc: Add synchronization on interrupt enable flag
error: git diff header lacks filename information when removing 1 leading pathname component (line 2)
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/i915/guc: Add synchronization on interrupt enable flag
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced