Re: [PATCH v7 2/8] cxl/pci: Add BI topology enable/disable

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Topology walk loop exits early, completely skipping CXL Root Ports and breaking BI enablement.
- [High] Missing explicit hardware commit for Endpoints and Root Ports.
- [High] Premature BI disablement on shared Switch DSPs due to missing `nr_bi` reference counting.
- [Medium] Hardware register state is leaked (left enabled) when a downstream port fails to commit.
- [Medium] Writing to the BI_COMMIT control bit without waiting for the BI_COMMITTED status bit to clear introduces a race condition.
--

--- Patch [2]: [PATCH v7 2/8] cxl/pci: Add BI topology enable/disable ---
commit 60c6baf3b8f67e918530663c4dbf82d8c044cc21
Author: Davidlohr Bueso <[email protected]>

cxl/pci: Add BI topology enable/disable

This commit implements cxl_bi_setup() to enable Back-Invalidate (BI) flows
on a device and every component in its path. It also implements the teardown
counterpart cxl_bi_dealloc(). The setup runs during endpoint port probe and
walks the topology to enable BI capabilities.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f82559d8a8c5a..a87c2ad9ac533 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
[ ... ]
> @@ -964,3 +966,380 @@ void devm_cxl_dport_bi_setup(struct cxl_dport *dport)
[ ... ]
> +static int __cxl_bi_commit_decoder(struct device *dev, void __iomem *bi)
> +{
> +	u32 status, ctrl;
> +	unsigned int scale, base;
> +
> +	if (!FIELD_GET(CXL_BI_DECODER_CAPS_EXPLICIT_COMMIT_REQ,
> +		       readl(bi + CXL_BI_DECODER_CAPS_OFFSET)))
> +		return 0;
> +
> +	ctrl = readl(bi + CXL_BI_DECODER_CTRL_OFFSET);
> +	writel(ctrl & ~CXL_BI_DECODER_CTRL_BI_COMMIT,
> +	       bi + CXL_BI_DECODER_CTRL_OFFSET);
> +	writel(ctrl | CXL_BI_DECODER_CTRL_BI_COMMIT,
> +	       bi + CXL_BI_DECODER_CTRL_OFFSET);

[Severity: Medium]
Will this sequence cause a race condition with the subsequent polling loop?

If the hardware was previously in a committed state, the status bit might
still be set when the polling starts.

Could this allow the polling loop to succeed instantly before the hardware has
processed the new commit request?

[ ... ]
> +static int __cxl_bi_ctrl_dport(struct cxl_dport *dport, bool enable)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dport->dport_dev);
> +	void __iomem *bi = dport->regs.bi_decoder;
> +	struct cxl_port *port = dport->port;
> +	u32 ctrl, value;
> +	int rc;
> +
> +	guard(mutex)(&port->bi_lock);
> +	if (!bi)
> +		return -EINVAL;
> +
> +	ctrl = readl(bi + CXL_BI_DECODER_CTRL_OFFSET);
> +
> +	switch (pci_pcie_type(pdev)) {
> +	case PCI_EXP_TYPE_ROOT_PORT:
> +		if (enable) {
> +			/*
> +			 * There is no point of failure from here on,
> +			 * BI will be enabled on the endpoint device.
> +			 */
> +			dport->nr_bi++;
> +
> +			if (FIELD_GET(CXL_BI_DECODER_CTRL_BI_FW, ctrl) &&
> +			    !FIELD_GET(CXL_BI_DECODER_CTRL_BI_ENABLE, ctrl))
> +				return 0;
> +
> +			value = ctrl | CXL_BI_DECODER_CTRL_BI_FW;
> +			value &= ~CXL_BI_DECODER_CTRL_BI_ENABLE;
> +		} else {
> +			if (WARN_ON_ONCE(dport->nr_bi == 0))
> +				return -EINVAL;
> +			if (--dport->nr_bi > 0)
> +				return 0;
> +
> +			value = ctrl & ~(CXL_BI_DECODER_CTRL_BI_FW |
> +					 CXL_BI_DECODER_CTRL_BI_ENABLE);
> +		}
> +
> +		writel(value, bi + CXL_BI_DECODER_CTRL_OFFSET);
> +		return 0;

[Severity: High]
Does this code need to explicitly commit the BI Decoder for Root Ports?

The control register is updated, but a hardware commit is never performed.

Will this fail to enable BI on Root Ports that require an explicit commit?

> +	case PCI_EXP_TYPE_DOWNSTREAM:
> +		if (enable) {
> +			value = ctrl & ~CXL_BI_DECODER_CTRL_BI_FW;
> +			value |= CXL_BI_DECODER_CTRL_BI_ENABLE;
> +		} else {
> +			if (!FIELD_GET(CXL_BI_DECODER_CTRL_BI_ENABLE, ctrl))
> +				return 0;
> +			value = ctrl & ~(CXL_BI_DECODER_CTRL_BI_FW |
> +					 CXL_BI_DECODER_CTRL_BI_ENABLE);
> +		}
> +
> +		writel(value, bi + CXL_BI_DECODER_CTRL_OFFSET);

[Severity: High]
Is there a missing reference count for Downstream Ports here?

Unlike Root Ports which check dport->nr_bi, this unconditionally clears the
CXL_BI_DECODER_CTRL_BI_ENABLE bit when disabling.

If this is a shared Switch DSP in a cascaded switch topology, will unbinding
one endpoint instantly break BI functionality for all other active endpoints
below the same upstream switch?

[ ... ]
> +static int __cxl_bi_ctrl_endpoint(struct cxl_dev_state *cxlds, bool enable)
> +{
> +	struct cxl_port *endpoint = cxlds->cxlmd->endpoint;
> +	void __iomem *bi = endpoint->regs.bi_decoder;
> +	u32 ctrl, val;
[ ... ]
> +	writel(val, bi + CXL_BI_DECODER_CTRL_OFFSET);
> +	cxlds->bi = enable;
> +
> +	dev_dbg(cxlds->dev, "BI requests %s\n",
> +		str_enabled_disabled(enable));
> +
> +	return 0;
> +}

[Severity: High]
Does the endpoint need an explicit hardware commit here?

Similar to the Root Port case, this modifies the CXL_BI_DECODER_CTRL_BI_ENABLE
bit but never calls __cxl_bi_commit_decoder() to commit the change to
hardware.

[ ... ]
> +static void cxl_bi_dealloc(void *data)
> +{
> +	struct cxl_port *endpoint = data;
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(endpoint->uport_dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_dport *dport_iter;
> +	struct cxl_port *port_iter;
> +
> +	if (!cxlds->bi)
> +		return;
> +
> +	scoped_guard(rwsem_read, &cxl_rwsem.region)
> +		cxl_bi_ctrl_endpoint_disable(cxlds);
> +
> +	/*
> +	 * Walk the same parent_dport chain that enabled the path. A bus
> +	 * lookup cannot stand in for it: an ancestor-driven teardown
> +	 * delists the parent port before this devres action runs.
> +	 */
> +	dport_iter = endpoint->parent_dport;
> +	port_iter = dport_iter->port;
> +	while (!is_cxl_root(port_iter)) {
> +		int rc = cxl_bi_ctrl_dport_disable(dport_iter);

[Severity: High]
Does this loop terminate before processing the Root Port?

When port_iter reaches the top, its owning port is cxl_root. Since
is_cxl_root(cxl_root) evaluates to true, it looks like the loop exits
immediately before executing the disable body for the Root Port.

[ ... ]
> +static int cxl_bi_enable_path(struct cxl_dev_state *cxlds,
> +			      struct cxl_port *port, struct cxl_dport *dport)
> +{
> +	struct cxl_dport *dport_iter, *failed;
> +	struct cxl_port *port_iter;
> +	int rc;
> +
> +	port_iter = port;
> +	dport_iter = dport;
> +	while (!is_cxl_root(port_iter)) {
> +		rc = cxl_bi_ctrl_dport_enable(dport_iter);
> +		if (rc)
> +			goto err_rollback;
> +
> +		dport_iter = port_iter->parent_dport;
> +		port_iter = dport_iter->port;
> +	}

[Severity: High]
Does this also skip enabling the Root Port?

Using the same !is_cxl_root() condition as cxl_bi_dealloc(), this loop will
exit before calling cxl_bi_ctrl_dport_enable() for the Root Port, leaving the
host path incomplete.

[ ... ]
> +err_rollback:
> +	failed = dport_iter;
> +	dport_iter = dport;
> +	port_iter = port;
> +	while (!is_cxl_root(port_iter) && dport_iter != failed) {
> +		cxl_bi_ctrl_dport_disable(dport_iter);
> +		dport_iter = port_iter->parent_dport;
> +		port_iter = dport_iter->port;
> +	}
> +	return rc;
> +}

[Severity: Medium]
Should the failed dport be included in the rollback loop?

If cxl_bi_ctrl_dport_enable() writes the enable bit but the hardware commit
fails, the control register has still been modified. 

By stopping the rollback when dport_iter != failed,
cxl_bi_ctrl_dport_disable() is never called for it, potentially leaking the
hardware register state as left enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2