Re: [PATCH v3 09/16] cxl: Add support for FWCTL get driver information callback

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

Dave Jiang wrote:
> Add definition for fwctl_ops->info() to return driver information. The
> function will return a data structure with a reserved u32. This is setup
> for future usages.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Set command_info to reserved. (Dan)
> ---
>  drivers/cxl/fwctl.c      | 18 ++++++++++++++++--
>  include/cxl/mailbox.h    |  1 +
>  include/uapi/fwctl/cxl.h | 19 +++++++++++++++++++
>  3 files changed, 36 insertions(+), 2 deletions(-)
>  create mode 100644 include/uapi/fwctl/cxl.h
> 
> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> index 985315d5d503..85dee01d89d1 100644
> --- a/drivers/cxl/fwctl.c
> +++ b/drivers/cxl/fwctl.c
> @@ -6,6 +6,8 @@
>  #include "cxlmem.h"
>  #include "features.h"
>  
> +#define to_cxl_features_state(fwctl) container_of(fwctl, struct cxl_features_state, fwctl)
> +
>  static int cxlctl_open_uctx(struct fwctl_uctx *uctx)
>  {
>  	return 0;
> @@ -17,8 +19,20 @@ 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);
> +	struct fwctl_device *fwctl = uctx->fwctl;
> +	struct cxl_features_state *cxlfs = to_cxl_features_state(fwctl);
> +	struct fwctl_info_cxl *info;
> +
> +	if (!cxlfs->num_user_features)
> +		return ERR_PTR(-EOPNOTSUPP);

You could save an error case by not registering the fwctl interface at
all when ->num_features == 0.

> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	*length = sizeof(*info);
> +
> +	return info;
>  }
>  
>  static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
> index 99d3bbe1ba2a..bbbbff3af61f 100644
> --- a/include/cxl/mailbox.h
> +++ b/include/cxl/mailbox.h
> @@ -4,6 +4,7 @@
>  #define __CXL_MBOX_H__
>  #include <linux/rcuwait.h>
>  #include <cxl/features.h>
> +#include <uapi/fwctl/cxl.h>
>  #include <uapi/linux/cxl_mem.h>
>  
>  /**
> diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
> new file mode 100644
> index 000000000000..4e766ffb1388
> --- /dev/null
> +++ b/include/uapi/fwctl/cxl.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2024, Intel Corporation
> + *
> + * These are definitions for the mailbox command interface of CXL subsystem.
> + */
> +#ifndef _UAPI_FWCTL_CXL_H_
> +#define _UAPI_FWCTL_CXL_H_
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct fwctl_info_cxl - ioctl(FWCTL_INFO) out_device_data
> + * @reserved: Place older for future usages

ignoring the s/older/holder/, I would just document this as:

	@reserved: zero

...and not tease a future.