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

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

On 5/13/25 8:43 AM, Gregory Price wrote:
> On Tue, May 06, 2025 at 05:43:05PM -0700, Dave Jiang 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.
>>
> 
> I'm finding the changes a little difficult to follow, can you lay out
> the expected order of operations before and after?
> 
> Specifically there's two new guard() calls, can you explain under what
> conditions those can be contended?

I'll add more to the commit log and explain.
> 
> ~Gregory
> 
>> 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
> ... snip ...
>> +static int update_switch_decoder(struct device *dev, void *data)
>> +{
>> +	struct cxl_dport *dport = data;
>> +	struct cxl_switch_decoder *cxlsd;
>> +	struct cxl_decoder *cxld;
>> +	int i;
>> +
>> +	if (!is_switch_decoder(dev))
>> +		return 0;
>> +
>> +	cxlsd = to_cxl_switch_decoder(dev);
>> +	cxld = &cxlsd->cxld;
>> +	guard(rwsem_write)(&cxl_region_rwsem);
>> +	for (i = 0; i < cxld->interleave_ways; i++) {
>> +		if (cxlsd->target_map[i] == dport->port_num) {
>> +			cxlsd->target[i] = dport;
>> +			return 0;
>> +		}
>> +	}
> ... snip ...
>> @@ -1695,6 +1798,19 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  				"found already registered port %s:%s\n",
>>  				dev_name(&port->dev),
>>  				dev_name(port->uport_dev));
>> +
>> +			/*
>> +			 * Attempt to do single pass dport setup by checking here
>> +			 * instead of doing it during port creation. Otherwise
>> +			 * it still needs to check here for dports that are
>> +			 * being probed with a port already created.
>> +			 */
>> +			scoped_guard(device, &port->dev) {
>> +				rc = cxl_switch_port_dport_setup(port, dport_dev);
>> +				if (rc)
>> +					return rc;
>> +			}
>> +
> ... snip ...
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index a35fc5552845..4d840a6ef802 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
> ... snip ...
>>  	/* Cache the data early to ensure is_visible() works */
>> @@ -69,24 +68,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  	if (rc < 0)
>>  		return rc;
>>  
> ... snip ...
>> -	return -ENXIO;
>> +	return 0;
>>  }
> 
> 	return devm_cxl_port_enumerate_dports(port);

This was actually done on purpose. devm_cxl_port_enumerate_dports() returns the number of dports enumerated. So usually the return value is greater than 0. in drivers/base/dd.c, call_driver_probe() throws the return value into a switch() where any value not 0 are errors. So the probe() call would fail. Here we are intercepting the return value and return a 0 if it's positive. I got bitten here during this series's debug. I should add a comment and explain why.  


> 
> 
> ~Gregory