Re: [PATCH V6 10/18] cxl/mem: Refactor cxl pmem region auto-assembling

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

On 1/26/26 3:48 PM, Dave Jiang wrote:
> 
> 
> On 1/23/26 4:31 AM, Neeraj Kumar wrote:
>> In 84ec985944ef3, devm_cxl_add_nvdimm() sequence was changed and called
>> before devm_cxl_add_endpoint(). It's because cxl pmem region auto-assembly
>> used to get called at last in cxl_endpoint_port_probe(), which requires
>> cxl_nvd presence.
>>
>> For cxl region persistency, region creation happens during nvdimm_probe
>> which need the completion of endpoint probe.
>>
>> In order to accommodate both cxl pmem region auto-assembly and cxl region
>> persistency, refactored following
>>
>> 1. Re-Sequence devm_cxl_add_nvdimm() after devm_cxl_add_endpoint(). This
>>    will be called only after successful completion of endpoint probe.
>>
>> 2. Create cxl_region_discovery() which performs pmem region
>>    auto-assembly and remove cxl pmem region auto-assembly from
>>    cxl_endpoint_port_probe()
>>
>> 3. Register cxl_region_discovery() with devm_cxl_add_memdev() which gets
>>    called during cxl_pci_probe() in context of cxl_mem_probe()
>>
>> 4. As cxlmd->attach->probe() calls registered cxl_region_discovery(), so
>>    move devm_cxl_add_nvdimm() before cxlmd->attach->probe(). It guarantees
>>    both the completion of endpoint probe and cxl_nvd presence before
>>    calling cxlmd->attach->probe().
>>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
> 
> Hi Neeraj,
> Just FYI this is the patch that breaks the auto-region assemble in cxl_test.

So it's missing this part in cxl_test. cxl_test auto region is now showing up. But now I'm hitting some lockdep issues. 

---
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index cb87e8c0e63c..03af15edd988 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -1686,6 +1686,7 @@ static void cxl_mock_test_feat_init(struct cxl_mockmem_data *mdata)

 static int cxl_mock_mem_probe(struct platform_device *pdev)
 {
+       struct cxl_memdev_attach memdev_attach = { 0 };
        struct device *dev = &pdev->dev;
        struct cxl_memdev *cxlmd;
        struct cxl_memdev_state *mds;
@@ -1767,7 +1768,8 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)

        cxl_mock_add_event_logs(&mdata->mes);

-       cxlmd = devm_cxl_add_memdev(cxlds, NULL);
+       memdev_attach.probe = cxl_region_discovery;
+       cxlmd = devm_cxl_add_memdev(cxlds, &memdev_attach);
        if (IS_ERR(cxlmd))
                return PTR_ERR(cxlmd);

---


> 
> DJ
> 
>> ---
>>  drivers/cxl/core/region.c | 37 +++++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxl.h         |  5 +++++
>>  drivers/cxl/mem.c         | 18 +++++++++---------
>>  drivers/cxl/pci.c         |  4 +++-
>>  drivers/cxl/port.c        | 39 +--------------------------------------
>>  5 files changed, 55 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index ae899f68551f..26238fb5e8cf 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3727,6 +3727,43 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
>>  
>> +static int discover_region(struct device *dev, void *unused)
>> +{
>> +	struct cxl_endpoint_decoder *cxled;
>> +	int rc;
>> +
>> +	if (!is_endpoint_decoder(dev))
>> +		return 0;
>> +
>> +	cxled = to_cxl_endpoint_decoder(dev);
>> +	if ((cxled->cxld.flags & CXL_DECODER_F_ENABLE) == 0)
>> +		return 0;
>> +
>> +	if (cxled->state != CXL_DECODER_STATE_AUTO)
>> +		return 0;
>> +
>> +	/*
>> +	 * Region enumeration is opportunistic, if this add-event fails,
>> +	 * continue to the next endpoint decoder.
>> +	 */
>> +	rc = cxl_add_to_region(cxled);
>> +	if (rc)
>> +		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
>> +			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
>> +
>> +	return 0;
>> +}
>> +
>> +int cxl_region_discovery(struct cxl_memdev *cxlmd)
>> +{
>> +	struct cxl_port *port = cxlmd->endpoint;
>> +
>> +	device_for_each_child(&port->dev, NULL, discover_region);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_region_discovery, "CXL");
>> +
>>  u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa)
>>  {
>>  	struct cxl_region_ref *iter;
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index c796c3db36e0..86efcc4fb963 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -906,6 +906,7 @@ struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev);
>>  int cxl_add_to_region(struct cxl_endpoint_decoder *cxled);
>>  struct cxl_dax_region *to_cxl_dax_region(struct device *dev);
>>  u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa);
>> +int cxl_region_discovery(struct cxl_memdev *cxlmd);
>>  #else
>>  static inline bool is_cxl_pmem_region(struct device *dev)
>>  {
>> @@ -928,6 +929,10 @@ static inline u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint,
>>  {
>>  	return 0;
>>  }
>> +static inline int cxl_region_discovery(struct cxl_memdev *cxlmd)
>> +{
>> +	return 0;
>> +}
>>  #endif
>>  
>>  void cxl_endpoint_parse_cdat(struct cxl_port *port);
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 333c366b69e7..7d19528d9b55 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -114,15 +114,6 @@ static int cxl_mem_probe(struct device *dev)
>>  		return -ENXIO;
>>  	}
>>  
>> -	if (cxl_pmem_size(cxlds) && IS_ENABLED(CONFIG_CXL_PMEM)) {
>> -		rc = devm_cxl_add_nvdimm(parent_port, cxlmd);
>> -		if (rc) {
>> -			if (rc == -ENODEV)
>> -				dev_info(dev, "PMEM disabled by platform\n");
>> -			return rc;
>> -		}
>> -	}
>> -
>>  	if (dport->rch)
>>  		endpoint_parent = parent_port->uport_dev;
>>  	else
>> @@ -142,6 +133,15 @@ static int cxl_mem_probe(struct device *dev)
>>  			return rc;
>>  	}
>>  
>> +	if (cxl_pmem_size(cxlds) && IS_ENABLED(CONFIG_CXL_PMEM)) {
>> +		rc = devm_cxl_add_nvdimm(parent_port, cxlmd);
>> +		if (rc) {
>> +			if (rc == -ENODEV)
>> +				dev_info(dev, "PMEM disabled by platform\n");
>> +			return rc;
>> +		}
>> +	}
>> +
>>  	if (cxlmd->attach) {
>>  		rc = cxlmd->attach->probe(cxlmd);
>>  		if (rc)
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 549368a9c868..70b40a10be7a 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -903,6 +903,7 @@ __ATTRIBUTE_GROUPS(cxl_rcd);
>>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>  {
>>  	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
>> +	struct cxl_memdev_attach memdev_attach = { 0 };
>>  	struct cxl_dpa_info range_info = { 0 };
>>  	struct cxl_memdev_state *mds;
>>  	struct cxl_dev_state *cxlds;
>> @@ -1006,7 +1007,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>  	if (rc)
>>  		dev_dbg(&pdev->dev, "No CXL Features discovered\n");
>>  
>> -	cxlmd = devm_cxl_add_memdev(cxlds, NULL);
>> +	memdev_attach.probe = cxl_region_discovery;
>> +	cxlmd = devm_cxl_add_memdev(cxlds, &memdev_attach);
>>  	if (IS_ERR(cxlmd))
>>  		return PTR_ERR(cxlmd);
>>  
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index 7937e7e53797..fbeff1978bfb 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -30,33 +30,6 @@ static void schedule_detach(void *cxlmd)
>>  	schedule_cxl_memdev_detach(cxlmd);
>>  }
>>  
>> -static int discover_region(struct device *dev, void *unused)
>> -{
>> -	struct cxl_endpoint_decoder *cxled;
>> -	int rc;
>> -
>> -	if (!is_endpoint_decoder(dev))
>> -		return 0;
>> -
>> -	cxled = to_cxl_endpoint_decoder(dev);
>> -	if ((cxled->cxld.flags & CXL_DECODER_F_ENABLE) == 0)
>> -		return 0;
>> -
>> -	if (cxled->state != CXL_DECODER_STATE_AUTO)
>> -		return 0;
>> -
>> -	/*
>> -	 * Region enumeration is opportunistic, if this add-event fails,
>> -	 * continue to the next endpoint decoder.
>> -	 */
>> -	rc = cxl_add_to_region(cxled);
>> -	if (rc)
>> -		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
>> -			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
>> -
>> -	return 0;
>> -}
>> -
>>  static int cxl_switch_port_probe(struct cxl_port *port)
>>  {
>>  	/* Reset nr_dports for rebind of driver */
>> @@ -82,17 +55,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>  	if (rc)
>>  		return rc;
>>  
>> -	rc = devm_cxl_endpoint_decoders_setup(port);
>> -	if (rc)
>> -		return rc;
>> -
>> -	/*
>> -	 * Now that all endpoint decoders are successfully enumerated, try to
>> -	 * assemble regions from committed decoders
>> -	 */
>> -	device_for_each_child(&port->dev, NULL, discover_region);
>> -
>> -	return 0;
>> +	return devm_cxl_endpoint_decoders_setup(port);
>>  }
>>  
>>  static int cxl_port_probe(struct device *dev)
> 
>