Re: [PATCH v4 01/15] cxl: Enumerate feature commands

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

On Fri, 7 Feb 2025 16:37:41 -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>

One trivial comment inline. Either way LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> v4:
> - Move features cap to mbox. (Dan)
> - Drop features_cmds bitmap. (Dan)
> - Move setup of cxlfs to next patch since there's nothing to do here.
> ---
>  drivers/cxl/core/mbox.c | 36 +++++++++++++++++++++++++++++++++++-
>  drivers/cxl/cxlmem.h    |  3 +++
>  include/cxl/features.h  | 13 +++++++++++++
>  include/cxl/mailbox.h   |  3 +++
>  4 files changed, 54 insertions(+), 1 deletion(-)
>  create mode 100644 include/cxl/features.h
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9c1b9e353e3e..5f8f5945cc02 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -706,6 +706,35 @@ static int cxl_xfer_log(struct cxl_memdev_state *mds, uuid_t *uuid,
>  	return 0;
>  }
>  
> +static int check_features_opcodes(u16 opcode, int *ro_cmds, int *wr_cmds)
> +{
> +	switch (opcode) {
> +	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
> +	case CXL_MBOX_OP_GET_FEATURE:
> +		(*ro_cmds)++;
> +		return 1;
> +	case CXL_MBOX_OP_SET_FEATURE:
> +		(*wr_cmds)++;
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +/* 'Get Supported Features' and 'Get Feature' */
> +#define MAX_FEATURES_READ_CMDS	2
> +static void set_features_cap(struct cxl_mailbox *cxl_mbox,
> +			     int ro_cmds, int wr_cmds)
> +{
> +	/* Setting up Features capability while walking the CEL */
> +	if (ro_cmds == MAX_FEATURES_READ_CMDS) {
> +		if (wr_cmds)
> +			cxl_mbox->feat_cap = CXL_FEATURES_RW;
> +		else
> +			cxl_mbox->feat_cap = CXL_FEATURES_RO;
> +	}
> +}
> +
>  /**
>   * cxl_walk_cel() - Walk through the Command Effects Log.
>   * @mds: The driver data for the operation
> @@ -721,7 +750,7 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  	struct cxl_cel_entry *cel_entry;
>  	const int cel_entries = size / sizeof(*cel_entry);
>  	struct device *dev = mds->cxlds.dev;
> -	int i;
> +	int i, ro_cmds = 0, wr_cmds = 0;
>  
>  	cel_entry = (struct cxl_cel_entry *) cel;
>  
> @@ -733,6 +762,9 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  		if (cmd) {
>  			set_bit(cmd->info.id, cxl_mbox->enabled_cmds);
>  			enabled++;
> +		} else {

All the following (is poison etc) are also effectively 'else' cases for this
if (cmd) but for simplicity of code that's not checked.

So maybe this should not be an else either?


> +			enabled += check_features_opcodes(opcode, &ro_cmds,
> +							  &wr_cmds);
>  		}
>  
>  		if (cxl_is_poison_command(opcode)) {
> @@ -748,6 +780,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  		dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
>  			enabled ? "enabled" : "unsupported by driver");
>  	}
> +
> +	set_features_cap(cxl_mbox, ro_cmds, wr_cmds);
>  }
>