Fix infinite loop when draining a stuck event log

Fix infinite loop when draining a stuck event log

[Anisa Su]

Based on the 7.2 tag.

While testing the DCD patchset on a real device, I encountered a scenario where
bit 4 of the Event Status Register, corresponding to the DC Event Log
(CXL r4.0 8.2.9.3.1 Table 8-203) is not cleared by the device after a DC event is
handled and the Clear Event Records (0x0101) command is sent.

This causes cxl_event_thread() to spin in an infinite loop because cxl_event_thread()
drains every log in the Event Status register and repeats until that register reads zero.
So the thread ends up resending infinite Get Event Records (0x0100) as fast as
the mailbox completes, even though the event log remains empty.

Although I encountered this specifically while testing DCD, this would apply
equally to other types of events (Informational, Warning, Failure, Fatal).

This patch bounds the loop on there being > 0 event records in the event
log. cxl_mem_get_records_log() and cxl_mem_get_event_records() now report failures
and which logs returned records; the thread stops on the first error and
drops any log whose status bit was set while it returned nothing. Every pass
either drains a record or clears a bit from a five-bit mask, so exiting the loop
no longer depends only on the status bit reported by the device. 
Logs are drained best effort so a broken one cannot suppress reporting from the rest.


Testing:

ndctl's cxl suite (pmem/ndctl pending, 02754b5) against cxl_test on
v7.2 plus this patch, 15 passed / 2 skipped / 0 failed.  

On HW:
Without the fix, we see an infinite loop:

[  452.430234] cxl_pci 0000:0b:00.0: DBG drain_mask: native_cxl_error=0 dcd_supported=1 mask=0x10
[  452.430248] cxl_pci 0000:0b:00.0: DBG event_thread: enter irq=236 mask=0x10
[  452.430254] cxl_pci 0000:0b:00.0: DBG event_thread: status=0x10 masked=0x10
^ Host receives DC interrupt

[  452.430258] cxl_pci 0000:0b:00.0: DBG get_event_records: status=0x10 dcd_supported=1
[  452.430262] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 size_out=1024
[  452.430268] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 size_in=1 doorbell_busy=0
[  452.430274] cxl_pci 0000:0b:00.0: Sending command: 0x0100
[  452.430309] cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
[  452.430313] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 doorbell cleared, dev rc=0
[  452.430345] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 nr_rec=1 flags=0x0
^ Get Event Record retrieves DC Add Event from event log

[  452.430355] cxl_pci 0000:0b:00.0: DCD event add : DPA:0x0 LEN:0x80000000
[  452.430363] cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430374] cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430381] cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430390] cxl decoder9.0: dpa:0x0 mapped in region:region0
[  452.430397] cxl decoder9.0: Add extent [range 0x0000000000000000-0x000000007fffffff] (e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430403] cxl_dax_region dax_region0: Alloc new tag group
[  452.430437]  extent0.1: dc_extent HPA [range 0x0000000000000000-0x000000007fffffff] (e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430444] cxl_dax_region dax_region0: Trying notify: type 0 tag e5407b12-9a68-4c31-8d5e-2f0916bd73a4
[  452.430449] cxl_dax_region dax_region0: Notify: type 0 tag e5407b12-9a68-4c31-8d5e-2f0916bd73a4
^ DC Add Event handled

[  452.430473] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x4802 size_in=32 doorbell_busy=0
[  452.430482] cxl_pci 0000:0b:00.0: Sending command: 0x4802
[  452.430658] cxl_pci 0000:0b:00.0: Doorbell wait took 1ms
[  452.430662] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x4802 doorbell cleared, dev rc=0
^ DC Add Response sent

[  452.430669] cxl_pci 0000:0b:00.0: Event log '4': Clearing 1
[  452.430674] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x101 size_in=8 doorbell_busy=0
[  452.430679] cxl_pci 0000:0b:00.0: Sending command: 0x0101
[  452.430705] cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
[  452.430708] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x101 doorbell cleared, dev rc=0
[  452.430712] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 cleared 1 recs rc=0
[  452.430717] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 size_out=1024
^ Clear Event Records succeeds

Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Reading event logs: 10
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x0100
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: DCD event add : DPA:0x0 LEN:0x80000000
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
Aug 26 06:30:56 cxlqual kernel: cxl decoder9.0: dpa:0x0 mapped in region:region0
Aug 26 06:30:56 cxlqual kernel: cxl decoder9.0: Add extent [range 0x0000000000000000-0x000000007fffffff] (e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
Aug 26 06:30:56 cxlqual kernel: cxl_dax_region dax_region0: Alloc new tag group
Aug 26 06:30:56 cxlqual kernel:  extent0.1: dc_extent HPA [range 0x0000000000000000-0x000000007fffffff] (e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
Aug 26 06:30:56 cxlqual kernel: cxl_dax_region dax_region0: Trying notify: type 0 tag e5407b12-9a68-4c31-8d5e-2f0916bd73a4
Aug 26 06:30:56 cxlqual kernel: cxl_dax_region dax_region0: Notify: type 0 tag e5407b12-9a68-4c31-8d5e-2f0916bd73a4
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x4802
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
^ DC Add Response succeeds

Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Event log '4': Clearing 1
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x0101
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 1ms
^ Clear Event Records succeeds

Then loops infinitely:
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x0100
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Reading event logs: 10
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x0100
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Reading event logs: 10
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x0100
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Reading event logs: 10
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x0100
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Reading event logs: 10
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Sending command: 0x0100
Aug 26 06:30:56 cxlqual kernel: cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
...

To determine the cause, I added print statements to check the status bit and saw:

[  452.430753] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 doorbell cleared, dev rc=0
[  452.430764] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 nr_rec=0 flags=0x0
^ ---
nr_rec = 0, so the event log is empty

[  452.430768] cxl_pci 0000:0b:00.0: DBG get_event_records: status=0x10 drained=1 ret=0
^ ---
But the status bit = 0x10, which is the lower 4th bit of the Event Status Register,
corresponding to the Dynamic Capacity Event Log

With the patch (the extra dbg messages are not in this patch, they're just for verifying
the fix):

[  452.430234] cxl_pci 0000:0b:00.0: DBG drain_mask: native_cxl_error=0 dcd_supported=1 mask=0x10
[  452.430248] cxl_pci 0000:0b:00.0: DBG event_thread: enter irq=236 mask=0x10
[  452.430254] cxl_pci 0000:0b:00.0: DBG event_thread: status=0x10 masked=0x10
[  452.430258] cxl_pci 0000:0b:00.0: DBG get_event_records: status=0x10 dcd_supported=1
[  452.430262] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 size_out=1024
[  452.430268] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 size_in=1 doorbell_busy=0
[  452.430274] cxl_pci 0000:0b:00.0: Sending command: 0x0100
[  452.430309] cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
[  452.430313] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 doorbell cleared, dev rc=0
[  452.430345] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 nr_rec=1 flags=0x0
[  452.430355] cxl_pci 0000:0b:00.0: DCD event add : DPA:0x0 LEN:0x80000000
[  452.430363] cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430374] cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430381] cxl_pci 0000:0b:00.0: DC extent DPA [range 0x0000000000000000-0x000000007fffffff] (DCR:[range 0x0000000000000000-0x0000001fffffffff])(e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430390] cxl decoder9.0: dpa:0x0 mapped in region:region0
[  452.430397] cxl decoder9.0: Add extent [range 0x0000000000000000-0x000000007fffffff] (e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430403] cxl_dax_region dax_region0: Alloc new tag group
[  452.430437]  extent0.1: dc_extent HPA [range 0x0000000000000000-0x000000007fffffff] (e5407b12-9a68-4c31-8d5e-2f0916bd73a4)
[  452.430444] cxl_dax_region dax_region0: Trying notify: type 0 tag e5407b12-9a68-4c31-8d5e-2f0916bd73a4
[  452.430449] cxl_dax_region dax_region0: Notify: type 0 tag e5407b12-9a68-4c31-8d5e-2f0916bd73a4
[  452.430473] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x4802 size_in=32 doorbell_busy=0
[  452.430482] cxl_pci 0000:0b:00.0: Sending command: 0x4802
[  452.430658] cxl_pci 0000:0b:00.0: Doorbell wait took 1ms
[  452.430662] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x4802 doorbell cleared, dev rc=0
[  452.430669] cxl_pci 0000:0b:00.0: Event log '4': Clearing 1
[  452.430674] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x101 size_in=8 doorbell_busy=0
[  452.430679] cxl_pci 0000:0b:00.0: Sending command: 0x0101
[  452.430705] cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
[  452.430708] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x101 doorbell cleared, dev rc=0
[  452.430712] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 cleared 1 recs rc=0
[  452.430717] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 size_out=1024
[  452.430721] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 size_in=1 doorbell_busy=0
[  452.430725] cxl_pci 0000:0b:00.0: Sending command: 0x0100
[  452.430749] cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
[  452.430753] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 doorbell cleared, dev rc=0
[  452.430764] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 nr_rec=0 flags=0x0
[  452.430768] cxl_pci 0000:0b:00.0: DBG get_event_records: status=0x10 drained=1 ret=0
[  452.430772] cxl_pci 0000:0b:00.0: DBG event_thread: drain status=0x10 drained=1 rc=0
[  452.430777] cxl_pci 0000:0b:00.0: DBG event_thread: status=0x10 masked=0x10
[  452.430781] cxl_pci 0000:0b:00.0: DBG get_event_records: status=0x10 dcd_supported=1
[  452.430784] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 size_out=1024
[  452.430788] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 size_in=1 doorbell_busy=0
[  452.430794] cxl_pci 0000:0b:00.0: Sending command: 0x0100
[  452.430820] cxl_pci 0000:0b:00.0: Doorbell wait took 0ms
[  452.430823] cxl_pci 0000:0b:00.0: DBG mbox: opcode=0x100 doorbell cleared, dev rc=0
[  452.430833] cxl_pci 0000:0b:00.0: DBG get_records_log: type=4 nr_rec=0 flags=0x0
[  452.430837] cxl_pci 0000:0b:00.0: DBG get_event_records: status=0x10 drained=0 ret=0
[  452.430840] cxl_pci 0000:0b:00.0: DBG event_thread: drain status=0x10 drained=0 rc=0
[  452.430844] cxl_pci 0000:0b:00.0: Event status 0x10 set with no records to read

^ --- detected that the status bit was set but the event log is empty and stops here


Anisa Su (1):
  cxl/events: Bound the event log drain loop

 drivers/cxl/core/mbox.c      | 58 ++++++++++++++++++++++++++++--------
 drivers/cxl/cxlmem.h         |  3 +-
 drivers/cxl/pci.c            | 39 +++++++++++++++++++-----
 tools/testing/cxl/test/mem.c |  4 +--
 4 files changed, 81 insertions(+), 23 deletions(-)


base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
2.43.0

[PATCH 1/1] cxl/events: Bound the event log drain loop

[Anisa Su]

cxl_event_thread() drains every log named in the Event Status register and
repeats until that register reads zero.  CXL 8.2.9.3.1 Table 8-203 marks
the register RO and leaves clearing it to the device, so a misbehaving device
that leaves a status bit set spins the thread forever, resending
Get Event Records as fast as the mailbox completes.

Bound the loop on work completed instead.  cxl_mem_get_records_log() and
cxl_mem_get_event_records() report failures and which logs returned
records; the thread stops on the first error and drops any log whose status
bit was set while it returned nothing.  Every pass either drains a record
or clears a bit from the mask, so the loop terminates whatever the device
reports.  Logs are drained best effort, so a broken one does not suppress
reporting from the rest.

Fixes: a49aa8141b65 ("cxl/mem: Wire up event interrupts")
Cc: <stable@vger.kernel.org>
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
 drivers/cxl/core/mbox.c      | 58 ++++++++++++++++++++++++++++--------
 drivers/cxl/cxlmem.h         |  3 +-
 drivers/cxl/pci.c            | 39 +++++++++++++++++++-----
 tools/testing/cxl/test/mem.c |  4 +--
 4 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 7c6c5b7450a5..5954ba20f0be 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1059,8 +1059,8 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 	return rc;
 }
 
-static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
-				    enum cxl_event_log_type type)
+static int cxl_mem_get_records_log(struct cxl_memdev_state *mds,
+				   enum cxl_event_log_type type, bool *drained)
 {
 	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
 	struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
@@ -1068,12 +1068,15 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 	struct cxl_get_event_payload *payload;
 	u8 log_type = type;
 	u16 nr_rec;
+	int rc = 0;
+
+	*drained = false;
 
 	mutex_lock(&mds->event.log_lock);
 	payload = mds->event.buf;
 
 	do {
-		int rc, i;
+		int i;
 		struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd) {
 			.opcode = CXL_MBOX_OP_GET_EVENT_RECORD,
 			.payload_in = &log_type,
@@ -1094,6 +1097,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 		nr_rec = le16_to_cpu(payload->record_count);
 		if (!nr_rec)
 			break;
+		*drained = true;
 
 		for (i = 0; i < nr_rec; i++)
 			__cxl_event_trace_record(cxlmd, type,
@@ -1112,31 +1116,59 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 	} while (nr_rec);
 
 	mutex_unlock(&mds->event.log_lock);
+
+	return rc;
 }
 
 /**
  * cxl_mem_get_event_records - Get Event Records from the device
  * @mds: The driver data for the operation
  * @status: Event Status register value identifying which events are available.
+ * @drained: Optional mask of the logs in @status that returned records.
  *
  * Retrieve all event records available on the device, report them as trace
- * events, and clear them.
+ * events, and clear them.  Every log named in @status is drained even if
+ * another one fails, so that a broken log does not suppress reporting from
+ * the rest.
+ *
+ * Return: 0, or the first error encountered.
  *
  * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
  * See CXL rev 3.0 @8.2.9.2.3 Clear Event Records
  */
-void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status)
+int cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status,
+			      u32 *drained)
 {
+	static const struct {
+		u32 status;
+		enum cxl_event_log_type type;
+	} logs[] = {
+		{ CXLDEV_EVENT_STATUS_FATAL, CXL_EVENT_TYPE_FATAL },
+		{ CXLDEV_EVENT_STATUS_FAIL, CXL_EVENT_TYPE_FAIL },
+		{ CXLDEV_EVENT_STATUS_WARN, CXL_EVENT_TYPE_WARN },
+		{ CXLDEV_EVENT_STATUS_INFO, CXL_EVENT_TYPE_INFO },
+	};
+	int ret = 0;
+
 	dev_dbg(mds->cxlds.dev, "Reading event logs: %x\n", status);
 
-	if (status & CXLDEV_EVENT_STATUS_FATAL)
-		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FATAL);
-	if (status & CXLDEV_EVENT_STATUS_FAIL)
-		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FAIL);
-	if (status & CXLDEV_EVENT_STATUS_WARN)
-		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_WARN);
-	if (status & CXLDEV_EVENT_STATUS_INFO)
-		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_INFO);
+	if (drained)
+		*drained = 0;
+
+	for (int i = 0; i < ARRAY_SIZE(logs); i++) {
+		bool got_records;
+		int rc;
+
+		if (!(status & logs[i].status))
+			continue;
+
+		rc = cxl_mem_get_records_log(mds, logs[i].type, &got_records);
+		if (got_records && drained)
+			*drained |= logs[i].status;
+		ret = ret ?: rc;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL");
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ed419d0c59f2..cb5372eb2d85 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -803,7 +803,8 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 				unsigned long *cmds);
 void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 				  unsigned long *cmds);
-void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
+int cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status,
+			      u32 *drained);
 void cxl_event_trace_record(struct cxl_memdev *cxlmd,
 			    enum cxl_event_log_type type,
 			    enum cxl_event_type event_type,
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 267c679b0b3c..3daf17835c06 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -514,21 +514,46 @@ static irqreturn_t cxl_event_thread(int irq, void *id)
 	struct cxl_dev_id *dev_id = id;
 	struct cxl_dev_state *cxlds = dev_id->cxlds;
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
-	u32 status;
+	u32 mask = CXLDEV_EVENT_STATUS_ALL;
+
+	while (mask) {
+		u32 status, drained, stuck;
+		int rc;
 
-	do {
 		/*
 		 * CXL 3.0 8.2.8.3.1: The lower 32 bits are the status;
 		 * ignore the reserved upper 32 bits
 		 */
 		status = readl(cxlds->regs.status + CXLDEV_DEV_EVENT_STATUS_OFFSET);
-		/* Ignore logs unknown to the driver */
-		status &= CXLDEV_EVENT_STATUS_ALL;
+		/* Ignore logs unknown to the driver, and logs given up on */
+		status &= mask;
 		if (!status)
 			break;
-		cxl_mem_get_event_records(mds, status);
+
+		/*
+		 * A failed drain leaves the log's status bit set, so another
+		 * pass would resend the same query forever.
+		 */
+		rc = cxl_mem_get_event_records(mds, status, &drained);
+		if (rc)
+			break;
+
+		/*
+		 * The Event Status register is device owned and read only, so
+		 * it cannot bound this loop; records read can.  A device that
+		 * leaves a bit set with an empty log makes no progress, and
+		 * only the device can clear that state, so stop polling that
+		 * log rather than spin on it.
+		 */
+		stuck = status & ~drained;
+		if (stuck) {
+			dev_warn_once(cxlds->dev,
+				      "Event status %#x set with no records to read\n",
+				      stuck);
+			mask &= ~stuck;
+		}
 		cond_resched();
-	} while (status);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -682,7 +707,7 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
 	if (rc)
 		return rc;
 
-	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
+	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL, NULL);
 
 	return 0;
 }
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index a7da279aa3ef..87e01612cea9 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -372,7 +372,7 @@ static void cxl_mock_event_trigger(struct device *dev)
 			event_reset_log(log);
 	}
 
-	cxl_mem_get_event_records(mdata->mds, mes->ev_status);
+	cxl_mem_get_event_records(mdata->mds, mes->ev_status, NULL);
 }
 
 struct cxl_event_record_raw maint_needed = {
@@ -1805,7 +1805,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (rc)
 		dev_dbg(dev, "No CXL FWCTL setup\n");
 
-	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
+	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL, NULL);
 	cxl_mock_test_feat_init(mdata);
 
 	return 0;
-- 
2.43.0

Re: [PATCH 1/1] cxl/events: Bound the event log drain loop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] TOCTOU race during IRQ setup causes spurious warnings and alters thread polling logic

Pre-existing issues:
- [Critical] Out-of-bounds Read & Information Leak via unvalidated record_count
- [High] Unbounded inner loop allows CXL device to cause soft lockup
- [High] Unconditional IRQ_HANDLED return defeats spurious interrupt detection
--

commit bfe23a43bdb497b440f694e603125e0a9dc270f5
Author: Anisa Su <anisa.su887@gmail.com>

cxl/events: Bound the event log drain loop

This commit bounds the event log drain loop based on the work completed
instead of waiting for the Event Status register to clear. It modifies
cxl_event_thread() to stop on the first error and drop any log that returns
no records while its status bit is set. The goal is to prevent infinite
spinning when a misbehaving device fails to clear the status bits.

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 55828a836c01a..ca2114a17ff4f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c

[ ... ]

> @@ -1070,12 +1070,15 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	struct cxl_get_event_payload *payload;
>  	u8 log_type = type;
>  	u16 nr_rec;
> +	int rc = 0;
> +
> +	*drained = false;
>  
>  	mutex_lock(&mds->event.log_lock);
>  	payload = mds->event.buf;
>  
>  	do {
> -		int rc, i;
> +		int i;
>  		struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd) {
>  			.opcode = CXL_MBOX_OP_GET_EVENT_RECORD,
>  			.payload_in = &log_type,
> @@ -1096,6 +1099,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		nr_rec = le16_to_cpu(payload->record_count);
>  		if (!nr_rec)
>  			break;
> +		*drained = true;

[Severity: Critical]
This is a pre-existing issue, but does this code properly validate the
hardware-provided payload->record_count?

In cxl_mem_get_records_log(), the nr_rec variable is read directly from
payload->record_count without any bounds checking against the payload buffer
capacity. The loop below then uses this untrusted count to read from the
flexible array payload->records[i]:

        for (i = 0; i < nr_rec; i++)
                __cxl_event_trace_record(cxlmd, type,
                                         &payload->records[i]);

A maliciously or erroneously large record_count from the device would cause an
out-of-bounds read of kernel memory, leaking it to tracepoints.

Additionally, this same unvalidated count is used later in
cxl_clear_event_record():

        u16 total = le16_to_cpu(get_pl->record_count);
        ...
        for (cnt = 0; cnt < total; cnt++) {
                struct cxl_event_record_raw *raw = &get_pl->records[cnt];
                struct cxl_event_generic *gen = &raw->event.generic;

                payload->handles[i++] = gen->hdr.handle;

This reads out-of-bounds kernel memory and sends it to the physical device via
the CLEAR_EVENT_RECORD command.

>  
>  		for (i = 0; i < nr_rec; i++)
>  			__cxl_event_trace_record(cxlmd, type,

[ ... ]

> @@ -1114,31 +1118,59 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	} while (nr_rec);

[Severity: High]
This is a pre-existing issue, but while this patch bounds the outer event log
drain loop in cxl_event_thread(), does it overlook this inner loop?

The do/while loop in cxl_mem_get_records_log() executes unconditionally as long
as the device returns a non-zero record_count. A misbehaving or malicious
device that continuously returns a payload with record_count > 0 will trap the
kernel thread in this loop indefinitely. This bypasses the new bounds checks
introduced in the outer loop, potentially causing a CPU soft lockup.

>  
>  	mutex_unlock(&mds->event.log_lock);
> +
> +	return rc;
>  }

[ ... ]

> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index c7c91e8dc51dc..9339f37cd6620 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -515,21 +515,46 @@ static irqreturn_t cxl_event_thread(int irq, void *id)
>  	struct cxl_dev_id *dev_id = id;
>  	struct cxl_dev_state *cxlds = dev_id->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	u32 status;
> +	u32 mask = CXLDEV_EVENT_STATUS_ALL;
> +
> +	while (mask) {
> +		u32 status, drained, stuck;
> +		int rc;
>  
> -	do {
>  		/*
>  		 * CXL 3.0 8.2.8.3.1: The lower 32 bits are the status;
>  		 * ignore the reserved upper 32 bits
>  		 */
>  		status = readl(cxlds->regs.status + CXLDEV_DEV_EVENT_STATUS_OFFSET);
> -		/* Ignore logs unknown to the driver */
> -		status &= CXLDEV_EVENT_STATUS_ALL;
> +		/* Ignore logs unknown to the driver, and logs given up on */
> +		status &= mask;
>  		if (!status)
>  			break;
> -		cxl_mem_get_event_records(mds, status);
> +
> +		/*
> +		 * A failed drain leaves the log's status bit set, so another
> +		 * pass would resend the same query forever.
> +		 */
> +		rc = cxl_mem_get_event_records(mds, status, &drained);
> +		if (rc)
> +			break;
> +
> +		/*
> +		 * The Event Status register is device owned and read only, so
> +		 * it cannot bound this loop; records read can.  A device that
> +		 * leaves a bit set with an empty log makes no progress, and
> +		 * only the device can clear that state, so stop polling that
> +		 * log rather than spin on it.
> +		 */
> +		stuck = status & ~drained;
> +		if (stuck) {
> +			dev_warn_once(cxlds->dev,
> +				      "Event status %#x set with no records to read\n",
> +				      stuck);
> +			mask &= ~stuck;
> +		}

[Severity: Medium]
Can this new logic cause spurious warnings and incorrectly stop polling due to
a race with the IRQ setup?

In cxl_event_config(), interrupts are enabled via cxl_event_irqsetup(), and
then cxl_mem_get_event_records() is explicitly called to drain existing logs:

        rc = cxl_event_irqsetup(mds);
        if (rc)
                return rc;

        cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL, NULL);

If an interrupt fires immediately after IRQs are enabled but before the
explicit drain, cxl_event_thread() will read the event status register and
might get preempted. While preempted, the probe code completes the drain and
clears the hardware status. When cxl_event_thread() resumes, it calls
cxl_mem_get_event_records() with its now-stale status. The function will
return no records (drained == 0), causing this logic to incorrectly conclude
the device is stuck, emit the dev_warn_once() warning, and stop polling
those logs.

>  		cond_resched();
> -	} while (status);
> +	}
>  
>  	return IRQ_HANDLED;

[Severity: High]
This is a pre-existing issue, but should this threaded IRQ handler return
IRQ_NONE if it didn't process any events?

If the handler is woken up but the status register reads 0 on the first
iteration (e.g., due to a spurious interrupt), the loop breaks immediately and
unconditionally returns IRQ_HANDLED.

Returning IRQ_HANDLED when no work was done prevents the kernel's core IRQ
subsystem from detecting and disabling a spurious interrupt storm. If a
failing CXL device floods the CPU with MSI interrupts, the kernel will never
disable the vector, which could completely lock up the processing core.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826184417.1042-1-anisa.su@samsung.com?part=1

Re: [PATCH 1/1] cxl/events: Bound the event log drain loop

[Cheatham, Benjamin]

On 8/26/2026 1:43 PM, Anisa Su wrote:
> cxl_event_thread() drains every log named in the Event Status register and
> repeats until that register reads zero.  CXL 8.2.9.3.1 Table 8-203 marks
> the register RO and leaves clearing it to the device, so a misbehaving device
> that leaves a status bit set spins the thread forever, resending
> Get Event Records as fast as the mailbox completes.
> 
> Bound the loop on work completed instead.  cxl_mem_get_records_log() and
> cxl_mem_get_event_records() report failures and which logs returned
> records; the thread stops on the first error and drops any log whose status
> bit was set while it returned nothing.  Every pass either drains a record
> or clears a bit from the mask, so the loop terminates whatever the device
> reports.  Logs are drained best effort, so a broken one does not suppress
> reporting from the rest.
> 
> Fixes: a49aa8141b65 ("cxl/mem: Wire up event interrupts")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---

One nit below, but looks good to me regardless so:
Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
>  drivers/cxl/core/mbox.c      | 58 ++++++++++++++++++++++++++++--------
>  drivers/cxl/cxlmem.h         |  3 +-
>  drivers/cxl/pci.c            | 39 +++++++++++++++++++-----
>  tools/testing/cxl/test/mem.c |  4 +--
>  4 files changed, 81 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 7c6c5b7450a5..5954ba20f0be 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1059,8 +1059,8 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  	return rc;
>  }
>  
> -static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> -				    enum cxl_event_log_type type)
> +static int cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> +				   enum cxl_event_log_type type, bool *drained)

Nit: Could this get renamed from "drained" to "got_records"? You use that name below when calling
this function and there's a bit of overloading of the name drained in this call path that makes
it a tad confusing.

Thanks,
Ben
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
>  	struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
> @@ -1068,12 +1068,15 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	struct cxl_get_event_payload *payload;
>  	u8 log_type = type;
>  	u16 nr_rec;
> +	int rc = 0;
> +
> +	*drained = false;
>  
>  	mutex_lock(&mds->event.log_lock);
>  	payload = mds->event.buf;
>  
>  	do {
> -		int rc, i;
> +		int i;
>  		struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd) {
>  			.opcode = CXL_MBOX_OP_GET_EVENT_RECORD,
>  			.payload_in = &log_type,
> @@ -1094,6 +1097,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		nr_rec = le16_to_cpu(payload->record_count);
>  		if (!nr_rec)
>  			break;
> +		*drained = true;
>  
>  		for (i = 0; i < nr_rec; i++)
>  			__cxl_event_trace_record(cxlmd, type,
> @@ -1112,31 +1116,59 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	} while (nr_rec);
>  
>  	mutex_unlock(&mds->event.log_lock);
> +
> +	return rc;
>  }
>  
>  /**
>   * cxl_mem_get_event_records - Get Event Records from the device
>   * @mds: The driver data for the operation
>   * @status: Event Status register value identifying which events are available.
> + * @drained: Optional mask of the logs in @status that returned records.
>   *
>   * Retrieve all event records available on the device, report them as trace
> - * events, and clear them.
> + * events, and clear them.  Every log named in @status is drained even if
> + * another one fails, so that a broken log does not suppress reporting from
> + * the rest.
> + *
> + * Return: 0, or the first error encountered.
>   *
>   * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
>   * See CXL rev 3.0 @8.2.9.2.3 Clear Event Records
>   */
> -void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status)
> +int cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status,
> +			      u32 *drained)
>  {
> +	static const struct {
> +		u32 status;
> +		enum cxl_event_log_type type;
> +	} logs[] = {
> +		{ CXLDEV_EVENT_STATUS_FATAL, CXL_EVENT_TYPE_FATAL },
> +		{ CXLDEV_EVENT_STATUS_FAIL, CXL_EVENT_TYPE_FAIL },
> +		{ CXLDEV_EVENT_STATUS_WARN, CXL_EVENT_TYPE_WARN },
> +		{ CXLDEV_EVENT_STATUS_INFO, CXL_EVENT_TYPE_INFO },
> +	};
> +	int ret = 0;
> +
>  	dev_dbg(mds->cxlds.dev, "Reading event logs: %x\n", status);
>  
> -	if (status & CXLDEV_EVENT_STATUS_FATAL)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FATAL);
> -	if (status & CXLDEV_EVENT_STATUS_FAIL)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FAIL);
> -	if (status & CXLDEV_EVENT_STATUS_WARN)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_WARN);
> -	if (status & CXLDEV_EVENT_STATUS_INFO)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_INFO);
> +	if (drained)
> +		*drained = 0;
> +
> +	for (int i = 0; i < ARRAY_SIZE(logs); i++) {
> +		bool got_records;
> +		int rc;
> +
> +		if (!(status & logs[i].status))
> +			continue;
> +
> +		rc = cxl_mem_get_records_log(mds, logs[i].type, &got_records);
> +		if (got_records && drained)
> +			*drained |= logs[i].status;
> +		ret = ret ?: rc;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL");
>  
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index ed419d0c59f2..cb5372eb2d85 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -803,7 +803,8 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				unsigned long *cmds);
>  void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				  unsigned long *cmds);
> -void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
> +int cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status,
> +			      u32 *drained);
>  void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>  			    enum cxl_event_log_type type,
>  			    enum cxl_event_type event_type,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 267c679b0b3c..3daf17835c06 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -514,21 +514,46 @@ static irqreturn_t cxl_event_thread(int irq, void *id)
>  	struct cxl_dev_id *dev_id = id;
>  	struct cxl_dev_state *cxlds = dev_id->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	u32 status;
> +	u32 mask = CXLDEV_EVENT_STATUS_ALL;
> +
> +	while (mask) {
> +		u32 status, drained, stuck;
> +		int rc;
>  
> -	do {
>  		/*
>  		 * CXL 3.0 8.2.8.3.1: The lower 32 bits are the status;
>  		 * ignore the reserved upper 32 bits
>  		 */
>  		status = readl(cxlds->regs.status + CXLDEV_DEV_EVENT_STATUS_OFFSET);
> -		/* Ignore logs unknown to the driver */
> -		status &= CXLDEV_EVENT_STATUS_ALL;
> +		/* Ignore logs unknown to the driver, and logs given up on */
> +		status &= mask;
>  		if (!status)
>  			break;
> -		cxl_mem_get_event_records(mds, status);
> +
> +		/*
> +		 * A failed drain leaves the log's status bit set, so another
> +		 * pass would resend the same query forever.
> +		 */
> +		rc = cxl_mem_get_event_records(mds, status, &drained);
> +		if (rc)
> +			break;
> +
> +		/*
> +		 * The Event Status register is device owned and read only, so
> +		 * it cannot bound this loop; records read can.  A device that
> +		 * leaves a bit set with an empty log makes no progress, and
> +		 * only the device can clear that state, so stop polling that
> +		 * log rather than spin on it.
> +		 */
> +		stuck = status & ~drained;
> +		if (stuck) {
> +			dev_warn_once(cxlds->dev,
> +				      "Event status %#x set with no records to read\n",
> +				      stuck);
> +			mask &= ~stuck;
> +		}
>  		cond_resched();
> -	} while (status);
> +	}
>  
>  	return IRQ_HANDLED;
>  }
> @@ -682,7 +707,7 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  	if (rc)
>  		return rc;
>  
> -	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
> +	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL, NULL);
>  
>  	return 0;
>  }
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index a7da279aa3ef..87e01612cea9 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -372,7 +372,7 @@ static void cxl_mock_event_trigger(struct device *dev)
>  			event_reset_log(log);
>  	}
>  
> -	cxl_mem_get_event_records(mdata->mds, mes->ev_status);
> +	cxl_mem_get_event_records(mdata->mds, mes->ev_status, NULL);
>  }
>  
>  struct cxl_event_record_raw maint_needed = {
> @@ -1805,7 +1805,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	if (rc)
>  		dev_dbg(dev, "No CXL FWCTL setup\n");
>  
> -	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
> +	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL, NULL);
>  	cxl_mock_test_feat_init(mdata);
>  
>  	return 0;

Re: [PATCH 1/1] cxl/events: Bound the event log drain loop

[Richard Cheng]

On Wed, Aug 26, 2026 at 11:43:08AM +0800, Anisa Su wrote:
> cxl_event_thread() drains every log named in the Event Status register and
> repeats until that register reads zero.  CXL 8.2.9.3.1 Table 8-203 marks
> the register RO and leaves clearing it to the device, so a misbehaving device
> that leaves a status bit set spins the thread forever, resending
> Get Event Records as fast as the mailbox completes.
> 
> Bound the loop on work completed instead.  cxl_mem_get_records_log() and
> cxl_mem_get_event_records() report failures and which logs returned
> records; the thread stops on the first error and drops any log whose status
> bit was set while it returned nothing.  Every pass either drains a record
> or clears a bit from the mask, so the loop terminates whatever the device
> reports.  Logs are drained best effort, so a broken one does not suppress
> reporting from the rest.
> 
> Fixes: a49aa8141b65 ("cxl/mem: Wire up event interrupts")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
>  drivers/cxl/core/mbox.c      | 58 ++++++++++++++++++++++++++++--------
>  drivers/cxl/cxlmem.h         |  3 +-
>  drivers/cxl/pci.c            | 39 +++++++++++++++++++-----
>  tools/testing/cxl/test/mem.c |  4 +--
>  4 files changed, 81 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 7c6c5b7450a5..5954ba20f0be 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1059,8 +1059,8 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  	return rc;
>  }
>  
> -static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> -				    enum cxl_event_log_type type)
> +static int cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> +				   enum cxl_event_log_type type, bool *drained)
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
>  	struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
> @@ -1068,12 +1068,15 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	struct cxl_get_event_payload *payload;
>  	u8 log_type = type;
>  	u16 nr_rec;
> +	int rc = 0;
> +
> +	*drained = false;
>  
>  	mutex_lock(&mds->event.log_lock);
>  	payload = mds->event.buf;
>  
>  	do {
> -		int rc, i;
> +		int i;
>  		struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd) {
>  			.opcode = CXL_MBOX_OP_GET_EVENT_RECORD,
>  			.payload_in = &log_type,
> @@ -1094,6 +1097,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		nr_rec = le16_to_cpu(payload->record_count);
>  		if (!nr_rec)
>  			break;
> +		*drained = true;
>  
>  		for (i = 0; i < nr_rec; i++)
>  			__cxl_event_trace_record(cxlmd, type,
> @@ -1112,31 +1116,59 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	} while (nr_rec);
>  
>  	mutex_unlock(&mds->event.log_lock);
> +
> +	return rc;
>  }
>  
>  /**
>   * cxl_mem_get_event_records - Get Event Records from the device
>   * @mds: The driver data for the operation
>   * @status: Event Status register value identifying which events are available.
> + * @drained: Optional mask of the logs in @status that returned records.
>   *
>   * Retrieve all event records available on the device, report them as trace
> - * events, and clear them.
> + * events, and clear them.  Every log named in @status is drained even if
> + * another one fails, so that a broken log does not suppress reporting from
> + * the rest.
> + *
> + * Return: 0, or the first error encountered.
>   *
>   * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
>   * See CXL rev 3.0 @8.2.9.2.3 Clear Event Records
>   */
> -void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status)
> +int cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status,
> +			      u32 *drained)
>  {
> +	static const struct {
> +		u32 status;
> +		enum cxl_event_log_type type;
> +	} logs[] = {
> +		{ CXLDEV_EVENT_STATUS_FATAL, CXL_EVENT_TYPE_FATAL },
> +		{ CXLDEV_EVENT_STATUS_FAIL, CXL_EVENT_TYPE_FAIL },
> +		{ CXLDEV_EVENT_STATUS_WARN, CXL_EVENT_TYPE_WARN },
> +		{ CXLDEV_EVENT_STATUS_INFO, CXL_EVENT_TYPE_INFO },
> +	};
> +	int ret = 0;
> +
>  	dev_dbg(mds->cxlds.dev, "Reading event logs: %x\n", status);
>  
> -	if (status & CXLDEV_EVENT_STATUS_FATAL)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FATAL);
> -	if (status & CXLDEV_EVENT_STATUS_FAIL)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_FAIL);
> -	if (status & CXLDEV_EVENT_STATUS_WARN)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_WARN);
> -	if (status & CXLDEV_EVENT_STATUS_INFO)
> -		cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_INFO);
> +	if (drained)
> +		*drained = 0;
> +
> +	for (int i = 0; i < ARRAY_SIZE(logs); i++) {
> +		bool got_records;
> +		int rc;
> +
> +		if (!(status & logs[i].status))
> +			continue;
> +
> +		rc = cxl_mem_get_records_log(mds, logs[i].type, &got_records);
> +		if (got_records && drained)
> +			*drained |= logs[i].status;
> +		ret = ret ?: rc;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL");
>  
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index ed419d0c59f2..cb5372eb2d85 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -803,7 +803,8 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				unsigned long *cmds);
>  void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				  unsigned long *cmds);
> -void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
> +int cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status,
> +			      u32 *drained);
>  void cxl_event_trace_record(struct cxl_memdev *cxlmd,
>  			    enum cxl_event_log_type type,
>  			    enum cxl_event_type event_type,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 267c679b0b3c..3daf17835c06 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -514,21 +514,46 @@ static irqreturn_t cxl_event_thread(int irq, void *id)
>  	struct cxl_dev_id *dev_id = id;
>  	struct cxl_dev_state *cxlds = dev_id->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	u32 status;
> +	u32 mask = CXLDEV_EVENT_STATUS_ALL;
> +
> +	while (mask) {
> +		u32 status, drained, stuck;
> +		int rc;
>  
> -	do {
>  		/*
>  		 * CXL 3.0 8.2.8.3.1: The lower 32 bits are the status;
>  		 * ignore the reserved upper 32 bits
>  		 */
>  		status = readl(cxlds->regs.status + CXLDEV_DEV_EVENT_STATUS_OFFSET);
> -		/* Ignore logs unknown to the driver */
> -		status &= CXLDEV_EVENT_STATUS_ALL;
> +		/* Ignore logs unknown to the driver, and logs given up on */
> +		status &= mask;
>  		if (!status)
>  			break;
> -		cxl_mem_get_event_records(mds, status);
> +
> +		/*
> +		 * A failed drain leaves the log's status bit set, so another
> +		 * pass would resend the same query forever.
> +		 */
> +		rc = cxl_mem_get_event_records(mds, status, &drained);
> +		if (rc)
> +			break;
> +

Hi Anisa,

IMHO, this new error path might leave an event log permanently undrainted.

Now cxl_event_thread() breaks on any error returned by cxl_mem_get_event_records(), which incldues a temporary failure e.g. "Retry Required" from Get Event
Records or Clear Event Records. In that case the log ramain non-empty.

CXL event interrupt are generated when a log transitions from empty to non-empty,
so after returning from the handler there may be no subsequent interrupt
to retry the drain.

Maybe we can use a bounded retry for retryable failure ? or schedule deferred
retry work before returning ?

Best regards,
Richard Cheng.

> +		/*
> +		 * The Event Status register is device owned and read only, so
> +		 * it cannot bound this loop; records read can.  A device that
> +		 * leaves a bit set with an empty log makes no progress, and
> +		 * only the device can clear that state, so stop polling that
> +		 * log rather than spin on it.
> +		 */
> +		stuck = status & ~drained;
> +		if (stuck) {
> +			dev_warn_once(cxlds->dev,
> +				      "Event status %#x set with no records to read\n",
> +				      stuck);
> +			mask &= ~stuck;
> +		}
>  		cond_resched();
> -	} while (status);
> +	}
>  
>  	return IRQ_HANDLED;
>  }
> @@ -682,7 +707,7 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  	if (rc)
>  		return rc;
>  
> -	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
> +	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL, NULL);
>  
>  	return 0;
>  }
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index a7da279aa3ef..87e01612cea9 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -372,7 +372,7 @@ static void cxl_mock_event_trigger(struct device *dev)
>  			event_reset_log(log);
>  	}
>  
> -	cxl_mem_get_event_records(mdata->mds, mes->ev_status);
> +	cxl_mem_get_event_records(mdata->mds, mes->ev_status, NULL);
>  }
>  
>  struct cxl_event_record_raw maint_needed = {
> @@ -1805,7 +1805,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	if (rc)
>  		dev_dbg(dev, "No CXL FWCTL setup\n");
>  
> -	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
> +	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL, NULL);
>  	cxl_mock_test_feat_init(mdata);
>  
>  	return 0;
> -- 
> 2.43.0
> 
>