Re: [PATCH v3 08/16] cxl: Add FWCTL support to the CXL memdev driver

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang wrote:
> Add fwctl support code to allow sending of CXL feature commands from
> userspace through as ioctls via FWCTL. Provide initial setup bits.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Fix spelling error in Kconfig text
> - Remove errant blank line
> - Drop using of __weak. (Jason)
> - Return NULL for allocate function. (Jonathan)
> ---
>  drivers/cxl/Kconfig        | 15 +++++++
>  drivers/cxl/Makefile       |  1 +
>  drivers/cxl/features.c     |  8 +++-
>  drivers/cxl/features.h     | 29 +++++++++++++
>  drivers/cxl/fwctl.c        | 89 ++++++++++++++++++++++++++++++++++++++
>  include/cxl/features.h     |  3 ++
>  include/uapi/fwctl/fwctl.h |  1 +
>  tools/testing/cxl/Kbuild   |  1 +
>  8 files changed, 145 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/cxl/fwctl.c
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 876469e23f7a..6bb1ffc74956 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -102,6 +102,21 @@ config CXL_MEM
>  
>  	  If unsure say 'm'.
>  
> +config CXL_FWCTL
> +	bool "CXL: Firmware Control support"
> +	depends on CXL_MEM
> +	select FWCTL
> +	help
> +	  Enable support for firmware control of CXL devices. This allows
> +	  a FWCTL char device to be created in order to issue ioctls from
> +	  user space to exercise the CXL Features commands supported by
> +	  a CXL mailbox. There are some Features defined in the CXL spec
> +	  such as memory sparing. However a lot of those features are
> +	  exclusive to the kernel. Most likely the Features manipulated
> +	  by CXL_FWCTL will be vendor defined Features.
> +
> +	  If unsure say 'n'.

Per previous comments, combine this with the CONFIG_CXL_FEATURES config
option and treat it all as one for now.

> +
>  config CXL_PORT
>  	default CXL_BUS
>  	tristate
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index 12fbc35081bb..cd28fb3bf855 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -18,4 +18,5 @@ cxl_port-y := port.o
>  cxl_acpi-y := acpi.o
>  cxl_pmem-y := pmem.o security.o
>  cxl_mem-y := mem.o features.o
> +cxl_mem-$(CONFIG_CXL_FWCTL) += fwctl.o
>  cxl_pci-y := pci.o
> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 0086fdb20bfd..c14dc6d45985 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> +#include <linux/pci.h>
>  #include <linux/device.h>
> +#include <linux/module.h>
>  #include <cxl/mailbox.h>
>  #include <cxl/features.h>
>  #include "cxl.h"
> @@ -173,7 +175,7 @@ static void cxlfs_free(void *_cxlfs)
>  }
>  DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) cxlfs_free(_T))
>  
> -static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
> +struct cxl_features_state *_devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
>  {
>  	int rc;
>  
> @@ -191,7 +193,7 @@ static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
>  	return no_free_ptr(cxlfs);
>  }
>  
> -static void devm_cxlfs_free(struct cxl_memdev *cxlmd)
> +void _devm_cxlfs_free(struct cxl_memdev *cxlmd)
>  {
>  	kfree(cxlmd->cxlfs);
>  	/* Set in devm_cxl_add_features(), make sure it's cleared */
> @@ -231,3 +233,5 @@ int devm_cxl_add_features(struct cxl_memdev *cxlmd)
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_add_features, "CXL");
> +
> +MODULE_IMPORT_NS("CXL");

What symbols does features.c need to import? I think this all gets
simpler with the proposal to move all of this to cxl_core.ko optional
features.o object.

> diff --git a/drivers/cxl/features.h b/drivers/cxl/features.h
> index 8706f944b476..a51625774b58 100644
> --- a/drivers/cxl/features.h
> +++ b/drivers/cxl/features.h
> @@ -3,8 +3,37 @@
>  #ifndef __CXL_FEATURES_LOCAL__
>  #define __CXL_FEATURES_LOCAL__
>  
> +struct cxl_features_state *_devm_cxlfs_allocate(struct cxl_memdev *cxlmd);
> +void _devm_cxlfs_free(struct cxl_memdev *cxlmd);
> +
> +#ifdef CONFIG_CXL_FWCTL
> +struct cxl_features_state *_devm_cxlfs_fwctl_allocate(struct cxl_memdev *cxlmd);
> +void _devm_cxlfs_fwctl_free(struct cxl_memdev *cxlmd);
> +#endif
> +
> +static inline struct cxl_features_state *
> +devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
> +{
> +#ifdef CONFIG_CXL_FWCTL
> +	return _devm_cxlfs_fwctl_allocate(cxlmd);
> +#else
> +	return _devm_cxlfs_allocate(cxlmd);
> +#endif
> +}
> +
> +static inline void devm_cxlfs_free(struct cxl_memdev *cxlmd)
> +{
> +#ifdef CONFIG_CXL_FWCTL
> +	_devm_cxlfs_fwctl_free(cxlmd);
> +#else
> +	_devm_cxlfs_free(cxlmd);
> +#endif
> +}

This looks odd. Lets just do an approach like this from cxl_pci:

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b2c943a4de0a..47d432e34ccf 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1002,6 +1002,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        if (rc)
                return rc;
 
+       rc = devm_cxl_setup_features(&pdev->dev, cxlds);
+       if (rc)
+               return rc;
+
        cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
        if (IS_ERR(cxlmd))
                return PTR_ERR(cxlmd);
@@ -1010,6 +1014,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        if (rc)
                return rc;
 
+       rc = devm_cxl_setup_fwctl(&pdev->dev, cxlmd);
+       if (rc)
+               return rc;
+
        rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
        if (rc)
                return rc;

...where Feature command functionality is ready to be used by kernel
internal paths prior to registering a memdev, in support of use cases
like EDAC memory repair. Then fwctl is registered after the memdev so it
can reference that object.

Then the header file looks like a more typical:

#ifdef CONFIG_CXL_FEATURES
int devm_cxl_setup_features(struct device *host, struct cxl_dev_state *cxlds);
int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd);
#else
static inline int devm_cxl_setup_features(struct device *host, struct cxl_dev_state *cxlds)
{
	return 0;
}
static inline int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd)
{
	return 0;
}
#endif

> +
>  struct cxl_feat_entry;
>  
> +struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd);
> +void devm_cxlfs_free(struct cxl_memdev *cxlmd);
>  int devm_cxl_add_features(struct cxl_memdev *cxlmd);
>  bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry);
>  
> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> new file mode 100644
> index 000000000000..985315d5d503
> --- /dev/null
> +++ b/drivers/cxl/fwctl.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2025 Intel Corporation. All rights reserved. */
> +#include <linux/fwctl.h>
> +#include <linux/device.h>
> +#include <cxl/features.h>
> +#include "cxlmem.h"
> +#include "features.h"
> +
> +static int cxlctl_open_uctx(struct fwctl_uctx *uctx)
> +{
> +	return 0;
> +}
> +
> +static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
> +{
> +}
> +
> +static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
> +{
> +	/* Place holder */
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> +			   void *in, size_t in_len, size_t *out_len)
> +{
> +	/* Place holder */
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static const struct fwctl_ops cxlctl_ops = {
> +	.device_type = FWCTL_DEVICE_TYPE_CXL,
> +	.uctx_size = sizeof(struct fwctl_uctx),
> +	.open_uctx = cxlctl_open_uctx,
> +	.close_uctx = cxlctl_close_uctx,
> +	.info = cxlctl_info,
> +	.fw_rpc = cxlctl_fw_rpc,
> +};
> +
> +static void remove_cxlfs(void *_cxlfs)
> +{
> +	struct cxl_features_state *cxlfs = _cxlfs;
> +	struct cxl_memdev *cxlmd = cxlfs->cxlmd;
> +	struct fwctl_device *fwctl = &cxlfs->fwctl;
> +
> +	/* Set in devm_cxl_add_features(), make sure it's cleared */
> +	cxlmd->cxlfs = NULL;
> +	fwctl_unregister(fwctl);
> +	fwctl_put(fwctl);
> +}
> +
> +DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) fwctl_put(&_T->fwctl))
> +
> +static struct cxl_features_state *
> +__devm_cxlfs_fwctl_allocate(struct cxl_memdev *cxlmd, const struct fwctl_ops *ops)

This is more than "allocate" it also registers ABI.

> +{
> +	struct device *dev = &cxlmd->dev;
> +	int rc;
> +
> +	struct cxl_features_state *cxlfs __free(free_cxlfs) =
> +		fwctl_alloc_device(dev, ops, struct cxl_features_state, fwctl);
> +	if (!cxlfs)
> +		return NULL;
> +
> +	cxlfs->cxlmd = cxlmd;
> +	rc = fwctl_register(&cxlfs->fwctl);
> +	if (rc)
> +		return NULL;
> +
> +	rc = devm_add_action_or_reset(dev, remove_cxlfs, cxlfs);
> +	if (rc)
> +		return NULL;

Same double free bug here, i.e. needs to be a handoff to the devm
domain:

	rc = devm_add_action_or_reset(dev, remove_cxlfs, no_free_ptr(cxlfs));

...but this also might be another case of a function that wants to
move the devm action responsibility to the caller.