Message-ID: <20251007134053.00000dd3@huawei.com>
Date: Tue, 7 Oct 2025 13:40:53 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: <alejandro.lucero-palau@....com>
CC: <linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<dan.j.williams@...el.com>, <edward.cree@....com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<dave.jiang@...el.com>, Alejandro Lucero <alucerop@....com>
Subject: Re: [PATCH v19 01/22] cxl/mem: Arrange for always-synchronous
 memdev attach

On Mon, 6 Oct 2025 11:01:09 +0100
<alejandro.lucero-palau@....com> wrote:

> From: Alejandro Lucero <alucerop@....com>
> 
> In preparation for CXL accelerator drivers that have a hard dependency on
> CXL capability initialization, arrange for the endpoint probe result to be
> conveyed to the caller of devm_cxl_add_memdev().
> 
> As it stands 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, that driver needs to know perform driver
> specific initialization if CXL is available, or exectute a fallback to PCIe
> only operation.
> 
> By moving devm_cxl_add_memdev() to cxl_mem.ko it removes async module
> loading as one reason that a memdev may not be attached upon return from
> devm_cxl_add_memdev().
> 
> The diff is busy as this moves cxl_memdev_alloc() down below the definition
> of cxl_memdev_fops and introduces devm_cxl_memdev_add_or_reset() to
> preclude needing to export more symbols from the cxl_core.
> 
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>

Alejandro, SoB chain broken here which makes this currently unmergeable.

Should definitely have your SoB as you sent the patch to the list and need
to make a statement that you believe it to be fine to do so (see the Certificate
of origin stuff in the docs).  Also, From should always be one of the authors.
If Dan wrote this as the SoB suggests then From should be set to him..

git commit --amend --author="Dan Williams <dan.j.williams@...el.com>"

Will fix that up.  Then either you add your SoB on basis you just 'handled'
the patch but didn't make substantial changes, or your SoB and a Codeveloped-by
if you did make major changes.  If it is minor stuff you can an
a sign off with # what changed 
comment next to it.

A few minor comments inline.

Thanks,

Jonathan


> ---
>  drivers/cxl/Kconfig       |  2 +-
>  drivers/cxl/core/memdev.c | 97 ++++++++++++++++-----------------------
>  drivers/cxl/mem.c         | 30 ++++++++++++
>  drivers/cxl/private.h     | 11 +++++
>  4 files changed, 82 insertions(+), 58 deletions(-)
>  create mode 100644 drivers/cxl/private.h
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 028201e24523..111e05615f09 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -22,6 +22,7 @@ if CXL_BUS
>  config CXL_PCI
>  	tristate "PCI manageability"
>  	default CXL_BUS
> +	select CXL_MEM
>  	help
>  	  The CXL specification defines a "CXL memory device" sub-class in the
>  	  PCI "memory controller" base class of devices. Device's identified by
> @@ -89,7 +90,6 @@ config CXL_PMEM
>  
>  config CXL_MEM
>  	tristate "CXL: Memory Expansion"
> -	depends on CXL_PCI
>  	default CXL_BUS
>  	help
>  	  The CXL.mem protocol allows a device to act as a provider of "System
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index c569e00a511f..2bef231008df 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c

> -
> -err:
> -	kfree(cxlmd);
> -	return ERR_PTR(rc);
>  }
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_memdev_add_or_reset, "CXL");
>  
>  static long __cxl_memdev_ioctl(struct cxl_memdev *cxlmd, unsigned int cmd,
>  			       unsigned long arg)
> @@ -1023,50 +1012,44 @@ static const struct file_operations cxl_memdev_fops = {
>  	.llseek = noop_llseek,
>  };
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> -				       struct cxl_dev_state *cxlds)
> +struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_memdev *cxlmd;
>  	struct device *dev;
>  	struct cdev *cdev;
>  	int rc;
>  
> -	cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
> -	if (IS_ERR(cxlmd))
> -		return cxlmd;
> +	cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL);

It's a little bit non obvious due to the device initialize mid way
through this, but given there are no error paths after that you can
currently just do.
	struct cxl_memdev *cxlmd __free(kfree) =
		cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
and
	return_ptr(cxlmd);

in the good path.  That lets you then just return rather than having
the goto err: handling for the error case that currently frees this
manually.

Unlike the change below, this one I think is definitely worth making.


> +	if (!cxlmd)
> +		return ERR_PTR(-ENOMEM);
>  
> -	dev = &cxlmd->dev;
> -	rc = dev_set_name(dev, "mem%d", cxlmd->id);
> -	if (rc)
> +	rc = ida_alloc_max(&cxl_memdev_ida, CXL_MEM_MAX_DEVS - 1, GFP_KERNEL);
> +	if (rc < 0)
>  		goto err;
> -
> -	/*
> -	 * Activate ioctl operations, no cxl_memdev_rwsem manipulation
> -	 * needed as this is ordered with cdev_add() publishing the device.
> -	 */
> +	cxlmd->id = rc;
> +	cxlmd->depth = -1;
>  	cxlmd->cxlds = cxlds;
>  	cxlds->cxlmd = cxlmd;
>  
> -	cdev = &cxlmd->cdev;
> -	rc = cdev_device_add(cdev, dev);
> -	if (rc)
> -		goto err;
> +	dev = &cxlmd->dev;
> +	device_initialize(dev);
> +	lockdep_set_class(&dev->mutex, &cxl_memdev_key);
> +	dev->parent = cxlds->dev;
> +	dev->bus = &cxl_bus_type;
> +	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> +	dev->type = &cxl_memdev_type;
> +	device_set_pm_not_required(dev);
> +	INIT_WORK(&cxlmd->detach_work, detach_memdev);
>  
> -	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
> -	if (rc)
> -		return ERR_PTR(rc);
> +	cdev = &cxlmd->cdev;
> +	cdev_init(cdev, &cxl_memdev_fops);
>  	return cxlmd;
>  
>  err:
> -	/*
> -	 * The cdev was briefly live, shutdown any ioctl operations that
> -	 * saw that state.
> -	 */
> -	cxl_memdev_shutdown(dev);
> -	put_device(dev);
> +	kfree(cxlmd);
>  	return ERR_PTR(rc);
>  }
> -EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, "CXL");
> +EXPORT_SYMBOL_NS_GPL(cxl_memdev_alloc, "CXL");
>  
>  static void sanitize_teardown_notifier(void *data)
>  {
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index f7dc0ba8905d..144749b9c818 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -7,6 +7,7 @@
>  
>  #include "cxlmem.h"
>  #include "cxlpci.h"
> +#include "private.h"
>  #include "core/core.h"
>  
>  /**
> @@ -203,6 +204,34 @@ static int cxl_mem_probe(struct device *dev)
>  	return devm_add_action_or_reset(dev, enable_suspend, NULL);
>  }
>  
> +/**
> + * devm_cxl_add_memdev - Add a CXL memory device
> + * @host: devres alloc/release context and parent for the memdev
> + * @cxlds: CXL device state to associate with the memdev
> + *
> + * Upon return the device will have had a chance to attach to the
> + * cxl_mem driver, but may fail if the CXL topology is not ready
> + * (hardware CXL link down, or software platform CXL root not attached)
> + */
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_memdev *cxlmd = cxl_memdev_alloc(cxlds);

Bit marginal but you could do a DEFINE_FREE() for cxlmd 
similar to the one that exists for put_cxl_port

You would then need to steal the pointer for the devm_ call at the
end of this function.


> +	int rc;
> +
> +	if (IS_ERR(cxlmd))
> +		return cxlmd;
> +
> +	rc = dev_set_name(&cxlmd->dev, "mem%d", cxlmd->id);
> +	if (rc) {
> +		put_device(&cxlmd->dev);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return devm_cxl_memdev_add_or_reset(host, cxlmd);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, "CXL");

Powered by blists - more mailing lists