Re: [PATCH v9 06/10] cxl: Defer dport allocation for switch ports

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

On Fri, 29 Aug 2025 11:09:24 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The current implementation enumerates the dports during the cxl_port
> driver probe. Without an endpoint connected, the dport may not be
> active during port probe. This scheme may prevent a valid hardware
> dport id to be retrieved and MMIO registers to be read when an endpoint
> is hot-plugged. Move the dport allocation and setup to behind memdev
> probe so the endpoint is guaranteed to be connected.
> 
> In the original enumeration behavior, there are 3 phases (or 2 if no CXL
> switches) for port creation. cxl_acpi() creates a Root Port (RP) from the
> ACPI0017.N device. Through that it enumerates downstream ports composed
> of ACPI0016.N devices through add_host_bridge_dport(). Once done, it
> uses add_host_bridge_uport() to create the ports that enumerate the PCI
> RPs as the dports of these ports. Every time a port is created, the port
> driver is attached, cxl_switch_porbe_probe() is called and
> devm_cxl_port_enumerate_dports() is invoked to enumerate and probe
> the dports.
> 
> The second phase is if there are any CXL switches. When the pci endpoint
> device driver (cxl_pci) calls probe, it will add a mem device and triggers
> the cxl_mem_probe(). cxl_mem_probe() calls devm_cxl_enumerate_ports()
> and attempts to discovery and create all the ports represent CXL switches.
> During this phase, a port is created per switch and the attached dports
> are also enumerated and probed.
> 
> The last phase is creating endpoint port which happens for all endpoint
> devices.
> 
> The new sequence is instead of creating all possible dports at initial
> port creation, defer port instantiation until a memdev beneath that
> dport arrives. Introduce devm_cxl_create_or_extend_port() to centralize
> the creation and extension of ports with new dports as memory devices
> arrive. As part of this rework, switch decoder target list is amended
> at runtime as dports show up.
> 
> While the decoders are allocated during the port driver probe,
> The decoders must also be updated since previously it's all done when all

it was all done?

> the dports are setup and now every time a dport is setup per endpoint, the
> switch target listing need to be updated with new dport. A
> guard(rwsem_write) is used to update decoder targets. This is similar to
> when decoder_populate_target() is called and the decoder programming
> must be protected.
> 
> Also the port registers are probed the first time when the first dport
> shows up. This ensures that the CXL link is established when the port
> registers are probed.
> 
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Hi Dave,

This isn't one of my more confident reviews but I think this looks ok.
Definitely could do with more eyes though. In the ideal world I'd emulate
a bunch of test cases to poke this with, but I'm snowed under for a while
so hopefully testing on the platforms that exhibit the problem will be enough.

One trivial comment inline.

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

> 
> ---
> v9:
> - Dropped tested-by tag by Robert since significant code changes.
>   Also dropped review tags.
> - Rework iteration loop. (Robert)
> - Dropped possible_ports in 'struct cxl_port' (Robert)
> - Stop the update_decoder_targets() iteration when found. (Robert)
> - Use number of decoders to gate decoder allocation.
> - cxl_port_get_or_add_dport() ->cxl_port_add_dport(). (Robert)
> - Unfold cxl_decoders_dport_update() (Robert)
> - Remove devm_cxl_add_dport_by_uport() (Robert)


> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index fa02366d35f2..9ec288ed39ae 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c

>  struct cxl_walk_context {
>  	struct pci_bus *bus;
>  	struct cxl_port *port;
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index f379e4c5121d..fa7ed5d50064 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c

> +static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
> +					      struct cxl_port *parent_port,
> +					      struct cxl_dport *parent_dport,
> +					      struct device *uport_dev,
> +					      struct device *dport_dev)
> +{
> +	resource_size_t component_reg_phys;
> +
> +	device_lock_assert(&parent_port->dev);
> +	if (!parent_port->dev.driver) {
> +		dev_warn(ep_dev,
> +			 "port %s:%s:%s disabled, failed to enumerate CXL.mem\n",
> +			 dev_name(&parent_port->dev), dev_name(uport_dev),
> +			 dev_name(dport_dev));
> +	}
> +
> +	struct cxl_port *port __free(put_cxl_port) =
> +		find_cxl_port_by_uport(uport_dev);
> +	if (!port) {
> +		component_reg_phys = find_component_registers(uport_dev);
> +		port = devm_cxl_add_port(&parent_port->dev, uport_dev,
> +					 component_reg_phys, parent_dport);
> +		if (IS_ERR(port))
> +			return (struct cxl_dport *)port;

			return ERR_CAST(port);
Just to make it even more obvious what is going on.

> +
> +		/*
> +		 * retry to make sure a port is found. a port device
> +		 * reference is taken.
> +		 */
> +		port = find_cxl_port_by_uport(uport_dev);
> +		if (!port)
> +			return ERR_PTR(-ENODEV);
> +
> +		dev_dbg(ep_dev, "created port %s:%s\n",
> +			dev_name(&port->dev), dev_name(port->uport_dev));
> +	} else {
> +		/*
> +		 * Port was created before right before this function is
> +		 * called. Signal the caller to deal with it.
> +		 */
> +		return ERR_PTR(-EAGAIN);
> +	}
> +
> +	guard(device)(&port->dev);
> +	return cxl_port_add_dport(port, dport_dev);
> +}