drivers/cxl/Kconfig | 2 +- drivers/cxl/cxl.h | 2 + drivers/cxl/cxlmem.h | 17 ++++-- drivers/cxl/core/edac.c | 64 ++++++++++++--------- drivers/cxl/core/memdev.c | 104 ++++++++++++++++++++++++----------- drivers/cxl/mem.c | 69 +++++++++-------------- drivers/cxl/pci.c | 2 +- drivers/cxl/port.c | 40 ++++++++++++++ tools/testing/cxl/test/mem.c | 2 +- 9 files changed, 192 insertions(+), 110 deletions(-)
The CXL subsystem is modular. That modularity is a benefit for separation of concerns and testing. It is generally appropriate for this class of devices that support hotplug and can dynamically add a CXL personality alongside their PCI personality. However, a cost of modules is ambiguity about when devices (cxl_memdevs, cxl_ports, cxl_regions) have had a chance to attach to their corresponding drivers on @cxl_bus_type. This problem of not being able to reliably determine when a device has had a chance to attach to its driver vs still waiting for the module to load, is a common problem for the "Soft Reserve Recovery" [1], and "Accelerator Memory" [2] enabling efforts. For "Soft Reserve Recovery" it wants to use wait_for_device_probe() as a sync point for when CXL devices present at boot have had a chance to attach to the cxl_pci driver (generic CXL memory expansion class driver). That breaks down if wait_for_device_probe() only flushes PCI device probe, but not the cxl_mem_probe() of the cxl_memdev that cxl_pci_probe() creates. For "Accelerator Memory", the driver is not cxl_pci, but any potential PCI driver that wants to use the devm_cxl_add_memdev() ABI to attach to the CXL memory domain. Those drivers want to know if the CXL link is live end-to-end (from endpoint, through switches, to the host bridge) and CXL memory operations are enabled. If not, a CXL accelerator may be able to fall back to PCI-only operation. Similar to the "Soft Reserve Memory" it needs to know that the CXL subsystem had a chance to probe the ancestor topology of the device and let that driver make a synchronous decision about CXL operation. In support of those efforts: * Clean up some resource lifetime issues in the current code * Move some object creation symbols (devm_cxl_add_memdev() and devm_cxl_add_endpoint()) into the cxl_mem.ko and cxl_port.ko objects. Implicitly guarantee that cxl_mem_driver and cxl_port_driver have been registered prior to any device objects being registered. This is preferred over explicit open-coded request_module(). * Use scoped-based-cleanup before adding more resource management in devm_cxl_add_memdev() * Give an accelerator the opportunity to run setup operations in cxl_mem_probe() so it can further probe if the CXL configuration matches its needs. Some of these previously appeared on a branch as an RFC [3] and left "Soft Reserve Recovery" and "Accelerator Memory" to jockey for ordering. Instead, create a shared topic branch for both of those efforts to import. The main changes since that RFC are fixing a bug and reducing the amount of refactoring (which contributed to hiding the bug). [1]: http://lore.kernel.org/20251120031925.87762-1-Smita.KoralahalliChannabasappa@amd.com [2]: http://lore.kernel.org/20251119192236.2527305-1-alejandro.lucero-palau@amd.com [3]: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-6.18/cxl-probe-order Dan Williams (6): cxl/mem: Fix devm_cxl_memdev_edac_release() confusion cxl/mem: Arrange for always-synchronous memdev attach cxl/port: Arrange for always synchronous endpoint attach cxl/mem: Convert devm_cxl_add_memdev() to scope-based-cleanup cxl/mem: Drop @host argument to devm_cxl_add_memdev() cxl/mem: Introduce a memdev creation ->probe() operation drivers/cxl/Kconfig | 2 +- drivers/cxl/cxl.h | 2 + drivers/cxl/cxlmem.h | 17 ++++-- drivers/cxl/core/edac.c | 64 ++++++++++++--------- drivers/cxl/core/memdev.c | 104 ++++++++++++++++++++++++----------- drivers/cxl/mem.c | 69 +++++++++-------------- drivers/cxl/pci.c | 2 +- drivers/cxl/port.c | 40 ++++++++++++++ tools/testing/cxl/test/mem.c | 2 +- 9 files changed, 192 insertions(+), 110 deletions(-) base-commit: ea5514e300568cbe8f19431c3e424d4791db8291 -- 2.51.1
On 12/4/25 02:21, Dan Williams wrote: > The CXL subsystem is modular. That modularity is a benefit for > separation of concerns and testing. It is generally appropriate for this > class of devices that support hotplug and can dynamically add a CXL > personality alongside their PCI personality. However, a cost of modules > is ambiguity about when devices (cxl_memdevs, cxl_ports, cxl_regions) > have had a chance to attach to their corresponding drivers on > @cxl_bus_type. > > This problem of not being able to reliably determine when a device has > had a chance to attach to its driver vs still waiting for the module to > load, is a common problem for the "Soft Reserve Recovery" [1], and > "Accelerator Memory" [2] enabling efforts. > > For "Soft Reserve Recovery" it wants to use wait_for_device_probe() as a > sync point for when CXL devices present at boot have had a chance to > attach to the cxl_pci driver (generic CXL memory expansion class > driver). That breaks down if wait_for_device_probe() only flushes PCI > device probe, but not the cxl_mem_probe() of the cxl_memdev that > cxl_pci_probe() creates. > > For "Accelerator Memory", the driver is not cxl_pci, but any potential > PCI driver that wants to use the devm_cxl_add_memdev() ABI to attach to > the CXL memory domain. Those drivers want to know if the CXL link is > live end-to-end (from endpoint, through switches, to the host bridge) > and CXL memory operations are enabled. If not, a CXL accelerator may be > able to fall back to PCI-only operation. Similar to the "Soft Reserve > Memory" it needs to know that the CXL subsystem had a chance to probe > the ancestor topology of the device and let that driver make a > synchronous decision about CXL operation. > > In support of those efforts: > > * Clean up some resource lifetime issues in the current code > * Move some object creation symbols (devm_cxl_add_memdev() and > devm_cxl_add_endpoint()) into the cxl_mem.ko and cxl_port.ko objects. > Implicitly guarantee that cxl_mem_driver and cxl_port_driver have been > registered prior to any device objects being registered. This is > preferred over explicit open-coded request_module(). > * Use scoped-based-cleanup before adding more resource management in > devm_cxl_add_memdev() > * Give an accelerator the opportunity to run setup operations in > cxl_mem_probe() so it can further probe if the CXL configuration matches > its needs. > > Some of these previously appeared on a branch as an RFC [3] and left > "Soft Reserve Recovery" and "Accelerator Memory" to jockey for ordering. > Instead, create a shared topic branch for both of those efforts to > import. The main changes since that RFC are fixing a bug and reducing > the amount of refactoring (which contributed to hiding the bug). > > [1]: http://lore.kernel.org/20251120031925.87762-1-Smita.KoralahalliChannabasappa@amd.com > [2]: http://lore.kernel.org/20251119192236.2527305-1-alejandro.lucero-palau@amd.com > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-6.18/cxl-probe-order > > Dan Williams (6): > cxl/mem: Fix devm_cxl_memdev_edac_release() confusion > cxl/mem: Arrange for always-synchronous memdev attach > cxl/port: Arrange for always synchronous endpoint attach > cxl/mem: Convert devm_cxl_add_memdev() to scope-based-cleanup > cxl/mem: Drop @host argument to devm_cxl_add_memdev() > cxl/mem: Introduce a memdev creation ->probe() operation > > drivers/cxl/Kconfig | 2 +- > drivers/cxl/cxl.h | 2 + > drivers/cxl/cxlmem.h | 17 ++++-- > drivers/cxl/core/edac.c | 64 ++++++++++++--------- > drivers/cxl/core/memdev.c | 104 ++++++++++++++++++++++++----------- > drivers/cxl/mem.c | 69 +++++++++-------------- > drivers/cxl/pci.c | 2 +- > drivers/cxl/port.c | 40 ++++++++++++++ > tools/testing/cxl/test/mem.c | 2 +- > 9 files changed, 192 insertions(+), 110 deletions(-) > > > base-commit: ea5514e300568cbe8f19431c3e424d4791db8291 I have tested this series with Type2 and it works as expected: 1) type2 fails to be installed if cxl_mem not loaded. 2) cxl_mem, cxl_port and cxl_core not removable as type2 driver installed depends on them. 3) cxl_acpi is still possible to remove ... FWIW: Tested-by: Alejandro Lucero <alucerop@amd.com>
Alejandro Lucero Palau wrote: [..] > > Dan Williams (6): > > cxl/mem: Fix devm_cxl_memdev_edac_release() confusion > > cxl/mem: Arrange for always-synchronous memdev attach > > cxl/port: Arrange for always synchronous endpoint attach > > cxl/mem: Convert devm_cxl_add_memdev() to scope-based-cleanup > > cxl/mem: Drop @host argument to devm_cxl_add_memdev() > > cxl/mem: Introduce a memdev creation ->probe() operation > > > > drivers/cxl/Kconfig | 2 +- > > drivers/cxl/cxl.h | 2 + > > drivers/cxl/cxlmem.h | 17 ++++-- > > drivers/cxl/core/edac.c | 64 ++++++++++++--------- > > drivers/cxl/core/memdev.c | 104 ++++++++++++++++++++++++----------- > > drivers/cxl/mem.c | 69 +++++++++-------------- > > drivers/cxl/pci.c | 2 +- > > drivers/cxl/port.c | 40 ++++++++++++++ > > tools/testing/cxl/test/mem.c | 2 +- > > 9 files changed, 192 insertions(+), 110 deletions(-) > > > > > > base-commit: ea5514e300568cbe8f19431c3e424d4791db8291 > > > I have tested this series with Type2 and it works as expected: > > > 1) type2 fails to be installed if cxl_mem not loaded. > > 2) cxl_mem, cxl_port and cxl_core not removable as type2 driver > installed depends on them. > > 3) cxl_acpi is still possible to remove ... > > > FWIW: > > > Tested-by: Alejandro Lucero <alucerop@amd.com> Thanks. Do note that module removability does not affect the unbind path. So you can still do all the same "remove" violence via: "echo $device > /sys/bus/cxl/driver/$driver/unbind" ...as would be caused by: "modprobe -r $driver" ...i.e. nothing blocks unbind even if the module is pinned.
On 12/4/25 02:21, Dan Williams wrote: > The CXL subsystem is modular. That modularity is a benefit for > separation of concerns and testing. It is generally appropriate for this > class of devices that support hotplug and can dynamically add a CXL > personality alongside their PCI personality. However, a cost of modules > is ambiguity about when devices (cxl_memdevs, cxl_ports, cxl_regions) > have had a chance to attach to their corresponding drivers on > @cxl_bus_type. > > This problem of not being able to reliably determine when a device has > had a chance to attach to its driver vs still waiting for the module to > load, is a common problem for the "Soft Reserve Recovery" [1], and > "Accelerator Memory" [2] enabling efforts. > > For "Soft Reserve Recovery" it wants to use wait_for_device_probe() as a > sync point for when CXL devices present at boot have had a chance to > attach to the cxl_pci driver (generic CXL memory expansion class > driver). That breaks down if wait_for_device_probe() only flushes PCI > device probe, but not the cxl_mem_probe() of the cxl_memdev that > cxl_pci_probe() creates. > > For "Accelerator Memory", the driver is not cxl_pci, but any potential > PCI driver that wants to use the devm_cxl_add_memdev() ABI to attach to > the CXL memory domain. Those drivers want to know if the CXL link is > live end-to-end (from endpoint, through switches, to the host bridge) > and CXL memory operations are enabled. If not, a CXL accelerator may be > able to fall back to PCI-only operation. Similar to the "Soft Reserve > Memory" it needs to know that the CXL subsystem had a chance to probe > the ancestor topology of the device and let that driver make a > synchronous decision about CXL operation. IMO, this is not the problem with accelerators, because this can not be dynamically done, or not easily. The HW will support CXL or PCI, and if CXL mem is not enabled by the firmware, likely due to a negotiation/linking problem, the driver can keep going with CXL.io. Of course, this is from my experience with sfc driver/hardware. Note sfc driver added the check for CXL availability based on Terry's v13. But this is useful for solving the problem of module removal which can leave the type2 driver without the base for doing any unwinding. Once a type2 uses code from those other cxl modules explicitly, the problem is avoided. You seem to have forgotten about this problem, what I think it is worth to describe. > > In support of those efforts: > > * Clean up some resource lifetime issues in the current code > * Move some object creation symbols (devm_cxl_add_memdev() and > devm_cxl_add_endpoint()) into the cxl_mem.ko and cxl_port.ko objects. > Implicitly guarantee that cxl_mem_driver and cxl_port_driver have been > registered prior to any device objects being registered. This is > preferred over explicit open-coded request_module(). > * Use scoped-based-cleanup before adding more resource management in > devm_cxl_add_memdev() > * Give an accelerator the opportunity to run setup operations in > cxl_mem_probe() so it can further probe if the CXL configuration matches > its needs. > > Some of these previously appeared on a branch as an RFC [3] and left > "Soft Reserve Recovery" and "Accelerator Memory" to jockey for ordering. > Instead, create a shared topic branch for both of those efforts to > import. The main changes since that RFC are fixing a bug and reducing > the amount of refactoring (which contributed to hiding the bug). > > [1]: http://lore.kernel.org/20251120031925.87762-1-Smita.KoralahalliChannabasappa@amd.com > [2]: http://lore.kernel.org/20251119192236.2527305-1-alejandro.lucero-palau@amd.com > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-6.18/cxl-probe-order > > Dan Williams (6): > cxl/mem: Fix devm_cxl_memdev_edac_release() confusion > cxl/mem: Arrange for always-synchronous memdev attach > cxl/port: Arrange for always synchronous endpoint attach > cxl/mem: Convert devm_cxl_add_memdev() to scope-based-cleanup > cxl/mem: Drop @host argument to devm_cxl_add_memdev() > cxl/mem: Introduce a memdev creation ->probe() operation > > drivers/cxl/Kconfig | 2 +- > drivers/cxl/cxl.h | 2 + > drivers/cxl/cxlmem.h | 17 ++++-- > drivers/cxl/core/edac.c | 64 ++++++++++++--------- > drivers/cxl/core/memdev.c | 104 ++++++++++++++++++++++++----------- > drivers/cxl/mem.c | 69 +++++++++-------------- > drivers/cxl/pci.c | 2 +- > drivers/cxl/port.c | 40 ++++++++++++++ > tools/testing/cxl/test/mem.c | 2 +- > 9 files changed, 192 insertions(+), 110 deletions(-) > > > base-commit: ea5514e300568cbe8f19431c3e424d4791db8291
Alejandro Lucero Palau wrote: [..] > > For "Accelerator Memory", the driver is not cxl_pci, but any potential > > PCI driver that wants to use the devm_cxl_add_memdev() ABI to attach to > > the CXL memory domain. Those drivers want to know if the CXL link is > > live end-to-end (from endpoint, through switches, to the host bridge) > > and CXL memory operations are enabled. If not, a CXL accelerator may be > > able to fall back to PCI-only operation. Similar to the "Soft Reserve > > Memory" it needs to know that the CXL subsystem had a chance to probe > > the ancestor topology of the device and let that driver make a > > synchronous decision about CXL operation. > > > IMO, this is not the problem with accelerators, because this can not be > dynamically done, or not easily. Hmm, what do you mean can not be dynamically done? The observation is that a CXL card and its driver have no idea if the card is going to be plugged into a PCIe only slot. At runtime the driver only finds out the CXL is not there from the result of devm_cxl_add_memdev(). > The HW will support CXL or PCI, and if > CXL mem is not enabled by the firmware, likely due to a > negotiation/linking problem, the driver can keep going with CXL.io. Right, I think we are in violent agreement. > Of course, this is from my experience with sfc driver/hardware. Note > sfc driver added the check for CXL availability based on Terry's v13. Note that Terry's check for CXL availabilty is purely a hardware detection, there are still software reasons why cxl_acpi and cxl_mem can prevent devm_cxl_add_memdev() success. > But this is useful for solving the problem of module removal which can > leave the type2 driver without the base for doing any unwinding. Once a > type2 uses code from those other cxl modules explicitly, the problem is > avoided. You seem to have forgotten about this problem, what I think it > is worth to describe. What problem exactly? If it needs to be captured in these changelogs or code comments, let me know.
On 12/5/25 21:17, dan.j.williams@intel.com wrote: > Alejandro Lucero Palau wrote: > [..] >>> For "Accelerator Memory", the driver is not cxl_pci, but any potential >>> PCI driver that wants to use the devm_cxl_add_memdev() ABI to attach to >>> the CXL memory domain. Those drivers want to know if the CXL link is >>> live end-to-end (from endpoint, through switches, to the host bridge) >>> and CXL memory operations are enabled. If not, a CXL accelerator may be >>> able to fall back to PCI-only operation. Similar to the "Soft Reserve >>> Memory" it needs to know that the CXL subsystem had a chance to probe >>> the ancestor topology of the device and let that driver make a >>> synchronous decision about CXL operation. >> >> IMO, this is not the problem with accelerators, because this can not be >> dynamically done, or not easily. > Hmm, what do you mean can not be dynamically done? The observation is > that a CXL card and its driver have no idea if the card is going to be > plugged into a PCIe only slot. Right. > > At runtime the driver only finds out the CXL is not there from the > result of devm_cxl_add_memdev(). If there is no CXL properly initialized, what also implies a PCI-only slot, the driver can know looking at the CXL.mem and CXL.cache status in the CXL control register. That is what sfc driver does now using Terry's patchset instead of only checking CXL DVSEC and trying further CXL initialization using the CXL core API for Type2. Neither call to create cxl dev state nor memdev is needed to figure out. Of course, those calls can point to another kind of problem, but the driver can find out without using them. >> The HW will support CXL or PCI, and if >> CXL mem is not enabled by the firmware, likely due to a >> negotiation/linking problem, the driver can keep going with CXL.io. > Right, I think we are in violent agreement. > >> Of course, this is from my experience with sfc driver/hardware. Note >> sfc driver added the check for CXL availability based on Terry's v13. > Note that Terry's check for CXL availabilty is purely a hardware > detection, there are still software reasons why cxl_acpi and cxl_mem > can prevent devm_cxl_add_memdev() success. > >> But this is useful for solving the problem of module removal which can >> leave the type2 driver without the base for doing any unwinding. Once a >> type2 uses code from those other cxl modules explicitly, the problem is >> avoided. You seem to have forgotten about this problem, what I think it >> is worth to describe. > What problem exactly? If it needs to be captured in these changelogs or > code comments, let me know. It is a surprise you not remembering this ... v17 tried to fix this problem which was pointed out in v16 by you in several patches. v17: https://lore.kernel.org/linux-cxl/6887b72724173_11968100cb@dwillia2-mobl4.notmuch/ Next my reply to another comment from you trying to clarify/enumerate different problems which were getting intertwined creating confusion (at least to me). Sadly none did comment further, likely none read my explanation ... even if I asked for it with another email and specifically in one community meeting: https://lore.kernel.org/linux-cxl/836d06d6-a36f-4ba3-b7c9-ba8687ba2190@amd.com/ Next discussion about trying to solve the modules removal adding a callback by the driver which you did not like: https://lore.kernel.org/linux-cxl/6892325deccdb_55f09100fb@dwillia2-xfh.jf.intel.com.notmuch/ Here, v16, you stated specifically about cxl kernel modules removed while a Type2 driver is using cxl: https://lore.kernel.org/linux-cxl/682e2a0b9b15b_1626e10088@dwillia2-xfh.jf.intel.com.notmuch/ https://lore.kernel.org/linux-cxl/682e300371a0_1626e1003@dwillia2-xfh.jf.intel.com.notmuch/ https://lore.kernel.org/linux-cxl/682e3f3343977_1626e100b0@dwillia2-xfh.jf.intel.com.notmuch/
Alejandro Lucero Palau wrote:
[..]
> If there is no CXL properly initialized, what also implies a PCI-only
> slot, the driver can know looking at the CXL.mem and CXL.cache status in
> the CXL control register. That is what sfc driver does now using Terry's
> patchset instead of only checking CXL DVSEC and trying further CXL
> initialization using the CXL core API for Type2. Neither call to create
> cxl dev state nor memdev is needed to figure out. Of course, those calls
> can point to another kind of problem, but the driver can find out
> without using them.
It can, but I am not sure why a driver would want to open code a partial
answer to that question and not just rely on the CXL core to do the full
determination?
> >> The HW will support CXL or PCI, and if
> >> CXL mem is not enabled by the firmware, likely due to a
> >> negotiation/linking problem, the driver can keep going with CXL.io.
> > Right, I think we are in violent agreement.
> >
> >> Of course, this is from my experience with sfc driver/hardware. Note
> >> sfc driver added the check for CXL availability based on Terry's v13.
> > Note that Terry's check for CXL availabilty is purely a hardware
> > detection, there are still software reasons why cxl_acpi and cxl_mem
> > can prevent devm_cxl_add_memdev() success.
> >
> >> But this is useful for solving the problem of module removal which can
> >> leave the type2 driver without the base for doing any unwinding. Once a
> >> type2 uses code from those other cxl modules explicitly, the problem is
> >> avoided. You seem to have forgotten about this problem, what I think it
> >> is worth to describe.
> > What problem exactly? If it needs to be captured in these changelogs or
> > code comments, let me know.
>
>
> It is a surprise you not remembering this ...
I did not immediately recognize that this statement: "problem of module
removal which can leave the type2 driver without the base for doing any
unwinding". This set is about init time fixes so talking about removal
through me for a loop.
Thanks for the additional context below.
> v17 tried to fix this problem which was pointed out in v16 by you in
> several patches.
>
>
> v17:
>
> https://lore.kernel.org/linux-cxl/6887b72724173_11968100cb@dwillia2-mobl4.notmuch/
>
> Next my reply to another comment from you trying to clarify/enumerate
> different problems which were getting intertwined creating confusion (at
> least to me). Sadly none did comment further, likely none read my
> explanation ... even if I asked for it with another email and
> specifically in one community meeting:
>
> https://lore.kernel.org/linux-cxl/836d06d6-a36f-4ba3-b7c9-ba8687ba2190@amd.com/
So this also is about the init race, not removal, right?
This is why I think Smita's patches are a precursor to Type-2 because
both need that sync-point to when that platform CXL initialization has
completed.
> Next discussion about trying to solve the modules removal adding a
> callback by the driver which you did not like:
>
> https://lore.kernel.org/linux-cxl/6892325deccdb_55f09100fb@dwillia2-xfh.jf.intel.com.notmuch/
>
A proposal that implements what I talk about there is something like
this:
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 51a07cd85c7b..a4cb6d0f0da7 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -641,6 +641,16 @@ static void detach_memdev(struct work_struct *work)
struct cxl_memdev *cxlmd;
cxlmd = container_of(work, typeof(*cxlmd), detach_work);
+
+ /*
+ * Default to detaching the memdev, but in the case of memdev ops the
+ * memdev creator may want to detach the parent device as well.
+ */
+ if (cxlmd->ops && cxlmd->ops->detach) {
+ cxlmd->ops->detach(cxlmd);
+ return;
+ }
+
device_release_driver(&cxlmd->dev);
put_device(&cxlmd->dev);
}
Where that detach implementation is something like:
void accelerator_driver_detach(struct cxl_memdev *cxlmd)
{
device_release_driver(cxlmd->dev.parent);
/* the above also detaches the cxlmd via devm action */
put_device(&cxlmd->dev);
}
What I am not sure about is whether the accelerator driver needs the
ability to do anything besides shutdown when the CXL hierarchy is torn
down. I.e. no ->detach() callback, just make it the rule that when @ops
are specified devm_cxl_add_memdev() failure is a permanent failure at
registration time and CXL hierarchy removal also takes down the
accelerator driver.
© 2016 - 2025 Red Hat, Inc.