Re: [PATCH] cxl/pci: Set the device timestamp

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

Jonathan Cameron wrote:
> > > >  /*
> > > > @@ -857,6 +859,29 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds)
> > > >  }
> > > >  EXPORT_SYMBOL_NS_GPL(cxl_mem_create_range_info, CXL);
> > > >  
> > > > +int cxl_set_timestamp(struct cxl_dev_state *cxlds, u64 ts)
> > > > +{
> > > > +	struct cxl_mbox_cmd mbox_cmd;
> > > > +	struct cxl_mbox_set_timestamp_in pi;
> > > > +
> > > > +	/*
> > > > +	 * Command is optional and functionality should not be affected if
> > > > +	 * the command is not available.
> > > > +	 */
> > > > +	if (!test_bit(CXL_MEM_COMMAND_ID_SET_TIMESTAMP, cxlds->enabled_cmds))
> > > > +		return 0;
> 
> One side effect of dropping the userspace handling is we loose
> the presence in enabled_cmds (based on the CEL).  I've replaced
> this with specific handling of the Not Supported mailbox return code
> and suitable comments on why I'm not considering that an error.
> 
> Hopefully that compromise makes sense.

That does make sense.

That also has implications for the debug messages that will say that the "set
timestamp" opcode is unsupported.

What do you think of the following to try to clarify the distinction of
supported opcodes that are used internally by the driver and the ones
that are wrapped by a user command?

-- >8 --
From d36b9ce647e615be8241d7fbf86ce555c72cc2a4 Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
Date: Fri, 27 Jan 2023 11:00:52 -0800
Subject: [PATCH] cxl/mbox: Indicate internal vs user enabled for command debug

At initial enumeration provide an indication of whether the kernel will
use an opcode internally, and / or make the functionality available via
user ioctl command.

Given that 'enum cxl_opcode' needs to be updated before the kernel can
use a CXL opcode internally, use that list to populate known_opcode().

For the QEMU CXL emulation this results in the following debug messages
at startup:

 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x100 Enabled: 0 User: none
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x101 Enabled: 0 User: none
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x102 Enabled: 0 User: none
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x103 Enabled: 0 User: none
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x200 Enabled: 1 User: Get FW Info
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x300 Enabled: 0 User: none
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x301 Enabled: 0 User: none
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x400 Enabled: 1 User: Get Supported Logs
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x401 Enabled: 1 User: Get Log
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4000 Enabled: 1 User: Identify Command
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4100 Enabled: 1 User: Get Partition Information
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4102 Enabled: 1 User: Get Label Storage Area
 cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4103 Enabled: 1 User: Set Label Storage Area

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/mbox.c | 48 ++++++++++++++++++++++++++++++++++-------
 drivers/cxl/cxlmem.h    |  1 +
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index cae43cea00cd..45206465fd4d 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -587,6 +587,40 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
 	return 0;
 }
 
+static bool known_opcode(u16 opcode)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_GET_FW_INFO:
+	case CXL_MBOX_OP_ACTIVATE_FW:
+	case CXL_MBOX_OP_GET_SUPPORTED_LOGS:
+	case CXL_MBOX_OP_GET_LOG:
+	case CXL_MBOX_OP_IDENTIFY:
+	case CXL_MBOX_OP_GET_PARTITION_INFO:
+	case CXL_MBOX_OP_SET_PARTITION_INFO:
+	case CXL_MBOX_OP_GET_LSA:
+	case CXL_MBOX_OP_SET_LSA:
+	case CXL_MBOX_OP_GET_HEALTH_INFO:
+	case CXL_MBOX_OP_GET_ALERT_CONFIG:
+	case CXL_MBOX_OP_SET_ALERT_CONFIG:
+	case CXL_MBOX_OP_GET_SHUTDOWN_STATE:
+	case CXL_MBOX_OP_SET_SHUTDOWN_STATE:
+	case CXL_MBOX_OP_GET_POISON:
+	case CXL_MBOX_OP_INJECT_POISON:
+	case CXL_MBOX_OP_CLEAR_POISON:
+	case CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS:
+	case CXL_MBOX_OP_SCAN_MEDIA:
+	case CXL_MBOX_OP_GET_SCAN_MEDIA:
+	case CXL_MBOX_OP_GET_SECURITY_STATE:
+	case CXL_MBOX_OP_SET_PASSPHRASE:
+	case CXL_MBOX_OP_DISABLE_PASSPHRASE:
+	case CXL_MBOX_OP_UNLOCK:
+	case CXL_MBOX_OP_FREEZE_SECURITY:
+	case CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE:
+		return true;
+	}
+	return false;
+}
+
 /**
  * cxl_walk_cel() - Walk through the Command Effects Log.
  * @cxlds: The device data for the operation
@@ -607,15 +641,13 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
 	for (i = 0; i < cel_entries; i++) {
 		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
 		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
+		bool known = known_opcode(opcode);
 
-		if (!cmd) {
-			dev_dbg(cxlds->dev,
-				"Opcode 0x%04x unsupported by driver\n", opcode);
-			continue;
-		}
-
-		set_bit(cmd->info.id, cxlds->enabled_cmds);
-		dev_dbg(cxlds->dev, "Opcode 0x%04x enabled\n", opcode);
+		if (cmd)
+			set_bit(cmd->info.id, cxlds->enabled_cmds);
+		dev_dbg(cxlds->dev, "Opcode %#04x Enabled: %d User: %s\n",
+			opcode, !!known,
+			cmd ? cxl_command_names[cmd->info.id].name : "none");
 	}
 }
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ab138004f644..21f97d631823 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -299,6 +299,7 @@ enum cxl_opcode {
 	CXL_MBOX_OP_FREEZE_SECURITY	= 0x4504,
 	CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE	= 0x4505,
 	CXL_MBOX_OP_MAX			= 0x10000
+	/* update known_opcode() when adding opcode support to this list */
 };
 
 #define DEFINE_CXL_CEL_UUID                                                    \
-- 
2.38.1