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

On Tue, 28 Jul 2026, [email protected] 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 Port=
s 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 downstr=
eam port fails to commit.

Yes, that's a corner case which I've fixed up but should not be a show stop=
per 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.

=2E..

>> +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 =3D port;
>> +	dport_iter =3D dport;
>> +	while (!is_cxl_root(port_iter)) {
>> +		rc =3D cxl_bi_ctrl_dport_enable(dport_iter);
>> +		if (rc)
>> +			goto err_rollback;
>> +
>> +		dport_iter =3D port_iter->parent_dport;
>> +		port_iter =3D 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 =3D dport_iter;
>> +	dport_iter =3D dport;
>> +	port_iter =3D port;
>> +	while (!is_cxl_root(port_iter) && dport_iter !=3D failed) {
>> +		cxl_bi_ctrl_dport_disable(dport_iter);
>> +		dport_iter =3D port_iter->parent_dport;
>> +		port_iter =3D 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 *dp=
ort, bool enable)
		writel(value, bi + CXL_BI_DECODER_CTRL_OFFSET);

		rc =3D __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 =3D __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 !=3D failed,
>cxl_bi_ctrl_dport_disable() is never called for it, potentially leaking the
>hardware register state as left enabled.
>
>--
>Sashiko AI review =B7 https://sashiko.dev/#/patchset/20260728144136.709882=
[email protected]?part=3D2