Re: [PATCH v13 10/15] cxl: Add CXL Device Reset sequencing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] cxl: 100s thread stall due to ignoring PCI errors in cxl_reset_wait_done()
- [Medium] cxl: spurious reset timeout for devices with <= 100ms budget
--

commit 0940b93cca76ebfd20726ff13344213cdb258009
Author: Srirangan Madhavan <smadhavan@nvidia.com>
cxl: Add CXL Device Reset sequencing

This commit implements CXL Device Reset for eligible Type 2 devices. It
disables caching, prepares the IOMMU, initiates reset with Memory Clear
enabled, and waits for completion.

> diff --git a/drivers/cxl/core/resource.c b/drivers/cxl/core/resource.c
> index b5f8c28e613fe..57eec77cbb213 100644
> --- a/drivers/cxl/core/resource.c
> +++ b/drivers/cxl/core/resource.c
[ ... ]
> +static int cxl_reset_wait_done(struct pci_dev *pdev, int dvsec, u16 cap)
> +{
> +	unsigned long deadline;
> +	u32 timeout_ms;
> +	u16 status2;
> +	int idx, rc;
> +
> +	idx = FIELD_GET(PCI_DVSEC_CXL_RST_TIMEOUT, cap);
> +	if (idx >= ARRAY_SIZE(cxl_reset_timeout_ms)) {
> +		int last = ARRAY_SIZE(cxl_reset_timeout_ms) - 1;
> +
> +		pci_warn(pdev,
> +			 "unknown CXL reset timeout encoding %d; using %u ms\n",
> +			 idx, cxl_reset_timeout_ms[last]);
> +		idx = last;
> +	}
> +
> +	timeout_ms = max_t(u32, cxl_reset_timeout_ms[idx],
> +			   CXL_RESET_RRS_WAIT_MS);
> +	msleep(CXL_RESET_RRS_WAIT_MS);
> +	deadline = jiffies + msecs_to_jiffies(timeout_ms -
> +					      CXL_RESET_RRS_WAIT_MS);

[Severity: Medium]
Does this logic yield a 0-jiffy budget and immediately timeout if the device
is not ready on the first poll?

Because timeout_ms is clamped to a minimum of 100ms (CXL_RESET_RRS_WAIT_MS),
the calculation of timeout_ms - CXL_RESET_RRS_WAIT_MS results in 0 for
devices advertising 10ms or 100ms timeouts.

After the initial 100ms sleep, any U16_MAX response (which is common while
the PCIe link is recovering) will immediately trigger -ETIMEDOUT without
retrying, providing zero grace period for link recovery.

> +
> +	for (;;) {
> +		rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_STATUS2,
> +					  &status2);
> +		if (rc || status2 == U16_MAX)
> +			goto not_ready;

[Severity: Medium]
Could this cause a kernel thread to stall for up to 100 seconds if the device
experiences a permanent error?

By grouping a non-zero rc (which pci_read_config_word can return for
permanent errors like device disconnection, e.g., PCIBIOS_DEVICE_NOT_FOUND)
with the transient U16_MAX state, the code falls through to not_ready.

If a device advertises a 100,000 ms timeout, this causes the kernel thread
to sleep and poll uselessly for up to 100 seconds when the device is entirely
gone, delaying recovery and blocking operations.

> +		if (status2 & PCI_DVSEC_CXL_RST_ERR)
> +			return -EIO;
> +		if (status2 & PCI_DVSEC_CXL_RST_DONE)
> +			return 0;
> +
> +not_ready:
> +		if (time_after_eq(jiffies, deadline))
> +			return -ETIMEDOUT;
> +
> +		msleep(CXL_RESET_STATUS_POLL_MS);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922083924.2451158-1-smadhavan@nvidia.com?part=10