Re: [PATCH 3/4] cxl: Add late host bridge uport mapping update

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

On 4/22/25 11:10 PM, Dan Williams wrote:
> Dave Jiang wrote:
>> Error message "cxl portN: Couldn't locate the CXL.cache and CXL.mem
>> capability array header" is reported through testing when a platform is
>> enabled with PCIe hotplug. The cxl_acpi module is responsible for
>> enumerating the host bridges through ACPI objects. During the enumeration
>> of the host bridge upstream ports (uports), the root port (RP) registers
>> are mapped. The enumeration can happen as soon as the cxl_acpi module
>> probe() function is called. However if the CXL link between the endpoint
>> device and the RP is not established before the enumeration happens,
>> the platform may not expose DVSEC ID 3 and/or DVSEC ID 7 blocks which
>> triggers the error message.
>>
>> Add an attempt to map the register block under the memdev probe() port
>> enumeration. When the PCI probe of the device endpoint is called, the
>> driver is now communicating with the CXL device and the CXL link is
>> considered established. Doing the register block mapping at that point
>> guarantees that the mandatory DVSEC blocks are present.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/acpi.c      | 17 +++++++++-
>>  drivers/cxl/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++--
>>  drivers/cxl/cxl.h       |  4 +++
>>  drivers/cxl/port.c      | 19 ++---------
>>  4 files changed, 93 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index cb14829bb9be..3c8f04bee9a3 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -662,9 +662,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>>  	if (rc)
>>  		return rc;
>>  
>> -	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
>> +	/*
>> +	 * While there is a chance the uport gets mapped when the probe
>> +	 * function gets called, it is not a guarantee due to acpi driver
>> +	 * can be probed before the root port has established the CXL
>> +	 * connection to the endpoint device. Bypass mapping during
>> +	 * port creation by pass in CXL_RESOURCE_NONE for the
>> +	 * component_reg_phys parameter. After, set the 'resource'
>> +	 * parameter of port->map to allow a setup via the endpoint
>> +	 * memdev probe.
>> +	 */
>> +	port = devm_cxl_add_port(host, bridge, CXL_RESOURCE_NONE, dport);
>>  	if (IS_ERR(port))
>>  		return PTR_ERR(port);
>> +	port->reg_map = (struct cxl_register_map) {
>> +		.host = host,
>> +		.reg_type = CXL_REGLOC_RBI_EMPTY,
>> +		.resource = component_reg_phys,
>> +	};
> 
> This looks racy. How do you know the cxl_mem_probe() path is not messing
> with the reg_map here?
> 
> I feel like the rules about when component_reg_phys is valid to pass to
> devm_cxl_add_port() are getting complicated pass the point of
> maintainability.
> 
> I would be happier if the rule is always "component registers are only
> probed and cxl_ports are only created when a memdev is present".
> 
> What is currently missing for that is that devm_cxl_enumerate_ports()
> does not know how to create the cxl_port instances beneath the cxl_root.
> I think the time has come for that. So cxl_acpi only creates the
> cxl_root and the host-bridges dports, and devm_cxl_enumerate_ports()
> does the rest.
> 
> I do not see anything fundamentally difficult about doing one more level
> of iteration in devm_cxl_enumerate_ports() to check that a host-bridge
> is registered as a dport of the cxl_root and then register a cxl_port
> (if not already created).

Essentially move add_host_bridge_uport() to cxl_core? It looks like we'll have to move some ACPI bits to core then in order to retrieve the CHBS.