Re: [PATCH 4/7] cxl/test: Add hierarchy enumeration support for type2 device

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

On 5/5/26 10:05 PM, Alison Schofield wrote:
> On Wed, Apr 22, 2026 at 04:02:34PM -0700, Dave Jiang wrote:
>> Add enumeration of type2 device hierarchy in cxl-test. The type2 device
>> is setup to be directly attached to a root port instead of rp -> switch
>> -> device that type3 hierarchy is setup..
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  tools/testing/cxl/test/cxl.c | 110 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 103 insertions(+), 7 deletions(-)
>>
> 
> skip
> 
> 
>> +static int cxl_type2_mem_init(void)
>> +{
>> +	int i, rc;
>> +
>> +	for (i = 0; i < NR_CXL_ROOT_PORTS; i++) {
> 
> I'd like to drop the artifact we carry here after copying type3 setup.
> Type 2 doesn't use the second pdev. I think we can make the one accel
> one region constraint explicit.
> 
> How about:
> 
> 	#define NR_CXL_TYPE2_ACCEL 1  /* One Type2 dev, One Region */
> 
> and then in cxl_type2_mem_init():
> 
> replace this:
> 	for (i = 0; i < NR_CXL_ROOT_PORTS; i++) {
> with this:
> 	for (i = 0; i < NR_CXL_TYPE2_ACCEL; i++) {
> 

ok

> 
> 
>> +		struct platform_device *dport = cxl_root_port[i];
>> +		struct platform_device *pdev;
>> +
>> +		pdev = platform_device_alloc("cxl_type2_accel", i);
>> +		if (!pdev)
>> +			goto err_mem;
>> +		pdev->dev.parent = &dport->dev;
>> +		set_dev_node(&pdev->dev, i % 2);
>> +
>> +		rc = platform_device_add(pdev);
>> +		if (rc) {
>> +			platform_device_put(pdev);
>> +			goto err_mem;
>> +		}
>> +		cxl_mem[i] = pdev;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_mem:
>> +	for (i = NR_CXL_ROOT_PORTS - 1; i >= 0; i--) {
>> +		struct platform_device *pdev = cxl_mem[i];
>> +
>> +		if (!pdev)
>> +			break;
>>  		platform_device_unregister(cxl_mem[i]);
>> +	}
>> +	return rc;
>>  }
> 
> snip
> 
>>  
>> +static int cxl_mem_init(void)
>> +{
>> +	if (type2_test)
>> +		return cxl_type2_mem_init();
>> +	return cxl_type3_mem_init();
>> +}
> 
> The precedent above would be good to follow for the topology helpers.
> Now we have scattered 'if (type2_test) return 0;' at 7 sites that need
> to know which topology this is. How about:
> 
> static int cxl_type3_topo_init(void)
> {
> 	/* inits all the things */
> }
> 
> static int cxl_type2_topo_init(void)
> {
> 	return cxl_rootports_populate();
> }
> 
> static int cxl_topo_init(void)
> {
> 	if (type2_test)
> 		return cxl_type2_topo_init();
> 	return cxl_type3_topo_init();
> }
> 
> static void cxl_topo_exit(void)
> {
> 	if (type2_test) {
> 		cxl_rootports_remove();
> 	return;
> 	}
> 
> 	cxl_rch_topo_exit();
> 	cxl_single_topo_exit();
> 	cxl_switches_remove();
> 	cxl_rootports_remove();
> }

ok

> 
>> +
>>  static ssize_t
>>  decoder_reset_preserve_registry_show(struct device *dev,
>>  				     struct device_attribute *attr, char *buf)
>> @@ -2008,6 +2098,9 @@ static int cxl_dsps_populate(void)
>>  
>>  static void cxl_switches_remove(void)
>>  {
>> +	if (type2_test)
>> +		return;
>> +
>>  	cxl_usps_remove();
>>  	cxl_dsps_remove();
>>  }
>> @@ -2016,6 +2109,9 @@ static int cxl_switches_populate(void)
>>  {
>>  	int rc;
>>  
>> +	if (type2_test)
>> +		return 0;
>> +
>>  	BUILD_BUG_ON(ARRAY_SIZE(cxl_switch_uport) != ARRAY_SIZE(cxl_root_port));
>>  	rc = cxl_usps_populate();
>>  	if (rc)
>> -- 
>> 2.53.0
>>