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

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

On 4/10/25 7:32 PM, Li Ming wrote:
> On 4/5/2025 6:57 AM, 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,
>> +	};
>>  
>>  	dev_info(bridge, "host supports CXL\n");
>>  
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 1c772c516dbe..8c29db214d60 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -758,6 +758,10 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>>  static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map,
>>  			       resource_size_t component_reg_phys)
>>  {
>> +	/* Skip the setup if the map has been setup previously. */
>> +	if (map->reg_type != CXL_REGLOC_RBI_EMPTY)
>> +		return 0;
>> +
>>  	*map = (struct cxl_register_map) {
>>  		.host = host,
>>  		.reg_type = CXL_REGLOC_RBI_EMPTY,
>> @@ -773,7 +777,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map
>>  	return cxl_setup_regs(map);
>>  }
>>  
>> -static int cxl_port_setup_regs(struct cxl_port *port,
>> +int cxl_port_setup_regs(struct cxl_port *port,
>>  			resource_size_t component_reg_phys)
>>  {
>>  	if (dev_is_platform(port->uport_dev))
>> @@ -781,6 +785,7 @@ static int cxl_port_setup_regs(struct cxl_port *port,
>>  	return cxl_setup_comp_regs(&port->dev, &port->reg_map,
>>  				   component_reg_phys);
>>  }
>> +EXPORT_SYMBOL_NS_GPL(cxl_port_setup_regs, "CXL");
>>  
>>  static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>>  				resource_size_t component_reg_phys)
>> @@ -1004,7 +1009,7 @@ int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(devm_cxl_register_pci_bus, "CXL");
>>  
>> -static bool dev_is_cxl_root_child(struct device *dev)
>> +bool dev_is_cxl_root_child(struct device *dev)
>>  {
>>  	struct cxl_port *port, *parent;
>>  
>> @@ -1021,6 +1026,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
>>  
>>  	return false;
>>  }
>> +EXPORT_SYMBOL_NS_GPL(dev_is_cxl_root_child, "CXL");
>>  
>>  struct cxl_root *find_cxl_root(struct cxl_port *port)
>>  {
>> @@ -1565,6 +1571,57 @@ static resource_size_t find_component_registers(struct device *dev)
>>  	return map.resource;
>>  }
>>  
>> +int devm_cxl_decoders_setup(struct cxl_port *port)
>> +{
>> +	struct cxl_dport *dport;
>> +	struct cxl_hdm *cxlhdm;
>> +	unsigned long index;
>> +	int dports = 0;
>> +
>> +	/* Make sure that no decoders have been allocated before proceeding. */
>> +	if (!ida_is_empty(&port->decoder_ida))
>> +		return 0;
>> +
>> +	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>> +	if (!IS_ERR(cxlhdm))
>> +		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>> +
>> +	if (PTR_ERR(cxlhdm) != -ENODEV) {
>> +		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
>> +		return PTR_ERR(cxlhdm);
>> +	}
>> +
>> +	xa_for_each(&port->dports, index, dport)
>> +		dports++;
>> +
>> +	if (dports == 1) {
>> +		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>> +		return devm_cxl_add_passthrough_decoder(port);
>> +	}
>> +
>> +	dev_err(&port->dev, "HDM decoder capability not found\n");
>> +	return -ENXIO;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_decoders_setup, "CXL");
>> +
>> +static int cxl_hb_port_setup(struct cxl_port *port)
>> +{
>> +	int rc;
>> +
>> +	device_lock_assert(&port->dev);
>> +
>> +	if (!dev_is_cxl_root_child(&port->dev))
>> +		return 0;
>> +
>> +	cxl_port_probe_dports(port);
>> +
>> +	rc = cxl_port_setup_regs(port, port->reg_map.resource);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return devm_cxl_decoders_setup(port);
>> +}
>> +
>>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			      struct device *uport_dev,
>>  			      struct device *dport_dev)
>> @@ -1605,6 +1662,17 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			return -ENXIO;
>>  		}
>>  
>> +		/*
>> +		 * Initiate delayed host bridge setup here in the path of memdev probe
>> +		 * to ensure that the CXL link is established.
>> +		 */
>> +		rc = cxl_hb_port_setup(parent_port);
>> +		if (rc) {
>> +			dev_warn(&cxlmd->dev, "Failed HB port setup of %s.\n",
>> +				 dev_name(&parent_port->dev));
>> +			return rc;
>> +		}
>> +
> 
> My understanding is that add_port_attach_ep() is only called for cxl switch port adding. cxl hb ports are already added during cxl acpi probing, they can be found in devm_cxl_enumerate_ports().
> 
> So if an endpoint directly connects to a root port, seems like no chance to call cxl_hb_port_setup() in add_port_attach_ep().

Thanks for pointing that out. I need to modify my qemu setup to test this scenario.

DJ

> 
> 
>>  		port = find_cxl_port_at(parent_port, dport_dev, &dport);
>>  		if (!port) {
>>  			component_reg_phys = find_component_registers(uport_dev);
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 0e61b76f5c13..b27e9d3306fe 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -906,6 +906,10 @@ void cxl_coordinates_combine(struct access_coordinate *out,
>>  			     struct access_coordinate *c2);
>>  
>>  bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
>> +int devm_cxl_decoders_setup(struct cxl_port *port);
>> +bool dev_is_cxl_root_child(struct device *dev);
>> +int cxl_port_setup_regs(struct cxl_port *port,
>> +			resource_size_t component_reg_phys);
>>  
>>  /*
>>   * Unit test builds overrides this to __weak, find the 'strong' version
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index 30c0335089b9..dc532ee9065f 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -59,7 +59,6 @@ static int discover_region(struct device *dev, void *root)
>>  
>>  static int cxl_switch_port_probe(struct cxl_port *port)
>>  {
>> -	struct cxl_hdm *cxlhdm;
>>  	int rc;
>>  
>>  	/* Cache the data early to ensure is_visible() works */
>> @@ -69,22 +68,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  	if (rc < 0)
>>  		return rc;
>>  
>> -	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>> -	if (!IS_ERR(cxlhdm))
>> -		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>> +	if (dev_is_cxl_root_child(&port->dev))
>> +		return 0;
>>  
>> -	if (PTR_ERR(cxlhdm) != -ENODEV) {
>> -		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
>> -		return PTR_ERR(cxlhdm);
>> -	}
>> -
>> -	if (rc == 1) {
>> -		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>> -		return devm_cxl_add_passthrough_decoder(port);
>> -	}
>> -
>> -	dev_err(&port->dev, "HDM decoder capability not found\n");
>> -	return -ENXIO;
>> +	return devm_cxl_decoders_setup(port);
>>  }
>>  
>>  static int cxl_endpoint_port_probe(struct cxl_port *port)
> 
>