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

[Davidlohr Bueso <dave@stgolabs.net>]

On Tue, 28 Jul 2026, sashiko-bot@kernel.org wrote:

>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.

No, we already covered this.

>- [High] Missing explicit hardware commit for Endpoints and Root Ports.

No, we already covered this.

>- [High] Premature BI disablement on shared Switch DSPs due to missing `nr_bi` reference counting.

No, we already covered this.

>- [Medium] Hardware register state is leaked (left enabled) when a downstream port fails to commit.

Yes, that's a corner case which I've fixed up but should not be a show stopper for v7.
The fix is trivial, see below.

>- [Medium] Writing to the BI_COMMIT control bit without waiting for the BI_COMMITTED status bit to clear introduces a race condition.

False positive. This is guaranteed per hw - see Table 8-158.

...

>> +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?

No, we've already covered that. But we can fix the register
rollback:

--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -1161,12 +1161,11 @@ static int __cxl_bi_ctrl_dport(struct cxl_dport *dport, bool enable)
		writel(value, bi + CXL_BI_DECODER_CTRL_OFFSET);

		rc = __cxl_bi_commit_decoder(dport->dport_dev, bi);
-		if (rc)
-			return rc;
-
-		if (port->regs.bi_rt)
-			return __cxl_bi_commit_rt(&port->dev, port->regs.bi_rt);
-		return 0;
+		if (!rc && port->regs.bi_rt)
+			rc = __cxl_bi_commit_rt(&port->dev, port->regs.bi_rt);
+		if (rc && enable) /* undo previous enablement */
+			writel(ctrl, bi + CXL_BI_DECODER_CTRL_OFFSET);
+		return rc;
	default:
		return -EINVAL;
	}


>
>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/20260728144136.709882-1-dave@stgolabs.net?part=2