Re: [PATCH v18 02/13] acpi/apei/ghes: Use raw_spinlock_t for CXL CPER work locks

On 7/20/2026 3:12 PM, Dave Jiang wrote:
> 
> 
> On 7/17/26 3:26 PM, Terry Bowman wrote:
>> The CXL CPER work registration and unregistration helpers in
>> drivers/acpi/apei/ghes.c acquire cxl_cper_work_lock and
>> cxl_cper_prot_err_work_lock with guard(spinlock), which leaves local
>> interrupts enabled. The corresponding post paths
>> (cxl_cper_post_event(), cxl_cper_post_prot_err()) execute in hard IRQ
>> context (they are called from the GHES error notification path) and
>> acquire the same locks via guard(spinlock_irqsave).
>>
>> If a CPU is holding one of these locks via guard(spinlock) when a
>> GHES interrupt arrives on the same CPU, the IRQ handler spins on the
>> held lock waiting for it to release, while the lock holder is
>> preempted by the IRQ. The result is a deadlock.
>>
>> Convert both locks from spinlock_t to raw_spinlock_t and use
>> guard(raw_spinlock_irqsave) at all call sites. On PREEMPT_RT kernels
>> spinlock_t is backed by rt_mutex and sleeping from hard IRQ context is
>> not permitted; raw_spinlock_t is safe in both contexts.
>>
>> Add WARN_ONCE to both register functions to surface double-registration
>> bugs at runtime.
>>
>> Restructure both unregister functions to clear the global work pointer
>> under the lock before calling cancel_work_sync(), closing the window
>> where a CPER interrupt could schedule work on a pointer about to be
>> freed. Add kfifo_reset() after cancel_work_sync() so stale entries
>> are not replayed on next module load.
>>
>> Both kfifos are single-consumer: only one work_struct is registered at
>> a time, enforced by the WARN_ONCE guard in the register functions.
>> kfifo_reset() is safe outside the lock because cancel_work_sync() has
>> already quiesced the consumer, and no new consumer can register until
>> the current module exit completes and a fresh module init runs.
>>
>> Remove the now-redundant cancel_work_sync() call from
>> cxl_pci_driver_exit() - cxl_cper_unregister_work() handles quiescing
>> internally.
>>
>> Reported-by: Sashiko <[email protected]>
>> Signed-off-by: Terry Bowman <[email protected]>
>> Fixes: 5e4a264bf8b5 ("acpi/ghes: Process CXL Component Events")
>> Fixes: 36f257e3b0ba ("acpi/ghes, cxl/pci: Process CXL CPER Protocol Errors")
>> Cc: [email protected]
> 
> With the minor sashiko issue addressed,
> Reviewed-by: Dave Jiang <[email protected]>
> 
> This can probably be picked up ahead of the series.
> 
> 

Yes.

-Terry

>>
>> ---
>>
>> Changes in v17 -> v18:
>> - New patch.
>> ---
>>  drivers/acpi/apei/ghes.c | 50 ++++++++++++++++++++++++++--------------
>>  drivers/cxl/pci.c        |  1 -
>>  2 files changed, 33 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 3236a3ce79d6b..ca7a138c1ff2e 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -749,7 +749,7 @@ static DEFINE_KFIFO(cxl_cper_prot_err_fifo, struct cxl_cper_prot_err_work_data,
>>  		    CXL_CPER_PROT_ERR_FIFO_DEPTH);
>>  
>>  /* Synchronize schedule_work() with cxl_cper_prot_err_work changes */
>> -static DEFINE_SPINLOCK(cxl_cper_prot_err_work_lock);
>> +static DEFINE_RAW_SPINLOCK(cxl_cper_prot_err_work_lock);
>>  struct work_struct *cxl_cper_prot_err_work;
>>  
>>  static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
>> @@ -761,7 +761,7 @@ static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
>>  	if (cxl_cper_sec_prot_err_valid(prot_err))
>>  		return;
>>  
>> -	guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
>> +	guard(raw_spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
>>  
>>  	if (!cxl_cper_prot_err_work)
>>  		return;
>> @@ -780,10 +780,11 @@ static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
>>  
>>  int cxl_cper_register_prot_err_work(struct work_struct *work)
>>  {
>> -	if (cxl_cper_prot_err_work)
>> -		return -EINVAL;
>> +	guard(raw_spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
>>  
>> -	guard(spinlock)(&cxl_cper_prot_err_work_lock);
>> +	if (WARN_ONCE(cxl_cper_prot_err_work,
>> +		      "CPER-CXL kfifo consumer already registered\n"))
>> +		return -EINVAL;
>>  	cxl_cper_prot_err_work = work;
>>  	return 0;
>>  }
>> @@ -791,11 +792,18 @@ EXPORT_SYMBOL_NS_GPL(cxl_cper_register_prot_err_work, "CXL");
>>  
>>  int cxl_cper_unregister_prot_err_work(struct work_struct *work)
>>  {
>> -	if (cxl_cper_prot_err_work != work)
>> -		return -EINVAL;
>> +	scoped_guard(raw_spinlock_irqsave, &cxl_cper_prot_err_work_lock) {
>> +		if (WARN_ONCE(cxl_cper_prot_err_work != work,
>> +			      "CPER-CXL kfifo consumer mismatch on unregister\n"))
>> +			return -EINVAL;
>> +		cxl_cper_prot_err_work = NULL;
>> +	}
>> +
>> +	cancel_work_sync(work);
>> +
>> +	/* Discard stale entries so they are not replayed on next module load */
>> +	kfifo_reset(&cxl_cper_prot_err_fifo);
>>  
>> -	guard(spinlock)(&cxl_cper_prot_err_work_lock);
>> -	cxl_cper_prot_err_work = NULL;
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_prot_err_work, "CXL");
>> @@ -811,7 +819,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_cper_prot_err_kfifo_get, "CXL");
>>  DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, CXL_CPER_FIFO_DEPTH);
>>  
>>  /* Synchronize schedule_work() with cxl_cper_work changes */
>> -static DEFINE_SPINLOCK(cxl_cper_work_lock);
>> +static DEFINE_RAW_SPINLOCK(cxl_cper_work_lock);
>>  struct work_struct *cxl_cper_work;
>>  
>>  static void cxl_cper_post_event(enum cxl_event_type event_type,
>> @@ -831,7 +839,7 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
>>  		return;
>>  	}
>>  
>> -	guard(spinlock_irqsave)(&cxl_cper_work_lock);
>> +	guard(raw_spinlock_irqsave)(&cxl_cper_work_lock);
>>  
>>  	if (!cxl_cper_work)
>>  		return;
>> @@ -849,10 +857,11 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
>>  
>>  int cxl_cper_register_work(struct work_struct *work)
>>  {
>> -	if (cxl_cper_work)
>> +	guard(raw_spinlock_irqsave)(&cxl_cper_work_lock);
>> +	if (WARN_ONCE(cxl_cper_work,
>> +		      "CXL CPER kfifo consumer already registered\n"))
>>  		return -EINVAL;
>>  
>> -	guard(spinlock)(&cxl_cper_work_lock);
>>  	cxl_cper_work = work;
>>  	return 0;
>>  }
>> @@ -860,11 +869,18 @@ EXPORT_SYMBOL_NS_GPL(cxl_cper_register_work, "CXL");
>>  
>>  int cxl_cper_unregister_work(struct work_struct *work)
>>  {
>> -	if (cxl_cper_work != work)
>> -		return -EINVAL;
>> +	scoped_guard(raw_spinlock_irqsave, &cxl_cper_work_lock) {
>> +		if (WARN_ONCE(cxl_cper_work != work,
>> +			      "CXL CPER kfifo consumer mismatch on unregister\n"))
>> +			return -EINVAL;
>> +		cxl_cper_work = NULL;
>> +	}
>> +
>> +	cancel_work_sync(work);
>> +
>> +	/* Discard stale entries so they are not replayed on next module load */
>> +	kfifo_reset(&cxl_cper_fifo);
>>  
>> -	guard(spinlock)(&cxl_cper_work_lock);
>> -	cxl_cper_work = NULL;
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_work, "CXL");
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 267c679b0b3c2..7c6faee7f85ed 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -1083,7 +1083,6 @@ static int __init cxl_pci_driver_init(void)
>>  static void __exit cxl_pci_driver_exit(void)
>>  {
>>  	cxl_cper_unregister_work(&cxl_cper_work);
>> -	cancel_work_sync(&cxl_cper_work);
>>  	pci_unregister_driver(&cxl_pci_driver);
>>  }
>>  
>