Re: [PATCH v1 03/19] cxl: Enumerate feature commands

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

On Wed, 22 Jan 2025 16:50:34 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add feature commands enumeration code in order to detect and enumerate
> the 3 feature related commands "get supported features", "get feature",
> and "set feature". The enumeration will help determine whether the driver
> can issue any of the 3 commands to the device.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/mbox.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h    |  3 +++
>  include/cxl/features.h  |  7 +++++++
>  include/cxl/mailbox.h   |  1 +
>  4 files changed, 52 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index bdb8f060f2c1..5e21ff99d70f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -38,6 +38,21 @@ static bool cxl_raw_allow_all;
>  	.flags = _flags,                                                       \
>  	}
>  
> +#define cxl_for_each_feature_cmd(cmd)                                          \
> +	for ((cmd) = &cxl_feature_commands[0];                                 \
> +	     ((cmd) - cxl_feature_commands) < ARRAY_SIZE(cxl_feature_commands); (cmd)++)
> +
> +#define CXL_FEATURE_CMD(_id, sin, sout, _flags)                                \
> +	[CXL_FEATURE_ID_##_id] = {                                             \
> +	.info =	{                                                              \
> +			.id = CXL_FEATURE_ID_##_id,                            \
> +			.size_in = sin,                                        \
> +			.size_out = sout,                                      \
> +		},                                                             \
> +	.opcode = CXL_MBOX_OP_##_id,                                           \
> +	.flags = _flags,                                                       \
> +	}
> +
>  #define CXL_VARIABLE_PAYLOAD	~0U
>  /*
>   * This table defines the supported mailbox commands for the driver. This table
> @@ -69,6 +84,13 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
>  };
>  
> +#define CXL_FEATURE_COMMAND_ID_MAX 3
Why do we need this define separately from CXL_FEATURE_ID_MAX as I'm fairly sure they 
must always be the same.  A [] array would work here too.

> +static struct cxl_mem_command cxl_feature_commands[CXL_FEATURE_COMMAND_ID_MAX] = {
> +	CXL_FEATURE_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD, 0),
> +	CXL_FEATURE_CMD(GET_FEATURE, 0xf, CXL_VARIABLE_PAYLOAD, 0),
> +	CXL_FEATURE_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0),
> +};

> diff --git a/include/cxl/features.h b/include/cxl/features.h
> index b92da1e92780..7a8be3c621a1 100644
> --- a/include/cxl/features.h
> +++ b/include/cxl/features.h
> @@ -5,6 +5,13 @@
>  
>  struct cxl_mailbox;
>  
> +enum feature_cmds {
> +	CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0,
> +	CXL_FEATURE_ID_GET_FEATURE,
> +	CXL_FEATURE_ID_SET_FEATURE,
> +	CXL_FEATURE_ID_MAX,

No comma on that one as it's meant to always be the terminator.

> +};