Re: [PATCH v5 09/10] cxl: Move enumeration of hostbridge ports to the memdev probe path

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

On Thu, 3 Jul 2025 16:27:09 -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.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v5:
> - Updated with new devm_cxl_port_add_dport() and friends.
Hi Dave,

One of the bits of code that I was confused by in the patch that
added devm_cxl_port_add_dport() goes away in here. I'm not
sure why.

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1089fce24293..c624f4801b68 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1789,14 +1789,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  
>  		guard(device)(&port->dev);
>  		new_dport = devm_cxl_port_add_dport(port, dport_dev);
> -		if (IS_ERR(new_dport)) {
> -			if (PTR_ERR(new_dport) != -EEXIST)
> -				return PTR_ERR(new_dport);

Interesting. Is this in the wrong patch?

> -
> -			new_dport = cxl_find_dport_by_dev(port, dport_dev);
> -			if (!new_dport)
> -				return -ENODEV;
> -		}
> +		if (IS_ERR(new_dport))
> +			return PTR_ERR(new_dport);
>  
>  		dport = new_dport;
>  	}

> +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);
> +	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;

The sequence I badly explained on v4 still bothers me here as I think
it might be possible to hit that if (dport) with dport never initialized.
Even if it isn't, I'd be surprised if the compiler / static analysis can
see that there is no such path.  So if we think it can't happen, just initialize
dport at declaration. If it can happen, I think it being null is still the
right answer, but maybe I'm missing something.


> +
> +		guard(device)(&port->dev);
> +		dport = devm_cxl_port_add_dport(port, hb_dport_dev);
> +		if (IS_ERR(dport)) {
> +			dev_dbg(&cxlmd->dev,
> +				"failed to add dport %s to port %s: %ld\n",
> +				dev_name(hb_dport_dev), dev_name(&port->dev),
> +				PTR_ERR(dport));
> +			return PTR_ERR(dport);
> +		}
> +		return 0;
> +	}