Re: [PATCH v17 15/19] PCI/CXL: Restore CXL state after PCI reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI/CXL: cxl_reset_quiesced() skips disabling device if cache restore fails
--

commit 0833e3bda8d31d8ddf0c1310990a9ee4b81a1760
Author: Srirangan Madhavan <smadhavan@nvidia.com>

PCI/CXL: Restore CXL state after PCI reset

This commit restores PCI configuration, cached HDM decoder settings, and CXL
Device DVSEC control state after a CXL device reset. If any restoration step
fails, it disables the device and updates the saved PCI command state so the
outer PCI reset flow does not re-enable memory decoding or bus mastering.

> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> index c07266ab01336..77d0e30841d9f 100644
> --- a/drivers/pci/cxl.c
> +++ b/drivers/pci/cxl.c

[ ... ]

> @@ -415,7 +572,9 @@ static int cxl_clear_memory(struct pci_dev *pdev, int dvsec, bool initiate)
>  }
>  
>  static int __cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
> -			      struct cxl_hdm_quiesce *q)
> +			      const struct cxl_hdm_info *snapshot,
> +			      struct cxl_hdm_quiesce *q,
> +			      u16 *command)
>  	__must_hold(cxl_region_quiesce)
>  {
>  	int rc, rc2;
> @@ -441,10 +600,15 @@ static int __cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
>  		pci_err(pdev, "failed to clear CXL Reset Memory Clear: %d\n", rc2);
>  	rc = rc ?: rc2;
>  
> +	if (!rc)
> +		rc = pci_cxl_restore_state(pdev, snapshot, command);
> +
>  	rc2 = cxl_hdm_quiesce_flush(q);
>  	if (rc2)
>  		pci_err(pdev, "failed to invalidate CPU cache after CXL reset: %d\n", rc2);
>  	rc = rc ?: rc2;
> +	if (rc)
> +		cxl_reset_save_disabled_state(pdev);
>  
>  	pci_dev_reset_iommu_done(pdev);
>  	return rc;
> @@ -455,6 +619,7 @@ static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap)
>  {
>  	struct cxl_hdm_info *snapshot __free(kfree) = NULL;
>  	struct cxl_pci_cache cache = {};
> +	u16 command;
>  	int rc, rc2;
>  
>  	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2,
> @@ -478,11 +643,19 @@ static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap)
>  		return rc;
>  	}
>  	if (!rc)
> -		rc = __cxl_reset_execute(pdev, dvsec, cap, &cache.quiesce);
> +		rc = __cxl_reset_execute(pdev, dvsec, cap, snapshot, &cache.quiesce,
> +					 &command);
>  	/* Restore cache policy after any attempt to disable caching. */
>  	rc2 = cxl_reset_restore_cache_policy(pdev, dvsec, cache.saved_ctrl2);
>  	cxl_hdm_quiesce_destroy(&cache.quiesce);
> -	return rc ?: rc2;
> +	if (rc)
> +		return rc;
> +	if (rc2) {
> +		cxl_reset_save_disabled_state(pdev);
> +		return rc2;
> +	}

[Severity: Medium]
In drivers/pci/cxl.c:cxl_reset_quiesced(), can returning early on if (rc)
before checking if (rc2) skip calling cxl_reset_save_disabled_state(pdev) when
restoring the CXL cache policy also fails?

Specifically, rc can be non-zero before cxl_reset_save_disabled_state(pdev) is
reached in two cases after CXL caching has been disabled:

1. cxl_reset_cache_prepare() sets cache.cache_disabled = true and disables
   caching via cxl_reset_disable_cache(), but cxl_reset_wait_cache_wbi() then
   fails.

2. cxl_reset_cache_prepare() succeeds, and __cxl_reset_execute() returns early
   from pci_wait_for_pending_transaction() or pci_dev_reset_iommu_prepare()
   before reaching cxl_reset_save_disabled_state(pdev) at the end of
   __cxl_reset_execute():

drivers/pci/cxl.c:__cxl_reset_execute() {
	if (!pci_wait_for_pending_transaction(pdev)) {
		pci_err(pdev, "timed out waiting for pending transactions\n");
		return -ETIMEDOUT;
	}

	rc = pci_dev_reset_iommu_prepare(pdev);
	if (rc) {
		pci_err(pdev, "failed to stop IOMMU for CXL reset: %d\n", rc);
		return rc;
	}
	...
}

In either case, pdev->saved_config_space[PCI_COMMAND / 4] still holds the
pre-reset MSE and BME bits. If cxl_reset_restore_cache_policy() then fails
(rc2 != 0), checking if (rc) first returns without calling
cxl_reset_save_disabled_state(pdev), allowing the outer PCI reset path to
re-enable memory decoding and bus mastering on a device whose CXL cache policy
in PCI_DVSEC_CXL_CTRL2 was not restored.

Should the if (rc2) check call cxl_reset_save_disabled_state(pdev) before
returning rc?

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