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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Error aggregation masks retryable errors, causing transient failures to be permanently abandoned without retry.
- [Medium] False positive hardware warning due to race condition between concurrent IRQ threads.
--

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

cxl/events: Bound get records loop in cxl_event_thread()

This commit limits the cxl_event_thread loop by checking completed work
rather than only relying on the hardware Event Status register. It bounds
the retry attempts for retryable errors (-EBUSY, -ETIMEDOUT, -EAGAIN) and
gives up on permanent errors.

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 7c6c5b7450a54..2b71a8e1f35f7 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
[ ... ]
> @@ -1112,31 +1129,60 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
[ ... ]
> +	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;

[Severity: High]
Does this error aggregation mask retryable errors? 

If an earlier log encounters a non-retryable error (like -EIO), it is stored
in ret. If a subsequent log encounters a retryable error (like -EAGAIN), the
earlier error takes precedence due to the ret = ret ?: rc logic.

Back in the caller cxl_event_thread(), the loop checks the return code:

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

Will this cause the thread to abort the entire batch and abandon the transient
error without retrying, permanently losing those hardware events?

> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL");
>  
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 267c679b0b3c2..239df18c9c87b 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)
[ ... ]
>  static irqreturn_t cxl_event_thread(int irq, void *id)
>  {
[ ... ]
> +	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);
[ ... ]
> -		cxl_mem_get_event_records(mds, status);
> +		rc = cxl_mem_get_event_records(mds, status, &drained);
[ ... ]
> +		/*
> +		 * 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]
Is there a risk of a false positive hardware warning here due to a race
condition? 

If multiple event logs share an IRQ vector, two concurrent cxl_event_thread
instances could race. If Thread A drains the events, Thread B would receive no
records, and its call to cxl_mem_get_event_records() would return with
drained == 0.

Thread B would then evaluate stuck = status & ~drained using the stale status
value read at the beginning of the loop. 

Could this lead to Thread B incorrectly concluding the device is stuck and
prematurely masking the log for its iteration?

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