* Re: [PATCH 1/1] cxl/events: Bound the event log drain loop
2026-08-26 18:43 ` [PATCH 1/1] cxl/events: Bound the event log drain loop Anisa Su
@ 2026-08-26 18:57 ` sashiko-bot
2026-08-26 21:28 ` Cheatham, Benjamin
2026-08-27 3:01 ` Richard Cheng
2 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-08-26 18:57 UTC (permalink / raw)
To: Anisa Su; +Cc: linux-cxl
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] cxl/events: Bound the event log drain loop
2026-08-26 18:43 ` [PATCH 1/1] cxl/events: Bound the event log drain loop Anisa Su
2026-08-26 18:57 ` sashiko-bot
@ 2026-08-26 21:28 ` Cheatham, Benjamin
2026-08-27 3:01 ` Richard Cheng
2 siblings, 0 replies; 5+ messages in thread
From: Cheatham, Benjamin @ 2026-08-26 21:28 UTC (permalink / raw)
To: Anisa Su, linux-cxl
Cc: Davidlohr Bueso, Jonathan Cameron, Gregory Price, Dave Jiang,
Alison Schofield, Vishal Verma, Anisa Su, stable
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;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] cxl/events: Bound the event log drain loop
2026-08-26 18:43 ` [PATCH 1/1] cxl/events: Bound the event log drain loop Anisa Su
2026-08-26 18:57 ` sashiko-bot
2026-08-26 21:28 ` Cheatham, Benjamin
@ 2026-08-27 3:01 ` Richard Cheng
2 siblings, 0 replies; 5+ messages in thread
From: Richard Cheng @ 2026-08-27 3:01 UTC (permalink / raw)
To: Anisa Su
Cc: linux-cxl, Davidlohr Bueso, Jonathan Cameron, Gregory Price,
Dave Jiang, Alison Schofield, Vishal Verma, Anisa Su, stable
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread