Re: [PATCH v13 13/15] cxl: Restore CXL state after PCI reset

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 22 Sep 2026 08:39:22 +0000
Srirangan Madhavan <smadhavan@nvidia.com> wrote:

> Restore the PCI configuration needed for HDM access, followed by cached
> HDM decoder and CXL Device DVSEC state. Recommit a cleared locked decoder
> before restoring its lock, but leave surviving locked state unchanged.
> 
> Stop at the first restoration failure and preserve a disabled PCI command
> snapshot. Keep IOMMU exclusion active through restoration and post-reset
> cache invalidation.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>

Hi Srirangan,

I'm out of time for today so main request it to take a look at that
final function. It is running quite a complex statemachine with a bunch
of ownership transfers. That's the sort of single function that takes
too much energy to review. Please have a revisit and see if there
is a simpler stepwise way to do that.

> @@ -726,6 +1064,18 @@ static int cxl_reset_disable_cache(struct pci_dev *pdev, int dvsec, u16 cap)
>  	return cxl_reset_wait_cache_wbi(pdev, dvsec);
>  }
>  
> +static int cxl_reset_restore_cache_policy(struct pci_dev *pdev, int dvsec)
> +{
> +	int rc;
> +
> +	rc = cxl_reset_update_ctrl2_no_replay(pdev, dvsec, 0,
> +					      PCI_DVSEC_CXL_DISABLE_CACHING);
> +	if (rc)
> +		pci_err(pdev, "failed to re-enable CXL caching: %d\n", rc);
> +
> +	return rc;
> +}
> +
>  static int cxl_reset_wait_done(struct pci_dev *pdev, int dvsec, u16 cap)
>  {
>  	unsigned long deadline;
> @@ -767,28 +1117,41 @@ static int cxl_reset_wait_done(struct pci_dev *pdev, int dvsec, u16 cap)
>  	}
>  }
>  
> -static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
> -			     struct cxl_hdm_range_context *range_ctx)
> +static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap)
>  {
> +	struct cxl_hdm_info *snapshot = NULL;
> +	struct cxl_hdm_range_context range_ctx;
> +	u16 command;
>  	int rc, rc2;
>  
> -	rc = cxl_reset_disable_cache(pdev, dvsec, cap);
> +	down_write(&cxl_rwsem.region);
> +	INIT_LIST_HEAD(&range_ctx.ranges);
> +
> +	rc = cxl_hdm_ranges_prepare(&range_ctx, pdev);
> +	if (rc)
> +		goto out_destroy_ranges;
> +
> +	rc = cxl_snapshot_hdm(pdev, &snapshot);
>  	if (rc)
>  		goto out_destroy_ranges;
>  
> +	rc = cxl_reset_disable_cache(pdev, dvsec, cap);
> +	if (rc)
> +		goto out_destroy_enable_cache;
> +
>  	if (!pci_wait_for_pending_transaction(pdev)) {
>  		pci_err(pdev, "timed out waiting for pending transactions\n");
>  		rc = -ETIMEDOUT;
> -		goto out_destroy_ranges;
> +		goto out_destroy_enable_cache;
>  	}
>  
>  	rc = pci_dev_reset_iommu_prepare(pdev);
>  	if (rc) {
>  		pci_err(pdev, "failed to stop IOMMU for CXL reset: %d\n", rc);
> -		goto out_destroy_ranges;
> +		goto out_destroy_enable_cache;
>  	}
>  
> -	/* Clear Memory Clear again even if this command write reports failure. */
> +	/* Clear Memory Clear below even if this command write reports failure. */
>  	rc = cxl_reset_update_ctrl2_no_replay(pdev, dvsec,
>  					      PCI_DVSEC_CXL_INIT_CXL_RST |
>  					      PCI_DVSEC_CXL_RST_MEM_CLR_EN, 0);
> @@ -798,39 +1161,60 @@ static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
>  	rc = cxl_reset_wait_done(pdev, dvsec, cap);
>  
>  out_clear_memory:
> -	rc2 = cxl_reset_update_ctrl2_no_replay(
> -		pdev, dvsec, 0, PCI_DVSEC_CXL_RST_MEM_CLR_EN);
> +	rc2 = cxl_reset_update_ctrl2_no_replay(pdev, dvsec, 0,
> +					       PCI_DVSEC_CXL_RST_MEM_CLR_EN);
>  	if (rc2)
>  		pci_err(pdev, "failed to clear CXL Reset Memory Clear: %d\n",
> -			rc2);
> +			 rc2);
>  	rc = rc ?: rc2;
> +	if (rc)
> +		goto out_finish_ranges;
> +
> +	cxl_restore_pci_state_for_hdm_restore(pdev, &command);
> +	rc = cxl_restore_state(pdev, snapshot);
>  
> +out_finish_ranges:
>  	/* Evict lines fetched during reset before ending DMA exclusion. */
> -	rc2 = cxl_hdm_ranges_finish(range_ctx);
> +	rc2 = cxl_hdm_ranges_finish(&range_ctx);
>  	rc = rc ?: rc2;
> +	if (rc)
> +		goto out_fail_closed;
> +
>  	pci_dev_reset_iommu_done(pdev);
> -	goto out_enable_cache;
>  
> -out_destroy_ranges:
> -	cxl_hdm_range_context_destroy(range_ctx);
> +	rc = cxl_reset_restore_cache_policy(pdev, dvsec);
> +	if (rc) {
> +		cxl_reset_save_disabled_state(pdev);
> +		goto out_unlock;
> +	}
>  
> -out_enable_cache:
> -	/*
> -	 * DISABLE_CACHING was the first preparation step. Restore the original
> -	 * cache policy last, after reset exclusion has ended.
> -	 */
> -	rc2 = cxl_reset_update_ctrl2_no_replay(
> -		pdev, dvsec, 0, PCI_DVSEC_CXL_DISABLE_CACHING);
> -	if (rc2)
> -		pci_err(pdev, "failed to re-enable CXL caching: %d\n", rc2);
> +	rc = cxl_reset_save_restored_state(pdev, command);
> +	goto out_unlock;
> +
> +out_fail_closed:
> +	cxl_reset_save_disabled_state(pdev);
> +	pci_dev_reset_iommu_done(pdev);
> +	rc2 = cxl_reset_restore_cache_policy(pdev, dvsec);
>  	rc = rc ?: rc2;
> +	goto out_unlock;
> +
> +out_destroy_enable_cache:
> +	cxl_hdm_range_context_destroy(&range_ctx);
> +	rc2 = cxl_reset_restore_cache_policy(pdev, dvsec);
> +	rc = rc ?: rc2;
> +	goto out_unlock;
> +
> +out_destroy_ranges:
> +	cxl_hdm_range_context_destroy(&range_ctx);
>  
> +out_unlock:
> +	kfree(snapshot);
> +	up_write(&cxl_rwsem.region);

This is going to take some signficant unpicking but as
it stands it is very complex to follow with releases quite
often out of reverse order of setup and complex hand
off from one function to another. Please take another look
even if I don't get back to this with more specific comments.


>  	return rc;
>  }