RE: [PATCH v13 3/9] cxl/mbox: Initialize the poison state

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

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Driver reads of the poison list are synchronized to ensure that a
> reader does not get an incomplete list because their request
> overlapped (was interrupted or preceded by) another read request
> of the same DPA range. (CXL Spec 3.0 Section 8.2.9.8.4.1). The
> driver maintains state information to achieve this goal.
> 
> To initialize the state, first recognize the poison commands in
> the CEL (Command Effects Log). If the device supports Get Poison
> List, allocate a single buffer for the poison list and protect it
> with a lock.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/mbox.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/cxl/cxlmem.h    | 34 +++++++++++++++++
>  drivers/cxl/pci.c       |  4 ++
>  3 files changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index fd1026970d3a..17737386283a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -5,6 +5,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/ktime.h>
>  #include <linux/mutex.h>
> +#include <cxlpci.h>
>  #include <cxlmem.h>
>  #include <cxl.h>
>  
> @@ -120,6 +121,43 @@ static bool cxl_is_security_command(u16 opcode)
>  	return false;
>  }
>  
> +static bool cxl_is_poison_command(u16 opcode)
> +{
> +#define CXL_MBOX_OP_POISON_CMDS 0x43
> +
> +	if ((opcode >> 8) == CXL_MBOX_OP_POISON_CMDS)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void cxl_set_poison_cmd_enabled(struct cxl_poison_state *poison,
> +				       u16 opcode)
> +{
> +	switch (opcode) {
> +	case CXL_MBOX_OP_GET_POISON:
> +		set_bit(CXL_POISON_ENABLED_LIST, poison->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_INJECT_POISON:
> +		set_bit(CXL_POISON_ENABLED_INJECT, poison->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_CLEAR_POISON:
> +		set_bit(CXL_POISON_ENABLED_CLEAR, poison->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS:
> +		set_bit(CXL_POISON_ENABLED_SCAN_CAPS, poison->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SCAN_MEDIA:
> +		set_bit(CXL_POISON_ENABLED_SCAN_MEDIA, poison->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_GET_SCAN_MEDIA:
> +		set_bit(CXL_POISON_ENABLED_SCAN_RESULTS, poison->enabled_cmds);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
>  {
>  	struct cxl_mem_command *c;
> @@ -635,13 +673,18 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
>  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
>  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>  
> -		if (!cmd) {
> +		if (!cmd && !cxl_is_poison_command(opcode)) {
>  			dev_dbg(cxlds->dev,
>  				"Opcode 0x%04x unsupported by driver\n", opcode);
>  			continue;
>  		}
>  
> -		set_bit(cmd->info.id, cxlds->enabled_cmds);
> +		if (cmd)
> +			set_bit(cmd->info.id, cxlds->enabled_cmds);
> +
> +		if (cxl_is_poison_command(opcode))
> +			cxl_set_poison_cmd_enabled(&cxlds->poison, opcode);
> +
>  		dev_dbg(cxlds->dev, "Opcode 0x%04x enabled\n", opcode);

Ah, nice I like how you suppressed the unsupported by driver message
which could have lead to confusion. I think we should duplicate this
approach for the DCD command set.

Looks good.