Fix infinite loop when draining a stuck event log

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 <[email protected]>

Signed-off-by Anisa Su <[email protected]>

---

 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(-)

drivers/cxl/core/mbox.c +0 -0

index 7c6c5b7450a5..5954ba20f0be 100644

drivers/cxl/core/mbox.c +43 -11

 {
 	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");
 

drivers/cxl/cxlmem.h +2 -1

index ed419d0c59f2..cb5372eb2d85 100644
@@ -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,

drivers/cxl/pci.c +32 -7

index 267c679b0b3c..3daf17835c06 100644
@@ -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;
 }

tools/testing/cxl/test/mem.c +2 -2

index a7da279aa3ef..87e01612cea9 100644
@@ -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;

Quoted 14 lines · 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.

Quoted 4 lines

Fixes a49aa8141b65 ("cxl/mem: Wire up event interrupts")

Cc <[email protected]>

Signed-off-by Anisa Su <[email protected]>

---

One nit below, but looks good to me regardless so:

Reviewed-by Ben Cheatham <[email protected]>

Quoted 14 lines

 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(-)

drivers/cxl/core/mbox.c +0 -0

index 7c6c5b7450a5..5954ba20f0be 100644
@@ -1059,8 +1059,8 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 	return rc;
 }
 

drivers/cxl/core/mbox.c static int cxl_clear_event_record(struct cxl_memdev_state *mds, · line 1063

Quoted 4 lines

drivers/cxl/core/mbox.c +2 -2

-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

Quoted 200 lines

drivers/cxl/core/mbox.c +43 -11

 {
 	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");
 

drivers/cxl/cxlmem.h +2 -1

index ed419d0c59f2..cb5372eb2d85 100644
@@ -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,

drivers/cxl/pci.c +32 -7

index 267c679b0b3c..3daf17835c06 100644
@@ -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;
 }

tools/testing/cxl/test/mem.c +2 -2

index a7da279aa3ef..87e01612cea9 100644
@@ -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;

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 <[email protected]>

Signed-off-by Anisa Su <[email protected]>

---
 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(-)

drivers/cxl/core/mbox.c +45 -13

index 7c6c5b7450a5..5954ba20f0be 100644
@@ -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");
 

drivers/cxl/cxlmem.h +2 -1

index ed419d0c59f2..cb5372eb2d85 100644
@@ -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,

drivers/cxl/pci.c +7 -4

index 267c679b0b3c..3daf17835c06 100644
@@ -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;

drivers/cxl/pci.c +16 -2

+		/*
+		 * 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;
 }

tools/testing/cxl/test/mem.c +2 -2

index a7da279aa3ef..87e01612cea9 100644
@@ -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;

Signature · 1 line

2.43.0

Quoted 175 lines · 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 <[email protected]>

Signed-off-by Anisa Su <[email protected]>

---
 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(-)

drivers/cxl/core/mbox.c +45 -13

index 7c6c5b7450a5..5954ba20f0be 100644
@@ -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");
 

drivers/cxl/cxlmem.h +2 -1

index ed419d0c59f2..cb5372eb2d85 100644
@@ -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,

drivers/cxl/pci.c +7 -4

index 267c679b0b3c..3daf17835c06 100644
@@ -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;

drivers/cxl/pci.c static irqreturn_t cxl_event_thread(int irq, void *id) · line 540

Quoted 10 lines

drivers/cxl/pci.c +9 -1

-		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.

Quoted 55 lines

drivers/cxl/pci.c +16 -2

+		/*
+		 * 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;
 }

tools/testing/cxl/test/mem.c +2 -2

index a7da279aa3ef..87e01612cea9 100644
@@ -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;

Signature · 1 line

2.43.0