Re: [PATCH v5 2/5] cxl/pci: Add BI topology enable/disable

On Mon, 15 Jun 2026, [email protected] wrote:

>Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>- [High] Mixing `goto` error handling with `__free()` scope-based cleanup declarations.

Trivial, fixed for v6. See below.

>- [High] Missing explicit commit for the Endpoint BI Decoder Control Register.

False positive, this commit does not apply to endpoints.

>- [High] Root port BI enablement logic is unreachable and contradicts the commit message.

False positive.

Sashiko read !is_cxl_root(port_iter) as "the RP is skipped," but the RP sits a
level below the platform root.

>- [High] Missing reference counting (`nr_bi`) for shared downstream ports in cascaded switch topologies.

I am avoiding cascaded switch topologies (PBR/HBR), nor does this touch at all wrt
BI in the latest spec.

See: https://lore.kernel.org/all/20260608000226.jb2ploxc5nkjestk@offworld/

>--
>
>commit 31994c2bf9d14374bacdb07d505a19b18a761e35
>Author: Davidlohr Bueso <[email protected]>
>
>cxl/pci: Add BI topology enable/disable
>
>This commit implements cxl_bi_setup() to walk the CXL port topology and enable
>BI flows on the device and every component in the path. It also implements a
>teardown counterpart, cxl_bi_dealloc().
>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index d1f487b3d809a..b1b0f3eab90e8 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>
>[ ... ]
>
>> +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;
>
>[ ... ]
>
>> +	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);
>> +		}
>
>[Severity: High]
>In cascaded switch topologies, can this unconditionally clear the
>CXL_BI_DECODER_CTRL_BI_ENABLE bit while other endpoints below the switch
>are still actively using it?
>
>Unlike the root port case which uses dport->nr_bi for reference counting,
>downstream ports appear to clear this bit as soon as the first endpoint
>unbinds. This might break BI routing for other active endpoints on a shared
>downstream port.
>
>Does this need reference counting similar to the root port?
>
>[ ... ]
>
>> +static int __cxl_bi_ctrl_endpoint(struct cxl_dev_state *cxlds, bool enable)
>> +{
>
>[ ... ]
>
>> +	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]
>Is this missing an explicit commit for the Endpoint BI Decoder Control
>Register?
>
>The CXL 4.0 specification requires an explicit commit after changing
>BI_ENABLE for any BI Decoder via CXL_BI_DECODER_CAPS_EXPLICIT_COMMIT_REQ.
>
>While __cxl_bi_ctrl_dport() correctly calls __cxl_bi_commit_decoder() after
>updating the downstream ports, __cxl_bi_ctrl_endpoint() omits this step.
>
>Could this cause the endpoint device to ignore the BI enablement state and
>silently fail to enable BI flows?
>
>[ ... ]
>
>> +int cxl_bi_setup(struct cxl_dev_state *cxlds)
>> +{
>> +	struct cxl_port *endpoint = cxlds->cxlmd->endpoint;
>> +	struct cxl_dport *dport_iter, *dport, *failed;
>> +	struct cxl_port *port_iter;
>> +	struct pci_dev *pdev;
>> +	int rc;
>> +
>> +	if (!dev_is_pci(cxlds->dev))
>> +		return 0;
>> +
>> +	pdev = to_pci_dev(cxlds->dev);
>> +	struct cxl_port *port __free(put_cxl_port) =
>> +		cxl_pci_find_port(pdev, &dport);
>> +
>> +	if (!port)
>> +		return -EINVAL;
>
>[ ... ]
>
>> +	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;
>
>[Severity: High]
>The commit message states this will write BI Enable on each dport's BI
>Decoder Control register. However, doesn't this loop terminate before
>processing the root port?
>
>Since the loop condition is !is_cxl_root(port_iter), when port_iter reaches
>the CXL root, the loop exits and skips configuring the root port entirely.
>
>Additionally, if the loop did reach the PCI_EXP_TYPE_ROOT_PORT code inside
>__cxl_bi_ctrl_dport(), that code actually clears the enable bit:
>
>__cxl_bi_ctrl_dport() {
>    ...
>    case PCI_EXP_TYPE_ROOT_PORT:
>        ...
>        value &= ~CXL_BI_DECODER_CTRL_BI_ENABLE;
>}
>
>Should this loop include the root port, and should the root port logic actually
>set the enable bit rather than clearing it?
>
>[ ... ]
>
>> +	/* finally, enable BI on the device */
>> +	rc = cxl_bi_ctrl_endpoint_enable(cxlds);
>> +	if (rc)
>> +		goto err_rollback;
>> +
>> +	return devm_add_action_or_reset(&cxlds->cxlmd->dev,
>> +					cxl_bi_dealloc, cxlds);
>> +
>> +err_rollback:
>> +	/*
>> +	 * Undo all dports enabled so far, including the failed one.
>> +	 */
>> +	failed = dport_iter;
>> +	dport_iter = dport;
>> +	port_iter = port;
>> +	while (!is_cxl_root(port_iter)) {
>> +		cxl_bi_ctrl_dport_disable(dport_iter);
>> +		if (dport_iter == failed)
>> +			break;
>> +		dport_iter = port_iter->parent_dport;
>> +		port_iter = dport_iter->port;
>> +	}
>> +	return rc;
>> +}
>
>[Severity: High]
>Is it safe to mix goto-based error handling with scope-based cleanup in
>this function?
>
>This function declares struct cxl_port *port __free(put_cxl_port) at the
>top, but also uses a goto err_rollback pattern for failure paths.
>
>Mixing goto labels with __free() declarations in the same function creates
>confusing ownership semantics and violates subsystem cleanup guidelines.
>
>Should this be fully converted to scope-based cleanup, or avoid using
>__free() here?

Sure. I went with this this change/unwind consolidation, fyi:

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index ed54268e9ec7..dc700f7c75c0 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -1247,10 +1247,52 @@ static void cxl_bi_dealloc(void *data)
  	}
  }
  
+/*
+ * Enable BI on every dport in the path, then on the device itself.
+ * On failure, unwind only the dports that fully enabled.
+ */
+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;
+	}
+
+	/* finally, enable BI on the device */
+	rc = cxl_bi_ctrl_endpoint_enable(cxlds);
+	if (rc)
+		goto err_rollback;
+
+	return 0;
+
+err_rollback:
+	/* Undo dports that fully enabled; skip the failed one. */
+	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;
+}
+
  int cxl_bi_setup(struct cxl_dev_state *cxlds)
  {
  	struct cxl_port *endpoint = cxlds->cxlmd->endpoint;
-	struct cxl_dport *dport_iter, *dport, *failed;
+	struct cxl_dport *dport_iter, *dport;
  	struct cxl_port *port_iter;
  	struct pci_dev *pdev;
  	int rc;
@@ -1293,35 +1335,11 @@ int cxl_bi_setup(struct cxl_dev_state *cxlds)
  		port_iter = dport_iter->port;
  	}
  
-	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;
-	}
-
-	/* finally, enable BI on the device */
-	rc = cxl_bi_ctrl_endpoint_enable(cxlds);
+	rc = cxl_bi_enable_path(cxlds, port, dport);
  	if (rc)
-		goto err_rollback;
+		return rc;
  
  	return devm_add_action_or_reset(&cxlds->cxlmd->dev,
  					cxl_bi_dealloc, cxlds);
-
-err_rollback:
-	/* Undo dports that fully enabled; skip the failed one. */
-	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;
  }
  EXPORT_SYMBOL_NS_GPL(cxl_bi_setup, "CXL");