Re: [PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

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

On Tue, 6 May 2025 17:43:05 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Current implementation only enumerates the dports during the port 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 hw
> dport id assignment and the register probing to behind memdev probe so the
> endpoint is guaranteed to be connected.
> 
> The detection of duplicate dport for add_dport() is removed. The port_id
> is not read from the hw at this point any longer. The port->id will always
> be unique since it's retrieved from an ida. The dup detection thus become
> irrelevant.
> 
> The decoders must also be updated since previously it's all done when all
> 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.
> 
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
A few trivial things inline.

> ---
> v2:
> - Fix spelling error in commit message. (Ming)
> - Move probing to the specific dport. (Ming)
> - Add decoder update.
> - Return errors when setup dports. (Jonathan)
> - Add driver check before setup dport. (Dan)
> - Add comment on why location to do dport update. (Dan)
> ---
>  drivers/cxl/core/core.h |   4 ++
>  drivers/cxl/core/pci.c  |  60 ++++++++++++----
>  drivers/cxl/core/port.c | 152 +++++++++++++++++++++++++++++++++++-----
>  drivers/cxl/cxl.h       |   4 ++
>  drivers/cxl/port.c      |  20 +-----
>  5 files changed, 192 insertions(+), 48 deletions(-)
> 

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 3b84b43ab194..16bd842904e5 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,52 @@ static unsigned short media_ready_timeout = 60;
>  module_param(media_ready_timeout, ushort, 0644);
>  MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");
>  
> +int cxl_dport_setup(struct cxl_dport *dport)
> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	struct cxl_register_map map;
> +	struct pci_dev *pdev;
> +	u32 lnkcap, port_num;
> +	int rc;
> +

I assume this is a cxl test thing?  If so maybe throw a comment in
as I at least sometimes forget that exists and wonder how on earth
we got here with something that wasn't a pci device.


> +	if (!dev_is_pci(dport_dev))
> +		return 0;
> +
> +	/*
> +	 * dport->port_id is valid means that dport has been probed and is
> +	 * setup.
> +	 */
> +	if (dport->port_num != CXL_DPORT_NUM_INVALID)
> +		return 0;
> +
> +	pdev = to_pci_dev(dport_dev);
> +	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> +				  &lnkcap))
> +		return -ENODEV;
> +
> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (rc) {
> +		dev_dbg(&port->dev, "failed to find component registers\n");
> +		return -ENODEV;
> +	}
> +
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);

I'd grab this where you read link cap above, or just before use below.
Here it's not associated with anything in particular so just feels
odd. I'd guess this is down to some code reorg leaving it somewhat
stranded (or a change in a later patch that makes this location an obviou
choice?)


> +	rc = cxl_dport_setup_regs(&port->dev, dport, map.resource);
> +	if (rc)
> +		return rc;
> +
> +	if (map.resource != CXL_RESOURCE_NONE) {
> +		dev_dbg(dport->dport_dev,
> +			"Component Register found for dport: %pa\n",
> +			&map.resource);
> +	}
> +
> +	dport->port_num = port_num;
> +
> +	return 0;
> +}

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 70173d23139c..04e18a102d26 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c


> +static int port_probe_dport(struct cxl_port *port, struct device *dport_dev,
> +			    struct cxl_dport **probed_dport)
> +{
> +	struct cxl_dport *dport;
> +	unsigned long index;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +	xa_for_each(&port->dports, index, dport) {
> +		if (dport->dport_dev == dport_dev) {

Maybe flip?  The code indent isn't huge though so up to you.

		if (!dport->dport_dev != dport_dev)
			continue;

> +			rc = cxl_dport_setup(dport);
> +			if (rc)
> +				return rc;

I'd throw in a blank line here.

> +			*probed_dport = dport;

and here in the interests of minor readability improvement

> +			return 0;
> +		}
> +	}
> +
> +	*probed_dport = NULL;
> +
> +	return -ENODEV;
> +}


> +
> +static int cxl_switch_port_dport_setup(struct cxl_port *port,
> +				       struct device *dport_dev)
> +{
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +
> +	if (!port->dev.driver)
> +		return -ENODEV;
> +
> +	rc = port_probe_dport(port, dport_dev, &dport);
> +	if (rc)
> +		return rc;
> +
> +	cxl_switch_parse_cdat(port);
> +
> +	/* Make sure that no decoders have been allocated before proceeding. */
> +	if (ida_is_empty(&port->decoder_ida))
> +		rc = devm_cxl_port_setup_decoders(port);
> +	else
> +		rc = update_decoders_with_dport(port, dport);
> +	if (rc)
> +		return rc;
> +
> +	return 0;

return rc?  or just return in each branch perhaps.

> +}
> +