Re: [PATCH 1/7] cxl/mbox: Track background commands from CEL

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

On Tue, 17 Jun 2025 12:36:05 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Remember whether or not a command is background-capable, per the CEL.
> This will later be used to consult the bitmap to evaluate incoming
> mbox commands.
> 
> Further, "enable" any background commands that didn't fall in the
> uapi, poison or security categories, such as fw related. This prevents
> situations where the command is incorrectly listed as unsupported by
> the driver.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c | 30 ++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h    | 28 ++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2689e6453c5a..4ea02f7a5b6b 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -202,6 +202,31 @@ static void cxl_set_poison_cmd_enabled(struct cxl_poison_state *poison,
>  	}
>  }
>  
> +static bool cxl_set_bgcmd_enabled(struct cxl_background_state *bg, u16 opcode)
> +{
> +	switch (opcode) {
> +	case CXL_MBOX_OP_TRANSFER_FW:
> +		set_bit(CXL_BG_ENABLED_TRANSFER_FW, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_ACTIVATE_FW:
> +		set_bit(CXL_BG_ENABLED_ACTIVATE_FW, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_DO_MAINTENANCE:
> +		set_bit(CXL_BG_ENABLED_DO_MAINTENANCE, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SCAN_MEDIA:
> +		set_bit(CXL_BG_ENABLED_SCAN_MEDIA, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SANITIZE:
> +		set_bit(CXL_BG_ENABLED_SANITIZE, bg->enabled_cmds);
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
>  {
>  	struct cxl_mem_command *c;
> @@ -757,6 +782,7 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  
>  	for (i = 0; i < cel_entries; i++) {
>  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> +		u16 effect = le16_to_cpu(cel_entry[i].effect);
>  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>  		int enabled = 0;
>  
> @@ -778,6 +804,10 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  			enabled++;
>  		}
>  
> +		if (effect & CXL_CEL_BACKGROUND_OPERATION)
> +			if (cxl_set_bgcmd_enabled(&mds->bg, opcode))
> +				enabled++; /* might be redundant */

I suspect we'll forget in the long run what 'redundant' means here.
Perhaps make it more explicit.
/* May already have been detected as enabled */

The whole incrementing of enabled pattern hides that we don't really care
about transitions other than 0->1.  Maybe we should consider making it
a bool and using
enabled |= check_features_opcode() a few lines above this.



> +
>  		dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
>  			enabled ? "enabled" : "unsupported by driver");
>  	}
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 551b0ba2caa1..5c7fd4a6704c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -248,6 +248,16 @@ enum security_cmd_enabled_bits {
>  	CXL_SEC_ENABLED_MAX
>  };
>  
> +/* Device enabled background commands */

What does 'Device enabled' refer to? Perhaps reword.

> +enum background_cmd_enabled_bits {
> +	CXL_BG_ENABLED_TRANSFER_FW,
> +	CXL_BG_ENABLED_ACTIVATE_FW,
> +	CXL_BG_ENABLED_DO_MAINTENANCE,
> +	CXL_BG_ENABLED_SCAN_MEDIA,
> +	CXL_BG_ENABLED_SANITIZE,
> +	CXL_BG_ENABLED_MAX
> +};
> +
>  /**
>   * struct cxl_poison_state - Driver poison state info
>   *
> @@ -365,6 +375,16 @@ struct cxl_security_state {
>  	struct kernfs_node *sanitize_node;
>  };
>  
> +/**
> + * struct cxl_background_state - Driver background operation state info
> + *
> + * @enabled_cmds: All background commands enabled in the CEL
> + */
> +struct cxl_background_state {
> +	DECLARE_BITMAP(enabled_cmds, CXL_BG_ENABLED_MAX);
> +};
> +
> +
>  /*
>   * enum cxl_devtype - delineate type-2 from a generic type-3 device
>   * @CXL_DEVTYPE_DEVMEM - Vendor specific CXL Type-2 device implementing HDM-D or
> @@ -484,6 +504,7 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
>   * @poison: poison driver state info
>   * @security: security driver state info
>   * @fw: firmware upload / activation state
> + * @bg: background operation state
>   * @mce_notifier: MCE notifier
>   *
>   * See CXL 3.0 8.2.9.8.2 Capacity Configuration and Label Storage for
> @@ -504,6 +525,7 @@ struct cxl_memdev_state {
>  	struct cxl_poison_state poison;
>  	struct cxl_security_state security;
>  	struct cxl_fw_state fw;
> +	struct cxl_background_state bg;
>  	struct notifier_block mce_notifier;
>  };
>  
> @@ -580,6 +602,12 @@ struct cxl_mbox_get_supported_logs {
>  	} __packed entry[];
>  }  __packed;
>  
> +/*
> + * Command Effects Log (CEL)
> + * CXL 3.2 Section 8.2.10.5.2.1; Table 8-87
> + */
> +#define CXL_CEL_BACKGROUND_OPERATION BIT(6)
> +
>  struct cxl_cel_entry {
>  	__le16 opcode;
>  	__le16 effect;