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

[sashiko-bot@kernel.org]

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.
- [High] Missing explicit commit for the Endpoint BI Decoder Control Register.
- [High] Root port BI enablement logic is unreachable and contradicts the commit message.
- [High] Missing reference counting (`nr_bi`) for shared downstream ports in cascaded switch topologies.
--

commit 31994c2bf9d14374bacdb07d505a19b18a761e35
Author: Davidlohr Bueso <dave@stgolabs.net>

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615145529.13848-1-dave@stgolabs.net?part=2