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

[Dave Jiang <dave.jiang@intel.com>]

On 7/1/25 4:32 AM, Jonathan Cameron wrote:
> 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.

I'm not quite sure I understand what you are saying here.

DJ

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