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

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

On Thu, 3 Jul 2025 16:22:38 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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.
Need more caffeine and a spell checker that day it seems.

My question was whether we can have (or a static analysis tool thinks we can
have)
1. port == NULL here, dport not initialized.
> 
> DJ
> 
> > 
> >   
> >> +	if (!port)
> >> +		port = find_cxl_port_by_uport(hb_uport_dev);

2. Then this successfully sets port.

> >> +
> >> +	/* Port already established, add the associated dport if needed. */
> >> +	if (port) {
> >> +		if (dport)

3. At which point this is using dport uninitialized.

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