Re: [PATCH v1 10/19] cxl: Add FWCTL support to the CXL features 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>
> ---
> v1:
> - Add missing dev_set_name() (Jason)
> ---
>  drivers/cxl/Kconfig        |  1 +
>  drivers/cxl/features.c     | 44 ++++++++++++++++++++++++++++++++++++--
>  include/cxl/features.h     |  2 ++
>  include/uapi/fwctl/fwctl.h |  1 +
>  4 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 9a6ffd81ac0e..5de06c3806ce 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -148,6 +148,7 @@ config CXL_REGION_INVALIDATION_TEST
>  
>  config CXL_FEATURES
>  	tristate "CXL: Features support"
> +	depends on FWCTL
>  	default CXL_BUS
>  	help
>  	  Enable CXL features support that are tied to a CXL mailbox.
> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 1c4bcd7465cb..5a2b771586d3 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c
> @@ -9,6 +9,37 @@
>  #include "cxl.h"
>  #include "cxlmem.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 *rpc_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 cxl_features),
> +	.open_uctx = cxlctl_open_uctx,
> +	.close_uctx = cxlctl_close_uctx,
> +	.info = cxlctl_info,
> +	.fw_rpc = cxlctl_fw_rpc,
> +};
> +
>  static void cxl_free_feature_entries(void *entries)
>  {
>  	kvfree(entries);
> @@ -146,13 +177,16 @@ static int cxl_get_supported_features(struct cxl_features_state *cfs)
>  					cxl_free_feature_entries, cfs->entries);
>  }
>  
> +DEFINE_FREE(cfs, struct cxl_features_state *, if (_T) fwctl_put(&_T->fwctl))
> +
>  static int cxl_features_probe(struct device *dev)
>  {
>  	struct cxl_features *features = to_cxl_features(dev);
>  	int rc;
>  
> -	struct cxl_features_state *cfs __free(kfree) =
> -		kzalloc(sizeof(*cfs), GFP_KERNEL);
> +	struct cxl_features_state *cfs __free(cfs) =
> +		fwctl_alloc_device(dev, &cxlctl_ops,
> +				   struct cxl_features_state, fwctl);
>  	if (!cfs)
>  		return -ENOMEM;
>  
> @@ -161,6 +195,10 @@ static int cxl_features_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> +	rc = fwctl_register(&cfs->fwctl);
> +	if (rc)
> +		return rc;

Repeating our offline discussion...

After looking at what cxl_features_driver does and the fact that the
EDAC scrub support is enabled by registration calls from cxl_mem and
cxl_region. Let's just have cxl_mem or cxl_pci register fwctl relative
to a cxl_memdev. So, similar to how a cxl_memdev hosts an ioctl interface
and a fw_upload interface, add a fwctl interface with no new
device+driver objects.

When / if other components like CXL switches want to support fwctl that
can also be had as just a fwctl registration relative to a cxl_port
device which would have been awkward to handle with a sub-device+driver.

> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index f9b27fb5c161..2c3817592308 100644
> --- a/include/uapi/fwctl/fwctl.h
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -43,6 +43,7 @@ enum {
>  enum fwctl_device_type {
>  	FWCTL_DEVICE_TYPE_ERROR = 0,
>  	FWCTL_DEVICE_TYPE_MLX5 = 1,
> +	FWCTL_DEVICE_TYPE_CXL,

Jonathan and Jason already handled the comment here.