Re: [PATCH v9 05/11] cxl: Add CXL Device Reset helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect ordering of CXL cache writeback (WBI), pending transaction wait, and IOMMU reset preparation.
- [Medium] Premature success of WBI wait loop on device disconnect due to unhandled U16_MAX status.
- [Medium] Potential false timeout in the reset wait loop due to a missing final hardware status check.
--

--- Patch [5]: [PATCH v9 05/11] cxl: Add CXL Device Reset helper ---
commit c832f1294f231fc19fa036ee29639b20e723c1a1
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Add CXL Device Reset helper

This commit adds an internal CXL Device Reset helper for Type 2 functions
that advertise CXL Reset in the CXL Device DVSEC. The helper handles cache
disabling, writeback, reset initiation, and re-enabling caching.

> diff --git a/drivers/cxl/core/reset.c b/drivers/cxl/core/reset.c
> index 97b72cc67b6bd..7a9b1ecfadcf9 100644
> --- a/drivers/cxl/core/reset.c
> +++ b/drivers/cxl/core/reset.c
[ ... ]
> +static int cxl_reset_disable_cache(struct pci_dev *pdev, int dvsec, u16 cap)
> +{
> +	int remaining_us = CXL_CACHE_WBI_TIMEOUT_US;
> +	u16 status2;
> +	int rc, rc2;
> +
> +	rc = cxl_reset_update_ctrl2(pdev, dvsec,
> +				    PCI_DVSEC_CXL_DISABLE_CACHING, 0);
> +	if (rc)
> +		return rc;
> +
> +	if (!(cap & PCI_DVSEC_CXL_CACHE_WBI_CAPABLE))
> +		return 0;
> +
> +	rc = cxl_reset_update_ctrl2(pdev, dvsec,
> +				    PCI_DVSEC_CXL_INIT_CACHE_WBI, 0);
> +	if (rc)
> +		goto err_enable_cache;
> +
> +	do {
> +		usleep_range(CXL_CACHE_WBI_POLL_US, CXL_CACHE_WBI_POLL_US + 1);
> +		remaining_us -= CXL_CACHE_WBI_POLL_US;
> +
> +		rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_STATUS2,
> +					  &status2);
> +		if (rc) {
> +			rc = pcibios_err_to_errno(rc);
> +			goto err_enable_cache;
> +		}
> +	} while (!(status2 & PCI_DVSEC_CXL_CACHE_INV) && remaining_us > 0);

[Severity: Medium]
If the device is disconnected or offline, pci_read_config_word() will return
success (0) and set status2 to U16_MAX (0xFFFF). Would this cause the loop
condition !(status2 & PCI_DVSEC_CXL_CACHE_INV) to evaluate to false and
bypass the wait entirely, falsely reporting success?

> +
> +	if (!(status2 & PCI_DVSEC_CXL_CACHE_INV)) {
> +		rc = -ETIMEDOUT;
> +		goto err_enable_cache;
> +	}
> +
> +	return 0;
[ ... ]
> +static int cxl_reset_wait_done(struct pci_dev *pdev, int dvsec, u16 cap)
> +{
[ ... ]
> +	do {
> +		rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_STATUS2,
> +					  &status2);
> +		if (rc || status2 == U16_MAX)
> +			goto not_ready;
> +
> +		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;

[Severity: Medium]
Does this timeout check need a final hardware status read?

If a thread preemption, interrupt, or scheduling delay occurs after the
config space read but before the timeout check, the hardware condition may
be met concurrently while the jiffies have advanced past the deadline. This
could potentially lead to a false -ETIMEDOUT error despite the device being
ready.

> +
> +		msleep(CXL_RESET_STATUS_POLL_MS);
> +	} while (true);
> +}
> +
> +static int cxl_reset_execute(struct pci_dev *pdev, int dvsec)
> +{
> +	bool cache_disabled = false;
> +	u16 cap;
> +	int rc;
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CAP, &cap);
> +	if (rc)
> +		return pcibios_err_to_errno(rc);
> +
> +	if (!pci_wait_for_pending_transaction(pdev))
> +		pci_err(pdev, "timed out waiting for pending transactions\n");
> +
> +	rc = pci_dev_reset_iommu_prepare(pdev);
> +	if (rc) {
> +		pci_err(pdev, "failed to stop IOMMU for CXL reset: %d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = cxl_reset_disable_cache(pdev, dvsec, cap);

[Severity: High]
Is it safe to wait for pending transactions and prepare the IOMMU for reset
before initiating the CXL cache Write-Back Invalidate (WBI)?

Initiating WBI in cxl_reset_disable_cache() forces the device to flush its
dirty cache lines via new DMA transactions. If the IOMMU has already been
prepared for reset (which may disable ATS, PRI, and block translations), could
these WBI transactions fault and lead to silent data loss or IOMMU errors?

Additionally, creating new pending transactions immediately before asserting
the CXL Reset might violate the PCIe requirement to drain all transactions
prior to reset. Should cxl_reset_disable_cache() happen before the IOMMU
preparation and pending transaction wait?

> +	if (rc)
> +		goto out;
> +	cache_disabled = true;
> +
> +	rc = cxl_reset_update_ctrl2(pdev, dvsec, PCI_DVSEC_CXL_INIT_CXL_RST,
> +				    PCI_DVSEC_CXL_RST_MEM_CLR_EN);

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