Re: [PATCH v2 1/4] cxl/events: Bound get records loop in cxl_event_thread()

[Anisa Su <anisa.su887@gmail.com>]

On Tue, Sep 01, 2026 at 07:48:02PM +0800, Li Ming wrote:
> 
> 在 2026/9/1 08:26, Anisa Su 写道:
> > cxl_event_thread() drains all logs named in the Event Status register and
> > repeats until the register reads zero. CXL r4.0 Section 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.
> 
> Hi Anisa,
> 
> Per the commit log and the description in CXL r4.0 section 8.2.9.3.1
> Table 8-203. SPEC mentions "Once the event log has zero event records,
> this bit(event status) is cleared". So it is a spec-violating issue on
> device side.
> 
Hello Ming,

Yes, that's correct. The event status bit must be cleared by the device.
However, while testing a DCD device, I encountered this scenario due to a
firmware bug. I mentioned more details in the cover letter of v1:
https://lore.kernel.org/linux-cxl/cover.1787768932.git.anisa.su@samsung.com/T/#m393e57d7f7f3916e8f2826a006ef0bc4dfd3606a

Sorry for not mentioning it more clearly this time.

> My understanding is that CXL driver does not need special handling for
> device behaviors that violate the CXL spec. If we are facing this
> issue, the correct way is fixing it on device side.
> 
I agree too, but since I encountered this on a real device, I thought it would
be worth patching up.

Also if anyone is wondering: the reason I used QEMU to test the patch this
time is because the firmware on the device I was testing has been updated,
so the error doesn't happen anymore. Instead, I emulated the bug in QEMU.

Patches 2-4 were just pre-existing errors noticed by Sashiko, not
something I encountered on real hardware, so maybe those are not necessary?

> Let's see if maintainers have comments on that.
> 
> 
> Besides, your patch makes me realize that checking the return value of
> cxl_mem_get_event_records() is valuable. GET_EVENT_RECORD and
> CLEAR_EVENT_RECORD operations could fail in
> cxl_mem_get_records_logs(), which cause that device could not have
> chance to clear event records, then device would not clear event
> status register. In that case, the loop in cxl_event_thread() will
> never break out.
> 
Yes, I think even if we decide not to add the special handling for detecting a
non-compliant device that doesn't clear the bit, checking the rc of
cxl_mem_get_event_records() is necessary.
> 
> Ming
> 
Thanks for the review!
Anisa

> > 
> > 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 drops any log whose status bit was set while it
> > returned nothing, and caps consecutive errors at CXL_EVENT_DRAIN_ATTEMPTS.
> > Every pass either drains a record, clears a bit from the mask or spends an
> > attempt, so the loop eventually terminates, even if the device misbehaves.
> > The first error is returned and an error from one log does not prevent
> > getting logs from the rest.
> > 
> > Retrying is limited to the failures that can plausibly be cleared:
> > -EBUSY and -ETIMEDOUT from the mailbox, and Retry Required from the
> > device. Anything else gives up on the first error, so cycles are not wasted
> > retrying on a non-retryable error.
> > 
> > Fixes: a49aa8141b65 ("cxl/mem: Wire up event interrupts")
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > 
> > ---
> > Changes:
> > [Richard Cheng]: retry on transient failures
> > ---
> >   drivers/cxl/core/mbox.c      | 74 +++++++++++++++++++++++++++++-------
> >   drivers/cxl/cxlmem.h         |  3 +-
> >   drivers/cxl/pci.c            | 63 ++++++++++++++++++++++++++----
> >   tools/testing/cxl/test/mem.c |  4 +-
> >   4 files changed, 120 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 7c6c5b7450a5..2b71a8e1f35f 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -988,6 +988,18 @@ static void __cxl_event_trace_record(struct cxl_memdev *cxlmd,
> >   	cxl_event_trace_record(cxlmd, type, ev_type, uuid, &record->event);
> >   }
> > +/*
> > + * cxl_mbox_cmd_rc2errno() collapses every device return code onto -ENXIO;
> > + * pick out the one the spec asks the caller to retry.
> > + */
> > +static int cxl_event_retry_rc(struct cxl_mbox_cmd *mbox_cmd, int rc)
> > +{
> > +	if (rc == -ENXIO && mbox_cmd->return_code == CXL_MBOX_CMD_RC_RETRY)
> > +		return -EAGAIN;
> > +
> > +	return rc;
> > +}
> > +
> >   static int cxl_clear_event_record(struct cxl_memdev_state *mds,
> >   				  enum cxl_event_log_type log,
> >   				  struct cxl_get_event_payload *get_pl)
> > @@ -1056,11 +1068,11 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
> >   free_pl:
> >   	kvfree(payload);
> > -	return rc;
> > +	return cxl_event_retry_rc(&mbox_cmd, 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 *got_records)
> >   {
> >   	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
> >   	struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
> > @@ -1068,12 +1080,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;
> > +
> > +	*got_records = 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,
> > @@ -1085,6 +1100,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> >   		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> >   		if (rc) {
> > +			rc = cxl_event_retry_rc(&mbox_cmd, rc);
> >   			dev_err_ratelimited(dev,
> >   				"Event log '%d': Failed to query event records : %d",
> >   				type, rc);
> > @@ -1094,6 +1110,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;
> > +		*got_records = true;
> >   		for (i = 0; i < nr_rec; i++)
> >   			__cxl_event_trace_record(cxlmd, type,
> > @@ -1112,31 +1129,60 @@ 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.  -EAGAIN if the device asked for
> > + * a command to be retried.
> >    *
> >    * 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..239df18c9c87 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -509,26 +509,75 @@ static bool cxl_alloc_irq_vectors(struct pci_dev *pdev)
> >   	return true;
> >   }
> > +#define CXL_EVENT_DRAIN_ATTEMPTS 3
> > +
> > +/*
> > + * -EAGAIN is the device asking for a retry, the rest are the mailbox being
> > + * momentarily unavailable.  Everything else is permanent as far as this can
> > + * tell.
> > + */
> > +static bool cxl_event_drain_retryable(int rc)
> > +{
> > +	return rc == -EAGAIN || rc == -EBUSY || rc == -ETIMEDOUT;
> > +}
> > +
> >   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;
> > +	int attempts = CXL_EVENT_DRAIN_ATTEMPTS;
> > +
> > +	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);
> > +
> > +		rc = cxl_mem_get_event_records(mds, status, &drained);
> > +		if (rc) {
> > +			if (!cxl_event_drain_retryable(rc)) {
> > +				dev_warn(cxlds->dev,
> > +					 "Event log drain failed: %d\n", rc);
> > +				break;
> > +			}
> > +			if (--attempts) {
> > +				fsleep(1000);
> > +				continue;
> > +			}
> > +			dev_warn(cxlds->dev,
> > +				 "Event log drain gave up after %d attempts: %d\n",
> > +				 CXL_EVENT_DRAIN_ATTEMPTS, rc);
> > +			break;
> > +		}
> > +		/* Progress was made, so reset number of attempts */
> > +		attempts = CXL_EVENT_DRAIN_ATTEMPTS;
> > +
> > +		/*
> > +		 * 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 +731,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;