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

[Ira Weiny <ira.weiny@intel.com>]

Dan Williams wrote:
> 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

I think this is nice debug output.

This kind of threw me though.  It looks like all enabled commands have
user commands.

Would it be better to s/Enabled/Kernel.  Or perhaps driver?

Should we also check anything that is exclusive to the kernel and report
that?

FWIW I've got a patch warming which updates the query command to factor in
the kernel exclusive and hardware support, in case anyone is looking at
this debug output as a way to determine that.

> 
> 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)

Perhaps driver_known_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 */

I'm wishing there was a way to not have to do this.  All I can think of to
make it easier would be to make 'driver_know_opcode()' static inline and
in this header.  I'm sure some macro magic could be used too.  Although
I'm not always a fan.

Ira