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

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

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;