Re: [PATCH v2 6/6] cxl/mem: Introduce cxl_memdev_attach for CXL-dependent operation

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Mon, 15 Dec 2025 16:56:16 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Unlike the cxl_pci class driver that opportunistically enables memory
> expansion with no other dependent functionality, CXL accelerator drivers
> have distinct PCIe-only and CXL-enhanced operation states. If CXL is
> available some additional coherent memory/cache operations can be enabled,
> otherwise traditional DMA+MMIO over PCIe/CXL.io is a fallback.
> 
> Allow for a driver to pass a routine to be called in cxl_mem_probe()
> context. This ability is inspired by and mirrors the semantics of
> faux_device_create(). It allows for the caller to run CXL-topology
> attach-dependent logic on behalf of the caller.

This seems confusing.  The caller is running logic for the caller?
It can do that whenever it likes!  One of those is presumably callee



> 
> The probe callback runs after the port topology is successfully attached
> for the given memdev.
> 
> Additionally the presence of @cxlmd->attach indicates that the accelerator
> driver be detached when CXL operation ends. This conceptually makes a CXL
> link loss event mirror a PCIe link loss event which results in triggering
> the ->remove() callback of affected devices+drivers. A driver can re-attach
> to recover back to PCIe-only operation. Live recovery, i.e. without a
> ->remove()/->probe() cycle, is left as a future consideration.  
> 
> Cc: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com> (✓ DKIM/intel.com)

Have we started adding DKIM stuff to tags?

> Tested-by: Alejandro Lucero <alucerop@amd.com> (✓ DKIM/amd.com)
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
One trivial thing on function naming inline.  Either way.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

To me this looks good to start building the other stuff on top of.
Thanks for unblocking this stuff (hopefully)

Jonathan

> ---
>  drivers/cxl/cxlmem.h         | 12 ++++++++++--
>  drivers/cxl/core/memdev.c    | 33 +++++++++++++++++++++++++++++----
>  drivers/cxl/mem.c            | 20 ++++++++++++++++----
>  drivers/cxl/pci.c            |  2 +-
>  tools/testing/cxl/test/mem.c |  2 +-
>  5 files changed, 57 insertions(+), 12 deletions(-)

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 63da2bd4436e..3ab4cd8f19ed 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c


> @@ -1081,6 +1093,18 @@ static struct cxl_memdev *cxl_memdev_autoremove(struct cxl_memdev *cxlmd)
>  {
>  	int rc;
>  
> +	/*

The general approach is fine but is the function name appropriate for this
new stuff?  Naturally I'm not suggesting the bikeshed should be any particular
alternative color just maybe not the current blue.


> +	 * If @attach is provided fail if the driver is not attached upon
> +	 * return. Note that failure here could be the result of a race to
> +	 * teardown the CXL port topology. I.e. cxl_mem_probe() could have
> +	 * succeeded and then cxl_mem unbound before the lock is acquired.
> +	 */
> +	guard(device)(&cxlmd->dev);
> +	if (cxlmd->attach && !cxlmd->dev.driver) {
> +		cxl_memdev_unregister(cxlmd);
> +		return ERR_PTR(-ENXIO);
> +	}
> +
>  	rc = devm_add_action_or_reset(cxlmd->cxlds->dev, cxl_memdev_unregister,
>  				      cxlmd);
>  	if (rc)