Re: [PATCH v5 10/15] cxl: Add support for fwctl RPC command to enable CXL feature commands

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

On Tue, 11 Feb 2025 11:28:04 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
> to a device. The cxl fwctl driver will start by supporting the CXL
> Feature commands: Get Supported Features, Get Feature, and Set Feature.
> 
> The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
> it indicates the security scope of the call. The Get Supported Features
> and Get Feature calls can be executed with the scope of
> FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects
> of the Feature reported by Get Supported Features call for the specific
> Feature.
> 
> Only "Get Supported Features" is supported in this patch. Additional
> commands will be added in follow on patches. "Get Supported Features"
> will filter the Features that are exclusive to the kernel. The flag
> field of the Feature details will be cleared of the "Changeable"
> field and the "set feat size" will be set to 0 to indicate that
> the feature is not changeable.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

One trivial suggestion inline. Feel free to ignore!

J
> ---
> v5:
> - drop unused feature_cmds. (Dan)
> - Fix index of for loop. (Ming)
> ---
>  drivers/cxl/core/features.c | 218 +++++++++++++++++++++++++++++++++++-
>  include/uapi/cxl/features.h |   1 +
>  include/uapi/fwctl/cxl.h    |  29 +++++
>  3 files changed, 246 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 923c054599ad..a43be03ada43 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -365,11 +365,225 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
>  	return info;
>  }

> +static bool cxlctl_validate_hw_command(struct cxl_features_state *cxlfs,
> +				       const struct fwctl_rpc_cxl *rpc_in,
> +				       enum fwctl_rpc_scope scope,
> +				       u16 opcode)
> +{
> +	struct cxl_mailbox *cxl_mbox = &cxlfs->cxlds->cxl_mbox;
> +
> +	switch (opcode) {
> +	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
> +	case CXL_MBOX_OP_GET_FEATURE:
> +		if (cxl_mbox->feat_cap < CXL_FEATURES_RO)
> +			return false;
> +		if (scope >= FWCTL_RPC_CONFIGURATION)
> +			return true;
> +		return false;

Trivial but could do

		return scope >= FWCTL_RPC_CONFIGURATION;

up to you though.

> +	case CXL_MBOX_OP_SET_FEATURE:
> +		if (cxl_mbox->feat_cap < CXL_FEATURES_RW)
> +			return false;
> +		return cxlctl_validate_set_features(cxlfs, rpc_in, scope);
> +	default:
> +		return false;
> +	}
> +}