Re: [PATCH v4 9/9] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 24 Jun 2025 14:39:16 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Current enuemration scheme in cxl_acpi module creates the ports under the
> root port by enumerating the hostbridges after the dports under the root
> port is created. However error messages "cxl portN: Couldn't locate the
> CXL.cache and CXL.mem capability array header" is observed when certain
> platform has PCIe hotplug option turned on in BIOS. If the cxl_acpi module
> probe is running before the CXL link between the endpoint device and the
> RP is established, then the platform may not have exposed DVSEC ID 3 and/or
> DVSEC ID 7 blocks which will trigger the error message. This behavior
> is defined by the spec and not a hardware quirk.
> 
> Setup an association in cxl_port to tie the host bridge device to the
> associated cxl_root. The cxl_root provides a callback that's setup
> by the cxl_acpi probe function in order to create a port per host bridge
> that was previously done during cxl_acpi probe. Add the calling of the
> callback in devm_cxl_enumerate_ports(). The observed behavior is that
> ports that are not connected to endpoint device(s) are no longer
> enumerated. This should also remove any excessive noise of port probe
> failing on those inactive ports.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v4:
> - Reworked against new delayed dport allocation mechanism

One comment inline.

This whole things is complex enough I'm not feeling particularly confident
in my reviewing - so definitely needs a lot more eyes.  With that in mind.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 20d2f834bf94..b1d19c609dde 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c

> +
> +static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
> +{
> +	struct device *hb_uport_dev, *hb_dport_dev;
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	rc = get_hostbridge_port_devices(cxlmd, &hb_uport_dev, &hb_dport_dev);
> +	if (rc)
> +		return -ENODEV;
> +
> +	struct cxl_root *cxl_root __free(put_cxl_root) =
> +		cxl_hb_uport_dev_to_root(hb_uport_dev);
> +	if (!cxl_root)
> +		return -ENODEV;
> +
> +	guard(device)(&cxl_root->port.dev);
> +	struct cxl_port *port __free(put_cxl_port) =
> +		find_cxl_port(hb_dport_dev, &dport);

I'm not certain that there isn't a path to dport being uninitialized
if !port.  Maybe we always get passed the early checks in match_port_by_dport()
at least once.  Event if it is safe, I suspect we might get false positive
reports as a compiler or static analysis tool is going to struggle to figure
that out.


> +	if (!port)
> +		port = find_cxl_port_by_uport(hb_uport_dev);
> +
> +	/* Port already established, add the associated dport if needed. */
> +	if (port) {
> +		if (dport)
> +			return 0;
> +
> +		guard(device)(&port->dev);
> +		return devm_cxl_port_add_dport(port, hb_dport_dev, &dport);
> +	}
> +
> +	/* No port found, setup a port via the root port ops */
> +	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
> +		return -EOPNOTSUPP;
> +
> +	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, hb_uport_dev);
> +	if (rc)
> +		return rc;
> +
> +	/* Add the dport that goes with the newly created port */
> +	return devm_cxl_add_dport_by_uport(hb_uport_dev, hb_dport_dev, &dport);
> +}
> +
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  {
>  	struct device *dev = &cxlmd->dev;
> @@ -1826,6 +1889,10 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  	if (cxlmd->cxlds->rcd)
>  		return 0;
>  
> +	rc = cxl_hostbridge_port_setup(cxlmd);
> +	if (rc)
> +		return rc;
> +
>  	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
>  	if (rc)
>  		return rc;