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

[Gregory Price <gourry@gourry.net>]

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?

~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);


~Gregory