Re: [PATCH V3 13/20] cxl/mem: Refactor cxl pmem region auto-assembling

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

On 11/7/25 5:39 AM, Neeraj Kumar wrote:
> On 06/10/25 08:55AM, Dave Jiang wrote:
>>
>>
>> On 9/29/25 6:30 AM, Neeraj Kumar wrote:
>>> On 23/09/25 03:37PM, Dave Jiang wrote:
>>>>
>>>>
>>>> On 9/17/25 6:41 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. Moved cxl pmem region auto-assembly from cxl_endpoint_port_probe() to
>>>>>    cxl_mem_probe() after devm_cxl_add_nvdimm(). It gurantees both the
>>>>>    completion of endpoint probe and cxl_nvd presence before its call.
>>>>
>>>> Given that we are going forward with this implementation [1] from Dan and drivers like the type2 enabling are going to be using it as well, can you please consider converting this change to Dan's mechanism instead of creating a whole new one?
>>>>
>>>> I think the region discovery can be done via the ops->probe() callback. Thanks.
>>>>
>>>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=for-6.18/cxl-probe-order&id=88aec5ea7a24da00dc92c7778df4851fe4fd3ec6
>>>>
>>>> DJ
>>>>
>>>
>>> Sure, Let me revisit this.
>>> It seems [1] is there in seperate branch "for-6.18/cxl-probe-order", and not yet merged into next, right?
>>
>> Right. I believe Smita and Alejandro are using that as well. Depending on who gets there first. We can setup an immutable branch at some point.
>>
>> [1]: https://lore.kernel.org/linux-cxl/20250822034202.26896-1-Smita.KoralahalliChannabasappa@amd.com/T/#t
>>
>> DJ
> 
> Hi Dave,
> 
> As per Dan's [1] newly introduced infra, Following is my understanding.
> 
> Currently cxl_pci does not care about the attach state of the cxl_memdev
> because all generic memory expansion functionality can be handled by the
> cxl_core. For accelerators, the driver needs to know and perform driver
> specific initialization if CXL is available, or exectute a fallback to PCIe
> only operation.
> 
> Dan's new infra is needed for CXL accelerator device drivers that need to
> make decisions about enabling CXL dependent functionality in the device, or
> falling back to PCIe-only operation.
> 
> During cxl_pci_probe() we call devm_cxl_add_memdev(struct cxl_memdev_ops *ops)
> where function pointer as ops gets registered which gets called in cxl_mem_probe()
> using cxlmd->ops->probe()
> 
> The probe callback runs after the port topology is successfully attached for
> the given memdev.
> 
> So to use this infra we have to pass cxl_region_discovery() as ops parameter
> of devm_cxl_add_memdev() getting called from cxl_pci_probe().
>  
> In this patch-set cxl_region_discovery() signature is different from cxlmd->ops->probe()
> 
>    {{{
>     void cxl_region_discovery(struct cxl_port *port)
>     {
>             device_for_each_child(&port->dev, NULL, discover_region);
>     }
> 
>     struct cxl_memdev_ops {
>             int (*probe)(struct cxl_memdev *cxlmd);
>     };
>    }}}
> 
> Even after changing the signature of cxl_region_discovery() as per cxlmd->ops->probe()
> may create problem as when the ops->probe() fails, then it will halts the probe sequence
> of cxl_pci_probe()
> 
> It is because discover_region() may fail if two memdevs are participating into one region

While discover_region() may fail, the return value is ignored. The current code disregards failures from device_for_each_child(). And also above, cxl_region_discovery() returns void. So I don't follow how ops->probe() would fail if we ignore errors from discover_region().

DJ

> 
> Also, region auto assembly is mandatory functionality which creates region
> if (cxled->state == CXL_DECODER_STATE_AUTO) gets satisfied.
> 
> Currently region auto assembly (added by a32320b71f085) happens after successfull
> enumeration of endpoint decoders at cxl_endpoint_port_probe(), which I have moved at
> cxl_mem_probe() after devm_cxl_add_nvdimm() which prepares cxl_nvd infra required by it.
> 
> As discussed in [1], this patch-set does the movement of auto region assembly from
> cxl_endpoint_port_probe() to cxl_mem_probe() and resolved the conflicting dependency
> of cxl_nvd infra required by both region creation using LSA and auto region assembly.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=for-6.18/cxl-probe-order&id=88aec5ea7a24da00dc92c7778df4851fe4fd3ec6 [2]: https://lore.kernel.org/linux-cxl/1931444790.41752909482841.JavaMail.epsvc@epcpadp2new/
> 
> Please let me know if my understanding is correct or I am missing something?
> 
> 
> Regards,
> Neeraj
> 
>