Re: [PATCH v6 1/9] cxl/hdm: Add helpers to restore and commit memdev decoders

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying CXL HDM decoder base/size registers before ensuring the decoder is uncommitted violates the CXL specification and introduces a race condition.
- [High] `cxl_restore_dvsec_range()` propagates positive PCIBIOS_* error codes instead of returning standard negative errnos, violating the expected API contract.
--

commit 9296092c66325a5bcb4efe105b5c2a5e625c1363
Author: Srirangan Madhavan <smadhavan@nvidia.com>
Subject: cxl/hdm: Add helpers to restore and commit memdev decoders

Add helpers to restore endpoint decoder programming for a CXL memdev from
CXL core's cached decoder objects, then commit it as a distinct step.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..f7af1041a9fc2 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> +static int cxl_restore_dvsec_range(struct cxl_memdev *cxlmd,
> +				   struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_decoder *cxld = &cxled->cxld;
> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	u64 base = cxld->hpa_range.start;
> +	u64 size = range_len(&cxld->hpa_range);
> +	u32 lo;
> +	int dvsec = cxlds->cxl_dvsec;
> +	int id = cxld->id;
> +	int rc;
> +
> +	if (!dvsec)
> +		return 0;
> +
> +	if (id >= CXL_DVSEC_RANGE_MAX)
> +		return 0;
> +
> +	rc = pci_write_config_dword(pdev, dvsec + PCI_DVSEC_CXL_RANGE_BASE_HIGH(id),
> +				    upper_32_bits(base));
> +	if (rc)
> +		return rc;

[Severity: High]
Since pci_write_config_dword() returns positive PCIBIOS_* error codes on
failure, will this function return a positive value instead of a standard
negative errno?

This propagates up to cxl_restore_memdev_decoders(), whose documentation
states it returns negative errno on failure. Callers checking if (rc < 0)
might treat these failures as success. Should this use pcibios_err_to_errno()
to translate the error values?

[ ... ]

> +static int cxl_restore_hdm_decoder(struct cxl_hdm *cxlhdm,
> +				   struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_decoder *cxld = &cxled->cxld;
> +	void __iomem *hdm;
> +	u64 base, size, skip;
> +	u32 ctrl;
> +	int id;
> +
> +	id = cxld->id;
> +	hdm = cxlhdm->regs.hdm_decoder;
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +	if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> +		return 0;
> +
> +	base = cxld->hpa_range.start;
> +	size = range_len(&cxld->hpa_range);
> +	skip = cxled->skip;
> +
> +	ctrl &= ~(CXL_HDM_DECODER0_CTRL_LOCK |
> +		  CXL_HDM_DECODER0_CTRL_COMMIT |
> +		  CXL_HDM_DECODER0_CTRL_COMMITTED |
> +		  CXL_HDM_DECODER0_CTRL_COMMIT_ERROR);
> +	if (cxld_set_interleave_fields(cxld, &ctrl))
> +		return -EINVAL;
> +	cxld_set_type(cxld, &ctrl);
> +
> +	/* Preserve setup_hw_decoder() programming order, without COMMIT. */
> +	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
> +	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> +	writel(upper_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
> +	writel(lower_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
> +	writel(upper_32_bits(skip), hdm + CXL_HDM_DECODER0_SKIP_HIGH(id));
> +	writel(lower_32_bits(skip), hdm + CXL_HDM_DECODER0_SKIP_LOW(id));
> +	wmb();
> +	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));

[Severity: High]
Does this violate the CXL specification by writing to the range registers
before ensuring the hardware un-commit is complete?

The code clears the COMMIT bit in the local variable but writes to the BASE,
SIZE, and SKIP registers before writing the updated control register to
hardware. If the hardware decoder was previously committed, changing these
registers is not allowed until the COMMITTED bit actually reads as 0.

Also, since the un-commit is only initiated at the end of this function,
could cxl_commit_restored_hdm_decoder() read the control register before the
hardware finishes un-committing? It might see COMMITTED == 1 and return 0
without setting COMMIT, leaving the decoder uncommitted and traffic disabled.

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