Re: [PATCH v15 13/16] PCI/CXL: Restore CXL state after PCI reset

[Alison Schofield <alison.schofield@intel.com>]

On Sun, Oct 11, 2026 at 02:14:19AM +0000, Srirangan Madhavan 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.
> 
> Program the decoders from built-in CXL code that takes only a mapped
> register block, and keep PCI state handling in drivers/pci/cxl.c.
> 
> The restore helper structure and diagnostics follow Dave Jiang's
> reference implementation on his cxl-type2-reset branch.
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?id=81fff5893a3140361cac625b09ada798afb92e03
> Suggested-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> Assisted-by: LLM
> ---
>  drivers/cxl/core/hdm_regs.c | 180 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/cxl.c           | 176 +++++++++++++++++++++++++++++++++--
>  include/cxl/hdm.h           |   2 +
>  3 files changed, 351 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm_regs.c b/drivers/cxl/core/hdm_regs.c
> index f0b360867655..cef5e9949d2c 100644
> --- a/drivers/cxl/core/hdm_regs.c
> +++ b/drivers/cxl/core/hdm_regs.c
> @@ -73,6 +73,26 @@ int cxld_await_commit(void __iomem *hdm, int id)
>  }
>  EXPORT_SYMBOL_FOR_MODULES(cxld_await_commit, "cxl_core");
>  
> +static int cxld_await_uncommit(void __iomem *hdm, int id)
> +{
> +	u32 ctrl;
> +	int i;
> +
> +	for (i = 0; i < COMMIT_TIMEOUT_MS; i++) {
> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMIT_ERROR, ctrl)) {
> +			ctrl &= ~CXL_HDM_DECODER0_CTRL_COMMIT;
> +			writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +			return -EIO;
> +		}
> +		if (!FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> +			return 0;
> +		fsleep(1000);
> +	}
> +
> +	return -ETIMEDOUT;
> +}

Hi Srirangan,

This looks like a copy of cxld_await_commit() w the final test flipped.
Could cxld_await_commit() take the Committed state to wait for, instead
of copying it here?

> +
>  static u32 cxl_hdm_write_range(void __iomem *hdm,
>  			       const struct cxl_decoder_config *config)
>  {
> @@ -273,3 +293,163 @@ int cxl_hdm_read_state(struct device *dev, void __iomem *hdm,
>  
>  	return 0;
>  }
> +
> +static int cxl_hdm_decoder_uncommit(struct device *dev, void __iomem *hdm,
> +				    int id, bool *retained)

Same ask as on v14.
Can the restore helpers return the error and let cxl_restore_state()
report it? v15 swaps struct pci_dev for struct device, but it still
runs through cxl_hdm_restore_state(), cxl_hdm_restore_decoder(),
this function and cxl_hdm_read_decoder_settings(), only for dev_err().

snip

> +cxl_hdm_restore_decoder(struct device *dev, void __iomem *hdm,
> +			const struct cxl_endpoint_decoder_settings *settings)
> +{
> +	const struct cxl_decoder_config *config = &settings->config;
> +	bool retained;
> +	u32 ctrl;
> +	int rc;
> +
> +	rc = cxl_hdm_decoder_uncommit(dev, hdm, config->id, &retained);
> +	if (rc)
> +		return rc;
> +
> +	if (retained) {
> +		struct cxl_endpoint_decoder_settings active;
> +
> +		rc = cxl_hdm_read_decoder_settings(dev, &active, hdm, config->id);
> +		if (rc) {
> +			dev_err(dev,
> +				"CXL HDM decoder %d has invalid configuration: %d\n",
> +				config->id, rc);


cxl_hdm_read_decoder_settings() already logs this same message, so it
gets logged twice. That goes away with the reporting change above.

snip
> +			config->id, rc);
> +		return rc;
> +	}
> +
> +	if (!(config->flags & CXL_DECODER_F_LOCK))
> +		return 0;
> +
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(config->id));
> +	if (ctrl == U32_MAX)
> +		return -EIO;
> +	ctrl |= CXL_HDM_DECODER0_CTRL_LOCK;
> +	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(config->id));

This reads like we commit the decoder first and set Lock On Commit next.
The spec seems to say the lock only takes effect at the moment the 
decoder commits, making this too late. Looking at spec 8.2.4.20.17.

-- Alison

> +
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(config->id));
> +	if (ctrl == U32_MAX ||
> +	    !(ctrl & CXL_HDM_DECODER0_CTRL_LOCK)) {
> +		dev_err(dev, "CXL HDM decoder %d failed to restore lock\n",
> +			config->id);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +