Re: [PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

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

On 4/22/25 10:05 AM, Jonathan Cameron wrote:
> On Fri, 4 Apr 2025 15:57:34 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Current implementation only enuemrates the dports dupring 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.
> A few additional comments from me.
> 
> 
> Can we point at anything in the PCI spec to say whether those IDs not being
> valid is allowed or not?  I always like to know whether to grump at hardware
> folk or not if it they manage to duplicate something unexpected :)

Nothing I can find. The thinking was stemmed from this series [1] by Robert.

[1]: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/

Mainly the intention of the current series is to enumerate ports triggered by memdev probe(). At that point we know there's an endpoint device and the link is established from EP to RP.

> 
> 
>>
>> 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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/core/core.h |  4 ++
>>  drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
>>  drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
>>  drivers/cxl/cxl.h       |  1 +
>>  drivers/cxl/port.c      |  2 -
>>  5 files changed, 114 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 15699299dc11..e2822ead6a67 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -134,4 +134,8 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>>  		    u16 *return_code);
>>  #endif
>>  
>> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb);
>> +void cxl_port_probe_dports(struct cxl_port *port);
>> +
>>  #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 96fecb799cbc..a47dd032abd7 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -24,6 +24,66 @@ 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");
>>  
>> +static int probe_dports(struct cxl_dport *dport)
> 
> Given return value is always ignored, maybe don't return anything?

ok

> 
>> +{
>> +	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;
>> +
>> +	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_id != CXL_PORT_ID_INVALID)
>> +		return 0;
>> +
>> +	pdev = to_pci_dev(dport_dev);
>> +	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
>> +				  &lnkcap))
>> +		return 0;
> 
> I'd be tempted to return an error that means come back later and eat it
> at next level up if possible (though currently you eat all errors so
> that is fine ;)
> >> +
>> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
>> +	if (rc) {
>> +		dev_dbg(&port->dev, "failed to find component registers\n");
>> +		return 0;
> 
> Likewise. Something called probe_dports() to me should be returning an erorr
> if this happens and caller be responsible for ignoring that or not rather
> than errors being eaten down here.
> 
>> +	}
>> +
>> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>> +	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/*
>> +	 * port_id is only set if the register block is also probed
>> +	 * successfully.
>> +	 */
>> +	dport->port_id = port_num;
>> +	cxl_switch_parse_cdat(port);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_port_probe_dports - probe downstream ports of the upstream port
>> + * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
>> + *
> 
> Bonus blank line doesn't add anything.

will remove

> 
>> + */
>> +void cxl_port_probe_dports(struct cxl_port *port)
>> +{
>> +	struct cxl_dport *dport;
>> +	unsigned long index;
>> +
>> +	device_lock_assert(&port->dev);
>> +	xa_for_each(&port->dports, index, dport)
>> +		probe_dports(dport);
>> +}
> 
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index e90e55bc11ac..1c772c516dbe 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
> 
>> @@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
>>  	sysfs_remove_link(&port->dev.kobj, link_name);
>>  }
>>  
>> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb)
>> +{
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct cxl_port *port = dport->port;
>> +	int rc;
>> +
>> +	if (rcrb == CXL_RESOURCE_NONE) {
>> +		rc = cxl_dport_setup_regs(&port->dev, dport,
>> +					  component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +	} else {
>> +		dport->rcrb.base = rcrb;
>> +		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> +							 CXL_RCRB_DOWNSTREAM);
>> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> +			return -ENXIO;
>> +		}
>> +
>> +		/*
>> +		 * RCH @dport is not ready to map until associated with its
>> +		 * memdev
>> +		 */
>> +		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +
>> +		dport->rch = true;
>> +	}
>> +
>> +	if (component_reg_phys != CXL_RESOURCE_NONE)
>> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
>> +			&component_reg_phys);
>> +
>> +	return 0;
>> +}
>> +
>>  static struct cxl_dport *
>>  __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  		     int port_id, resource_size_t component_reg_phys,
>> @@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	dport->port = port;
>>  	dport->id = id;
>>
> 
> Maybe do this factoring out as a precursor patch?

ok will do
> 
>> -	if (rcrb == CXL_RESOURCE_NONE) {
>> -		rc = cxl_dport_setup_regs(&port->dev, dport,
>> -					  component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -	} else {
>> -		dport->rcrb.base = rcrb;
>> -		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> -							 CXL_RCRB_DOWNSTREAM);
>> -		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> -			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(-ENXIO);
>> -		}
>> -
>> -		/*
>> -		 * RCH @dport is not ready to map until associated with its
>> -		 * memdev
>> -		 */
>> -		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -
>> -		dport->rch = true;
>> +	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
>> +	if (rc) {
>> +		ida_free(&port->dport_ida, id);
>> +		return ERR_PTR(rc);
>>  	}
>>