2020-12-09 00:30:02

by Ben Widawsky

[permalink] [raw]
Subject: [RFC PATCH 14/14] WIP/cxl/mem: Add get firmware for testing

This also serves as an example how to add a new command

Signed-off-by: Ben Widawsky <[email protected]>
---
drivers/cxl/mem.c | 22 ++++++++++++++++++++++
include/uapi/linux/cxl_mem.h | 3 ++-
2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 6b2f8d3776b5..76aa1e6e4117 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -116,6 +116,7 @@ struct cxl_mem_command {
static struct cxl_mem_command mem_commands[] = {
CXL_CMD(INVALID, NONE, 0, 0, "Reserved", false, 0),
CXL_CMD(RAW, TAINT, ~0, ~0, "Raw", true, 0),
+ CXL_CMD(GET_FW_INFO, NONE, 0, 0x50, "Get FW Info", false, 0x0200),
};

static int cxl_mem_wait_for_doorbell(struct cxl_mem *cxlm)
@@ -827,6 +828,23 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
return cxl_register(dev);
}

+static int cxl_mem_enable_commands(struct cxl_mem *cxlm)
+{
+ struct cxl_mem_command *c;
+
+ /*
+ * For now we pretend Get FW info is supported.
+ *
+ * FIXME: Invoke GET LOG to get the Command Effect Logs (CEL).
+ */
+ c = cxl_mem_find_command(0x200);
+ if (!c)
+ return -ENOENT;
+
+ c->enable = true;
+ return 0;
+}
+
/**
* cxl_mem_identify() - Send the IDENTIFY command to the device.
* @cxlm: The device to identify.
@@ -936,6 +954,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (rc)
return rc;

+ rc = cxl_mem_enable_commands(cxlm);
+ if (rc)
+ return rc;
+
rc = cxl_mem_identify(cxlm);
if (rc)
return rc;
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index f2fbb0dcda06..3ac39acf8fa7 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -50,7 +50,8 @@ struct cxl_command_info {
__u32 id;
#define CXL_MEM_COMMAND_ID_INVALID 0
#define CXL_MEM_COMMAND_ID_RAW 1
-#define CXL_MEM_COMMAND_ID_MAX (CXL_MEM_COMMAND_ID_RAW + 1)
+#define CXL_MEM_COMMAND_ID_GET_FW_INFO 2
+#define CXL_MEM_COMMAND_ID_MAX (CXL_MEM_COMMAND_ID_GET_FW_INFO + 1)

__u32 flags;
#define CXL_MEM_COMMAND_FLAG_NONE 0
--
2.29.2


2020-12-10 00:46:50

by Dan Williams

[permalink] [raw]
Subject: Re: [RFC PATCH 14/14] WIP/cxl/mem: Add get firmware for testing

On Tue, Dec 8, 2020 at 4:25 PM Ben Widawsky <[email protected]> wrote:
>
> This also serves as an example how to add a new command
>
> Signed-off-by: Ben Widawsky <[email protected]>
> ---
> drivers/cxl/mem.c | 22 ++++++++++++++++++++++
> include/uapi/linux/cxl_mem.h | 3 ++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 6b2f8d3776b5..76aa1e6e4117 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -116,6 +116,7 @@ struct cxl_mem_command {
> static struct cxl_mem_command mem_commands[] = {
> CXL_CMD(INVALID, NONE, 0, 0, "Reserved", false, 0),
> CXL_CMD(RAW, TAINT, ~0, ~0, "Raw", true, 0),
> + CXL_CMD(GET_FW_INFO, NONE, 0, 0x50, "Get FW Info", false, 0x0200),

I think CXL_CMD() arguments can be put on a diet if the
mem-command-id-to-name was moved to its own table, and the opcode was
defined as something like:

#define CXL_MBOX_OP_GET_FW_INFO 0x200

...so that CXL_CMD can just do:

.opcode = CXL_MBOX_OP_##_id

That @_enable arg wants a flag #define like CMD_ENABLE which reads
better than 'true'. I would then add CMD_KERNEL_EXCL alongside that
flag to indicate the commands that may be exclusive to the kernel when
the device is active in a PMEM memory region, or ones that have an
alternate ABI wrapped around them like the keys subsystem for security
or the firwmare activation sysfs interface.

> };
>
> static int cxl_mem_wait_for_doorbell(struct cxl_mem *cxlm)
> @@ -827,6 +828,23 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
> return cxl_register(dev);
> }
>
> +static int cxl_mem_enable_commands(struct cxl_mem *cxlm)
> +{
> + struct cxl_mem_command *c;
> +
> + /*
> + * For now we pretend Get FW info is supported.
> + *
> + * FIXME: Invoke GET LOG to get the Command Effect Logs (CEL).
> + */
> + c = cxl_mem_find_command(0x200);
> + if (!c)
> + return -ENOENT;
> +
> + c->enable = true;
> + return 0;
> +}
> +
> /**
> * cxl_mem_identify() - Send the IDENTIFY command to the device.
> * @cxlm: The device to identify.
> @@ -936,6 +954,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> if (rc)
> return rc;
>
> + rc = cxl_mem_enable_commands(cxlm);
> + if (rc)
> + return rc;
> +
> rc = cxl_mem_identify(cxlm);
> if (rc)
> return rc;
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index f2fbb0dcda06..3ac39acf8fa7 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -50,7 +50,8 @@ struct cxl_command_info {
> __u32 id;
> #define CXL_MEM_COMMAND_ID_INVALID 0
> #define CXL_MEM_COMMAND_ID_RAW 1
> -#define CXL_MEM_COMMAND_ID_MAX (CXL_MEM_COMMAND_ID_RAW + 1)
> +#define CXL_MEM_COMMAND_ID_GET_FW_INFO 2
> +#define CXL_MEM_COMMAND_ID_MAX (CXL_MEM_COMMAND_ID_GET_FW_INFO + 1)

Seems like CXL_MEM_COMMAND_ID definitions want to be an enum so that
CXL_MEM_COMMAND_ID_MAX does not need to be touched every time.

>
> __u32 flags;
> #define CXL_MEM_COMMAND_FLAG_NONE 0
> --
> 2.29.2
>