Re: [PATCH] cxl/mbox: Store event records for EDAC repair regardless of tracing

[Dave Jiang <dave.jiang@intel.com>]

On 9/25/26 3:44 PM, Jonathan Cameron wrote:
> On Thu, 24 Sep 2026 14:23:20 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> The driver throws away media error records unless a tracepoint is enabled.
>>
>> cxl_event_trace_record() stores general media and DRAM records for EDAC
>> repair, but both calls sit inside a block gated on the cxl_general_media
>> and cxl_dram tracepoints. That gate only exists to skip the DPA-to-HPA
>> lookup used to annotate those tracepoints. Storing a record does not need
>> the lookup.
>>
>> With both tracepoints off, two things break. The kernel clears the device
>> event log either way, so the records are destroyed in hardware and kept
>> nowhere else. And live repair stops working altogether: sPPR and memory
>> sparing require the target DPA to have reported an error, no DPA ever has
>> a record, so every request fails with -EINVAL.
> 
> Hi Dave,
> 
> Given userspace is always in that path (I think), without the
> tracepoints being enabled it will be one impressive guess to
> successfully make repair happen - you'll have to magically know
> what address will see a hit in the stored error records.
> 
> Maybe that happens in some test case, but it doesn't feel real
> to me.

If you don't think that's possible then I'll drop the patch.

DJ

> 
>>
>> Move the two stores ahead of the tracepoint check. Keep them under the
>> memdev lock, which is what stops err_rec_free() from freeing
>> cxlmd->err_rec_array while the CPER callback runs. Leave the region and
>> DPA rwsems with the lookup.
>>
>> Fixes: 0b5ccb0de1e2 ("cxl/edac: Support for finding memory operation attributes from the current boot")
> 
> Why the fixes tag?   Maybe this is enabling something new that is
> useful but I'm not immediately understanding what was 'broken' before.
> 
> 
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> Assisted-by: LLM
>> ---
>> Found by reviewing the gate, not from a failure report.
>>
>> The documented flow is unaffected, which is why this has gone unnoticed:
>> rasdaemon watches the tracepoints, so they are on and the records exist
>> (Documentation/edac/memory_repair.rst:94-97). Offline repair is unaffected
>> too, since both checks sit under cxl_is_memdev_memory_online():
>>
>>   edac.c:1807  sPPR     -EINVAL when neither cxl_find_rec_dram() nor
>>                         cxl_find_rec_gen_media() finds a record
>>   edac.c:1357  sparing  -EINVAL when cxl_mem_get_rec_dram() finds none
>>
>> The unconditional log clear is at mbox.c:1115, after the record loop.
>>
>> The memdev lock has to cover the store. err_rec_free() is a devm action on
>> &cxlmd->dev (edac.c:2021) that NULLs err_rec_array and frees it, and the
>> CPER path (cxl/pci.c:1053) can run while cxl_mem unbinds. Hoisting the
>> stores out from under guard(device) would trade the gating bug for a
>> use-after-free.
>>
>> The casts on cxlmd go away because the parameter has not been const since
>> dc372e5f429c ("cxl/pci: Hold memdev lock in cxl_event_trace_record()").
>>
>> Tested in QEMU with cxl_test. kprobe entry counts over the same 220 mock
>> event records, using cxl_event_trace_record() as the control:
>>
>>   tracepoints off      control   store_gen_media   store_dram
>>     before                 220                 0            0
>>     after                  220                22           33
>>
>>   tracepoints on       control   store_gen_media   store_dram
>>     before                 220                22           33
>>     after                  220                22           33
>>
>> Records are kept with tracing off, and the tracing-on path is unchanged -
>> the hoist neither drops nor duplicates a store. The guest log shows
>> cxl_clear_event_record() draining the device log in both cases, so the
>> records really are gone from hardware whether or not the kernel kept them.
>> Builds warning-free with CONFIG_CXL_EDAC_MEM_FEATURES=y and disabled.
>>
>> Not covered: the -EINVAL itself. cxl_test mocks no sPPR or sparing feature,
>> so the repair path cannot be driven end to end.
>>
>> Left alone to keep this minimal: the WARN_ON_ONCE checks on the AP/CME
>> counter-expire descriptor (mbox.c:952-964) stay inside the gate, and are
>> arguably mis-gated for the same reason.
>> ---
>>  drivers/cxl/core/mbox.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index 55828a836c01..89fb2f569e4b 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -913,6 +913,22 @@ void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>>  		return;
>>  	}
>>  
>> +	/*
>> +	 * EDAC memory repair looks these records up from the current boot, so
>> +	 * store them whether or not tracing is enabled. Hold the memdev lock
>> +	 * across the store to keep cxlmd->err_rec_array alive against a
>> +	 * concurrent cxl_mem unbind.
>> +	 */
>> +	guard(device)(&cxlmd->dev);
>> +
>> +	if (event_type == CXL_CPER_EVENT_GEN_MEDIA) {
>> +		if (cxl_store_rec_gen_media(cxlmd, evt))
>> +			dev_dbg(&cxlmd->dev, "CXL store rec_gen_media failed\n");
>> +	} else if (event_type == CXL_CPER_EVENT_DRAM) {
>> +		if (cxl_store_rec_dram(cxlmd, evt))
>> +			dev_dbg(&cxlmd->dev, "CXL store rec_dram failed\n");
>> +	}
>> +
>>  	if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled()) {
>>  		u64 dpa, hpa = ULLONG_MAX, hpa_alias = ULLONG_MAX;
>>  		struct cxl_region *cxlr;
>> @@ -922,7 +938,6 @@ void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>>  		 * translations. Take topology mutation locks and lookup
>>  		 * { HPA, REGION } from { DPA, MEMDEV } in the event record.
>>  		 */
>> -		guard(device)(&cxlmd->dev);
>>  		guard(rwsem_read)(&cxl_rwsem.region);
>>  		guard(rwsem_read)(&cxl_rwsem.dpa);
>>  
>> @@ -937,9 +952,6 @@ void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>>  		}
>>  
>>  		if (event_type == CXL_CPER_EVENT_GEN_MEDIA) {
>> -			if (cxl_store_rec_gen_media((struct cxl_memdev *)cxlmd, evt))
>> -				dev_dbg(&cxlmd->dev, "CXL store rec_gen_media failed\n");
>> -
>>  			if (evt->gen_media.media_hdr.descriptor &
>>  			    CXL_GMER_EVT_DESC_THRESHOLD_EVENT)
>>  				WARN_ON_ONCE((evt->gen_media.media_hdr.type &
>> @@ -952,9 +964,6 @@ void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>>  			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
>>  						hpa_alias, &evt->gen_media);
>>  		} else if (event_type == CXL_CPER_EVENT_DRAM) {
>> -			if (cxl_store_rec_dram((struct cxl_memdev *)cxlmd, evt))
>> -				dev_dbg(&cxlmd->dev, "CXL store rec_dram failed\n");
>> -
>>  			if (evt->dram.media_hdr.descriptor &
>>  			    CXL_GMER_EVT_DESC_THRESHOLD_EVENT)
>>  				WARN_ON_ONCE((evt->dram.media_hdr.type &
>>
>> base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
>