[PATCH v2 0/4] cxl: Add a firmware update mechanism and cxl_test emulation

[PATCH v2 0/4] cxl: Add a firmware update mechanism and cxl_test emulation

[Vishal Verma]

Add firmware update capability to the CXL memdev driver, and emulation
in cxl_test. Since the 'Transfer FW' mailbox command is a background
command, this series depends on background command support [1].

Since Transfer FW can be a long-running background command, it is
desirable to retain kernel control over it, so that one command doesn't
monopolize the mailbox interface. The sysfs based firmware loader
mechanism that was developed for FPGAs is a suitable candidate to help
accomplish this. Patch 1 goes into more detail on this.

The poll interval for the Transfer FW command is arbitrarily set at 1
second, and a poll count of 30, giving us a total wait time of thirty
seconds before which each slice of the transfer times out. This seems
like a good mix of responsiveness and a total wait - the spec doesn't
have any guidance on any upper or lower bounds for this. This likely
does not need to be user-configurable, so for now it is just hard-coded
in the driver.

Patch 2 is a fix for the Poison commands effect population found while
developing these.

Patch 3 is a cleanup to give names to command effects.

Patch 4 implements the emulation of firmware update related commands in
the cxl_test environment to enable unit testing.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-6.5/cxl-background

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
Changes in v2:
- Link to v1: https://lore.kernel.org/r/20230421-vv-fw_update-v1-0-22468747d72f@intel.com
- Make command field names more consistent with the spec (Jonathan)
- Remove pointers to info/activate/transfer from fw state (Jonathan)
- Use struct_size() insted of open coding it (Jonathan)
- Fix a memory leak with 'transfer' in cxl_fw_write() (Jonathan)
- Move fw setup/teardown into a devm action (Jonathan, Dan)
- Use decimals in command struct definitions to match the spec (Alison)
- Move fw loader registration up into cxl_pci_probe() (Dan)
- Remove the fw_name cargo cult (Dan)
- Change timeout for each transfer chunk to 30s (Dan)
- Add a timeout in cxl_fw_write() as well (not just in abort)
- Use IS_ALIGNED() instead of open coding it (Dan)
- Clean up cur_size calculation in ->write() (Dan)
- Remove the lock and 'clear_to_send (Dan)
- Use atomic bitops for the cancel flag (Dan)
- Gate fw loader setup on command availability (Dan)
- Clean up the effects list in cxl_test using defines (Jonathan)
- Use the sha256 routine in include/crypto/sha2.h (Jonathan)

---
Vishal Verma (4):
      cxl: add a firmware update mechanism using the sysfs firmware loader
      tools/testing/cxl: Fix command effects for inject/clear poison
      tools/testing/cxl: Use named effects for the Command Effect Log
      tools/testing/cxl: add firmware update emulation to CXL memdevs

 drivers/cxl/cxlmem.h                    |  85 +++++++++
 drivers/cxl/core/memdev.c               | 309 +++++++++++++++++++++++++++++++-
 drivers/cxl/pci.c                       |   4 +
 tools/testing/cxl/test/mem.c            | 194 +++++++++++++++++++-
 Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
 drivers/cxl/Kconfig                     |   1 +
 6 files changed, 594 insertions(+), 10 deletions(-)
---
base-commit: ccadf1310fb0bc8d2cbcd14f94a6279c12ea9bee
change-id: 20230602-vv-fw_update-e1c96a60c687

Best regards,
-- 
Vishal Verma <vishal.l.verma@intel.com>

[PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Vishal Verma]

The sysfs based firmware loader mechanism was created to easily allow
userspace to upload firmware images to FPGA cards. This also happens to
be pretty suitable to create a user-initiated but kernel-controlled
firmware update mechanism for CXL devices, using the CXL specified
mailbox commands.

Since firmware update commands can be long-running, and can be processed
in the background by the endpoint device, it is desirable to have the
ability to chunk the firmware transfer down to smaller pieces, so that
one operation does not monopolize the mailbox, locking out any other
long running background commands entirely - e.g. security commands like
'sanitize' or poison scanning operations.

The firmware loader mechanism allows a natural way to perform this
chunking, as after each mailbox command, that is restricted to the
maximum mailbox payload size, the cxl memdev driver relinquishes control
back to the fw_loader system and awaits the next chunk of data to
transfer. This opens opportunities for other background commands to
access the mailbox and send their own slices of background commands.

Add the necessary helpers and state tracking to be able to perform the
'Get FW Info', 'Transfer FW', and 'Activate FW' mailbox commands as
described in the CXL spec. Wire these up to the firmware loader
callbacks, and register with that system to create the memX/firmware/
sysfs ABI.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Russ Weight <russell.h.weight@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Ben Widawsky <bwidawsk@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/cxl/cxlmem.h                    |  85 +++++++++
 drivers/cxl/core/memdev.c               | 309 +++++++++++++++++++++++++++++++-
 drivers/cxl/pci.c                       |   4 +
 Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
 drivers/cxl/Kconfig                     |   1 +
 5 files changed, 409 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 1d8e81c87c6a..835b544812bc 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -49,6 +49,7 @@ struct cxl_memdev {
 	struct work_struct detach_work;
 	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_nvdimm *cxl_nvd;
+	const char *fw_name;
 	int id;
 	int depth;
 };
@@ -83,6 +84,7 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
 }
 
 struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
+int cxl_memdev_setup_fw_upload(struct cxl_dev_state *cxlds);
 int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 			 resource_size_t base, resource_size_t len,
 			 resource_size_t skipped);
@@ -260,6 +262,84 @@ struct cxl_poison_state {
 	struct mutex lock;  /* Protect reads of poison list */
 };
 
+/*
+ * Get FW Info
+ * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
+ */
+struct cxl_mbox_get_fw_info {
+	u8 num_slots;
+	u8 slot_info;
+	u8 activation_cap;
+	u8 reserved[13];
+	char slot_1_revision[16];
+	char slot_2_revision[16];
+	char slot_3_revision[16];
+	char slot_4_revision[16];
+} __packed;
+
+#define CXL_FW_INFO_SLOT_INFO_CUR_MASK			GENMASK(2, 0)
+#define CXL_FW_INFO_SLOT_INFO_NEXT_MASK			GENMASK(5, 3)
+#define CXL_FW_INFO_SLOT_INFO_NEXT_SHIFT		3
+#define CXL_FW_INFO_ACTIVATION_CAP_HAS_LIVE_ACTIVATE	BIT(0)
+
+/*
+ * Transfer FW Input Payload
+ * CXL rev 3.0 section 8.2.9.3.2; Table 8-57
+ */
+struct cxl_mbox_transfer_fw {
+	u8 action;
+	u8 slot;
+	u8 reserved[2];
+	__le32 offset;
+	u8 reserved2[0x78];
+	u8 data[];
+} __packed;
+
+#define CXL_FW_TRANSFER_ACTION_FULL	0x0
+#define CXL_FW_TRANSFER_ACTION_INITIATE	0x1
+#define CXL_FW_TRANSFER_ACTION_CONTINUE	0x2
+#define CXL_FW_TRANSFER_ACTION_END	0x3
+#define CXL_FW_TRANSFER_ACTION_ABORT	0x4
+
+/*
+ * CXL rev 3.0 section 8.2.9.3.2 mandates 128-byte alignment for FW packages
+ * and for each part transferred in a Transfer FW command.
+ */
+#define CXL_FW_TRANSFER_ALIGNMENT	128
+
+/*
+ * Activate FW Input Payload
+ * CXL rev 3.0 section 8.2.9.3.3; Table 8-58
+ */
+struct cxl_mbox_activate_fw {
+	u8 action;
+	u8 slot;
+} __packed;
+
+#define CXL_FW_ACTIVATE_ONLINE		0x0
+#define CXL_FW_ACTIVATE_OFFLINE		0x1
+
+/* FW state bits */
+#define CXL_FW_STATE_BITS		32
+#define CXL_FW_CANCEL		BIT(0)
+
+/**
+ * struct cxl_fw_state - Firmware upload / activation state
+ *
+ * @state: fw_uploader state bitmask
+ * @oneshot: whether the fw upload fits in a single transfer
+ * @num_slots: Number of FW slots available
+ * @cur_slot: Slot number currently active
+ * @next_slot: Slot number for the new firmware
+ */
+struct cxl_fw_state {
+	DECLARE_BITMAP(state, CXL_FW_STATE_BITS);
+	bool oneshot;
+	int num_slots;
+	int cur_slot;
+	int next_slot;
+};
+
 /**
  * struct cxl_dev_state - The driver device state
  *
@@ -297,6 +377,8 @@ struct cxl_poison_state {
  * @serial: PCIe Device Serial Number
  * @event: event log driver state
  * @poison: poison driver state info
+ * @fw: firmware upload / activation state
+ * @fwl: handle for registration with the firmware loader system
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
@@ -336,6 +418,8 @@ struct cxl_dev_state {
 
 	struct cxl_event_state event;
 	struct cxl_poison_state poison;
+	struct cxl_fw_state fw;
+	struct fw_upload *fwl;
 
 	struct rcuwait mbox_wait;
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
@@ -349,6 +433,7 @@ enum cxl_opcode {
 	CXL_MBOX_OP_GET_EVT_INT_POLICY	= 0x0102,
 	CXL_MBOX_OP_SET_EVT_INT_POLICY	= 0x0103,
 	CXL_MBOX_OP_GET_FW_INFO		= 0x0200,
+	CXL_MBOX_OP_TRANSFER_FW		= 0x0201,
 	CXL_MBOX_OP_ACTIVATE_FW		= 0x0202,
 	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
 	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 057a43267290..f45c8b174d9d 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2020 Intel Corporation. */
 
+#include <linux/firmware.h>
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
@@ -441,6 +442,7 @@ static void cxl_memdev_unregister(void *_cxlmd)
 	struct cxl_memdev *cxlmd = _cxlmd;
 	struct device *dev = &cxlmd->dev;
 
+	kfree(cxlmd->fw_name);
 	cxl_memdev_shutdown(dev);
 	cdev_device_del(&cxlmd->cdev, dev);
 	put_device(dev);
@@ -542,6 +544,311 @@ static int cxl_memdev_release_file(struct inode *inode, struct file *file)
 	return 0;
 }
 
+/**
+ * cxl_mem_get_fw_info - Get Firmware info
+ * @cxlds: The device data for the operation
+ *
+ * Retrieve firmware info for the device specified.
+ *
+ * Return: 0 if no error: or the result of the mailbox command.
+ *
+ * See CXL-3.0 8.2.9.3.1 Get FW Info
+ */
+static int cxl_mem_get_fw_info(struct cxl_dev_state *cxlds)
+{
+	struct cxl_mbox_get_fw_info info;
+	struct cxl_mbox_cmd mbox_cmd;
+	int rc;
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_FW_INFO,
+		.size_out = sizeof(info),
+		.payload_out = &info,
+	};
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
+	if (rc < 0)
+		return rc;
+
+	cxlds->fw.num_slots = info.num_slots;
+	cxlds->fw.cur_slot = FIELD_GET(CXL_FW_INFO_SLOT_INFO_CUR_MASK,
+				       info.slot_info);
+
+	return 0;
+}
+
+/**
+ * cxl_mem_activate_fw - Activate Firmware
+ * @cxlds: The device data for the operation
+ * @slot: slot number to activate
+ *
+ * Activate firmware in a given slot for the device specified.
+ *
+ * Return: 0 if no error: or the result of the mailbox command.
+ *
+ * See CXL-3.0 8.2.9.3.3 Activate FW
+ */
+static int cxl_mem_activate_fw(struct cxl_dev_state *cxlds, int slot)
+{
+	struct cxl_mbox_activate_fw activate;
+	struct cxl_mbox_cmd mbox_cmd;
+
+	if (slot == 0 || slot > cxlds->fw.num_slots)
+		return -EINVAL;
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_ACTIVATE_FW,
+		.size_in = sizeof(activate),
+		.payload_in = &activate,
+	};
+
+	/* Only offline activation supported for now */
+	activate.action = CXL_FW_ACTIVATE_OFFLINE;
+	activate.slot = slot;
+
+	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
+}
+
+/**
+ * cxl_mem_abort_fw_xfer - Abort an in-progress FW transfer
+ * @cxlds: The device data for the operation
+ *
+ * Abort an in-progress firmware transfer for the device specified.
+ *
+ * Return: 0 if no error: or the result of the mailbox command.
+ *
+ * See CXL-3.0 8.2.9.3.2 Transfer FW
+ */
+static int cxl_mem_abort_fw_xfer(struct cxl_dev_state *cxlds)
+{
+	struct cxl_mbox_transfer_fw *transfer;
+	struct cxl_mbox_cmd mbox_cmd;
+	int rc;
+
+	transfer = kzalloc(struct_size(transfer, data, 0), GFP_KERNEL);
+	if (!transfer)
+		return -ENOMEM;
+
+	/* Set a 1s poll interval and a total wait time of 30s */
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_TRANSFER_FW,
+		.size_in = sizeof(*transfer),
+		.payload_in = transfer,
+		.poll_interval_ms = 1000,
+		.poll_count = 30,
+	};
+
+	transfer->action = CXL_FW_TRANSFER_ACTION_ABORT;
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
+	kfree(transfer);
+	return rc;
+}
+
+static void cxl_fw_cleanup(struct fw_upload *fwl)
+{
+	struct cxl_dev_state *cxlds = fwl->dd_handle;
+
+	cxlds->fw.next_slot = 0;
+}
+
+static int cxl_fw_do_cancel(struct fw_upload *fwl)
+{
+	struct cxl_dev_state *cxlds = fwl->dd_handle;
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	int rc;
+
+	rc = cxl_mem_abort_fw_xfer(cxlds);
+	if (rc < 0)
+		dev_err(&cxlmd->dev, "Error aborting FW transfer: %d\n", rc);
+
+	return FW_UPLOAD_ERR_CANCELED;
+}
+
+static enum fw_upload_err cxl_fw_prepare(struct fw_upload *fwl, const u8 *data,
+					 u32 size)
+{
+	struct cxl_dev_state *cxlds = fwl->dd_handle;
+	struct cxl_mbox_transfer_fw *transfer;
+
+	if (!size)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	cxlds->fw.oneshot = struct_size(transfer, data, size) <
+			    cxlds->payload_size;
+
+	if (cxl_mem_get_fw_info(cxlds))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	/*
+	 * So far no state has been changed, hence no other cleanup is
+	 * necessary. Simply return the cancelled status.
+	 */
+	if (test_and_clear_bit(CXL_FW_CANCEL, cxlds->fw.state))
+		return FW_UPLOAD_ERR_CANCELED;
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
+				       u32 offset, u32 size, u32 *written)
+{
+	struct cxl_dev_state *cxlds = fwl->dd_handle;
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	struct cxl_mbox_transfer_fw *transfer;
+	struct cxl_mbox_cmd mbox_cmd;
+	u32 cur_size, remaining;
+	size_t size_in;
+	int rc;
+
+	*written = 0;
+
+	/* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
+	if (!IS_ALIGNED(offset, CXL_FW_TRANSFER_ALIGNMENT)) {
+		dev_err(&cxlmd->dev,
+			"misaligned offset for FW transfer slice (%u)\n",
+			offset);
+		return FW_UPLOAD_ERR_RW_ERROR;
+	}
+
+	/* Pick transfer size based on cxlds->payload_size */
+	cur_size = min_t(size_t, size, cxlds->payload_size - sizeof(*transfer));
+	remaining = size - cur_size;
+	size_in = struct_size(transfer, data, cur_size);
+
+	if (test_and_clear_bit(CXL_FW_CANCEL, cxlds->fw.state))
+		return cxl_fw_do_cancel(fwl);
+
+	/*
+	 * Slot numbers are 1-indexed
+	 * cur_slot is the 0-indexed next_slot (i.e. 'cur_slot - 1 + 1')
+	 * Check for rollover using modulo, and 1-index it by adding 1
+	 */
+	cxlds->fw.next_slot = (cxlds->fw.cur_slot % cxlds->fw.num_slots) + 1;
+
+	/* Do the transfer via mailbox cmd */
+	transfer = kzalloc(size_in, GFP_KERNEL);
+	if (!transfer)
+		return FW_UPLOAD_ERR_RW_ERROR;
+
+	transfer->offset = cpu_to_le32(offset / CXL_FW_TRANSFER_ALIGNMENT);
+	memcpy(transfer->data, data + offset, cur_size);
+	if (cxlds->fw.oneshot) {
+		transfer->action = CXL_FW_TRANSFER_ACTION_FULL;
+		transfer->slot = cxlds->fw.next_slot;
+	} else {
+		if (offset == 0) {
+			transfer->action = CXL_FW_TRANSFER_ACTION_INITIATE;
+		} else if (remaining == 0) {
+			transfer->action = CXL_FW_TRANSFER_ACTION_END;
+			transfer->slot = cxlds->fw.next_slot;
+		} else {
+			transfer->action = CXL_FW_TRANSFER_ACTION_CONTINUE;
+		}
+	}
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_TRANSFER_FW,
+		.size_in = size_in,
+		.payload_in = transfer,
+		.poll_interval_ms = 1000,
+		.poll_count = 30,
+	};
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
+	if (rc < 0) {
+		kfree(transfer);
+		rc = FW_UPLOAD_ERR_RW_ERROR;
+		goto out_free;
+	}
+
+	*written = cur_size;
+
+	/* Activate FW if oneshot or if the last slice was written */
+	if (cxlds->fw.oneshot || remaining == 0) {
+		dev_dbg(&cxlmd->dev, "Activating firmware slot: %d\n",
+			cxlds->fw.next_slot);
+		rc = cxl_mem_activate_fw(cxlds, cxlds->fw.next_slot);
+		if (rc < 0) {
+			dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
+				rc);
+			rc = FW_UPLOAD_ERR_HW_ERROR;
+			goto out_free;
+		}
+	}
+
+	rc = FW_UPLOAD_ERR_NONE;
+
+out_free:
+	kfree(transfer);
+	return rc;
+}
+
+static enum fw_upload_err cxl_fw_poll_complete(struct fw_upload *fwl)
+{
+	struct cxl_dev_state *cxlds = fwl->dd_handle;
+
+	/*
+	 * cxl_internal_send_cmd() handles background operations synchronously.
+	 * No need to wait for completions here - any errors would've been
+	 * reported and handled during the ->write() call(s).
+	 * Just check if a cancel request was received, and return success.
+	 */
+	if (test_and_clear_bit(CXL_FW_CANCEL, cxlds->fw.state))
+		return cxl_fw_do_cancel(fwl);
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void cxl_fw_cancel(struct fw_upload *fwl)
+{
+	struct cxl_dev_state *cxlds = fwl->dd_handle;
+
+	set_bit(CXL_FW_CANCEL, cxlds->fw.state);
+}
+
+static const struct fw_upload_ops cxl_memdev_fw_ops = {
+        .prepare = cxl_fw_prepare,
+        .write = cxl_fw_write,
+        .poll_complete = cxl_fw_poll_complete,
+        .cancel = cxl_fw_cancel,
+        .cleanup = cxl_fw_cleanup,
+};
+
+static void devm_cxl_remove_fw_upload(void *fwl)
+{
+	firmware_upload_unregister(fwl);
+}
+
+int cxl_memdev_setup_fw_upload(struct cxl_dev_state *cxlds)
+{
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	struct fw_upload *fwl;
+	int rc;
+
+	if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, cxlds->enabled_cmds))
+		return 0;
+
+	fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev,
+				       dev_name(&cxlmd->dev),
+				       &cxl_memdev_fw_ops, cxlds);
+	if (IS_ERR(fwl)) {
+		dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
+		return PTR_ERR(fwl);
+	}
+
+	cxlds->fwl = fwl;
+	rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload,
+				      cxlds->fwl);
+	if (rc)
+		dev_err(&cxlmd->dev,
+			"Failed to add firmware loader remove action: %d\n",
+			rc);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
+
 static const struct file_operations cxl_memdev_fops = {
 	.owner = THIS_MODULE,
 	.unlocked_ioctl = cxl_memdev_ioctl,
@@ -581,7 +888,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
 
 	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
 	if (rc)
-		return ERR_PTR(rc);
+		goto err;
 	return cxlmd;
 
 err:
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index a78e40e6d0e0..ef0b4821b312 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -842,6 +842,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
+	rc = cxl_memdev_setup_fw_upload(cxlds);
+	if (rc)
+		return rc;
+
 	rc = cxl_event_config(host_bridge, cxlds);
 	if (rc)
 		return rc;
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 48ac0d911801..06a7718d3fc3 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -58,6 +58,17 @@ Description:
 		affinity for this device.
 
 
+What:		/sys/bus/cxl/devices/memX/firmware/
+Date:		April, 2023
+KernelVersion:	v6.5
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RW) Firmware uploader mechanism. The different files under
+		this directory can be used to upload and activate new
+		firmware for CXL devices. The interfaces under this are
+		documented in sysfs-class-firmware.
+
+
 What:		/sys/bus/cxl/devices/*/devtype
 Date:		June, 2021
 KernelVersion:	v5.14
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index ff4e78117b31..80d8e35fa049 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -82,6 +82,7 @@ config CXL_PMEM
 config CXL_MEM
 	tristate "CXL: Memory Expansion"
 	depends on CXL_PCI
+	select FW_UPLOAD
 	default CXL_BUS
 	help
 	  The CXL.mem protocol allows a device to act as a provider of "System

-- 
2.40.1

[PATCH v2 2/4] tools/testing/cxl: Fix command effects for inject/clear poison

[Vishal Verma]

The CXL spec (3.0, section 8.2.9.8.4) Lists Inject Poison and Clear
Poison as having the effects of "Immediate Data Change". Fix this in the
mock driver so that the command effect log is populated correctly.

Fixes: 371c16101ee8 ("tools/testing/cxl: Mock the Inject Poison mailbox command")
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 tools/testing/cxl/test/mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 34b48027b3de..403cd3608772 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -52,11 +52,11 @@ static struct cxl_cel_entry mock_cel[] = {
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_INJECT_POISON),
-		.effect = cpu_to_le16(0),
+		.effect = cpu_to_le16(EFFECT(2)),
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_CLEAR_POISON),
-		.effect = cpu_to_le16(0),
+		.effect = cpu_to_le16(EFFECT(2)),
 	},
 };
 

-- 
2.40.1

[PATCH v2 3/4] tools/testing/cxl: Use named effects for the Command Effect Log

[Vishal Verma]

As more emulated mailbox commands are added to cxl_test, it is a pain
point to look up command effect numbers for each effect. Replace the
bare numbers in the mock driver with an enum that lists all possible
effects.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Russ Weight <russell.h.weight@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Ben Widawsky <bwidawsk@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 tools/testing/cxl/test/mem.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 403cd3608772..68668d8df1cd 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -21,42 +21,56 @@
 
 static unsigned int poison_inject_dev_max = MOCK_INJECT_DEV_MAX;
 
+enum cxl_command_effects {
+	CONF_CHANGE_COLD_RESET = 0,
+	CONF_CHANGE_IMMEDIATE,
+	DATA_CHANGE_IMMEDIATE,
+	POLICY_CHANGE_IMMEDIATE,
+	LOG_CHANGE_IMMEDIATE,
+	SECURITY_CHANGE_IMMEDIATE,
+	BACKGROUND_OP,
+	SECONDARY_MBOX_SUPPORTED,
+};
+
+#define CXL_CMD_EFFECT_NONE cpu_to_le16(0)
+
 static struct cxl_cel_entry mock_cel[] = {
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_SUPPORTED_LOGS),
-		.effect = cpu_to_le16(0),
+		.effect = CXL_CMD_EFFECT_NONE,
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_IDENTIFY),
-		.effect = cpu_to_le16(0),
+		.effect = CXL_CMD_EFFECT_NONE,
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_LSA),
-		.effect = cpu_to_le16(0),
+		.effect = CXL_CMD_EFFECT_NONE,
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_PARTITION_INFO),
-		.effect = cpu_to_le16(0),
+		.effect = CXL_CMD_EFFECT_NONE,
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_SET_LSA),
-		.effect = cpu_to_le16(EFFECT(1) | EFFECT(2)),
+		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_IMMEDIATE) |
+				      EFFECT(DATA_CHANGE_IMMEDIATE)),
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
-		.effect = cpu_to_le16(0),
+		.effect = CXL_CMD_EFFECT_NONE,
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_POISON),
-		.effect = cpu_to_le16(0),
+		.effect = CXL_CMD_EFFECT_NONE,
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_INJECT_POISON),
-		.effect = cpu_to_le16(EFFECT(2)),
+		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE)),
 	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_CLEAR_POISON),
-		.effect = cpu_to_le16(EFFECT(2)),
+		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE)),
 	},
 };
 

-- 
2.40.1

[PATCH v2 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Vishal Verma]

Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
CXL mailbox commands to the cxl_test emulated memdevs to enable
end-to-end unit testing of a firmware update flow. For now, only
advertise an 'offline activation' capability as that is all the CXL
memdev driver currently implements.

Add some canned values for the serial number fields, and create a
platform device sysfs knob to calculate the sha256sum of the firmware
image that was received, so a unit test can compare it with the original
file that was uploaded.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Russ Weight <russell.h.weight@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Ben Widawsky <bwidawsk@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 tools/testing/cxl/test/mem.c | 162 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 162 insertions(+)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 68668d8df1cd..70cedeac0a49 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -8,11 +8,14 @@
 #include <linux/sizes.h>
 #include <linux/bits.h>
 #include <asm/unaligned.h>
+#include <crypto/sha2.h>
 #include <cxlmem.h>
 
 #include "trace.h"
 
 #define LSA_SIZE SZ_128K
+#define FW_SIZE SZ_64M
+#define FW_SLOTS 3
 #define DEV_SIZE SZ_2G
 #define EFFECT(x) (1U << x)
 
@@ -72,6 +75,20 @@ static struct cxl_cel_entry mock_cel[] = {
 		.opcode = cpu_to_le16(CXL_MBOX_OP_CLEAR_POISON),
 		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE)),
 	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_FW_INFO),
+		.effect = CXL_CMD_EFFECT_NONE,
+	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_TRANSFER_FW),
+		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_COLD_RESET) |
+				      EFFECT(BACKGROUND_OP)),
+	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_ACTIVATE_FW),
+		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_COLD_RESET) |
+				      EFFECT(CONF_CHANGE_IMMEDIATE)),
+	},
 };
 
 /* See CXL 2.0 Table 181 Get Health Info Output Payload */
@@ -123,6 +140,10 @@ struct mock_event_store {
 
 struct cxl_mockmem_data {
 	void *lsa;
+	void *fw;
+	int fw_slot;
+	int fw_staged;
+	size_t fw_size;
 	u32 security_state;
 	u8 user_pass[NVDIMM_PASSPHRASE_LEN];
 	u8 master_pass[NVDIMM_PASSPHRASE_LEN];
@@ -1128,6 +1149,89 @@ static struct attribute *cxl_mock_mem_core_attrs[] = {
 };
 ATTRIBUTE_GROUPS(cxl_mock_mem_core);
 
+static int mock_fw_info(struct cxl_dev_state *cxlds,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
+	struct cxl_mbox_get_fw_info fw_info = {
+		.num_slots = FW_SLOTS,
+		.slot_info = (mdata->fw_slot & 0x7) |
+			     ((mdata->fw_staged & 0x7) << 3),
+		.activation_cap = 0,
+	};
+
+	strcpy(fw_info.slot_1_revision, "cxl_test_fw_001");
+	strcpy(fw_info.slot_2_revision, "cxl_test_fw_002");
+	strcpy(fw_info.slot_3_revision, "cxl_test_fw_003");
+	strcpy(fw_info.slot_4_revision, "");
+
+	if (cmd->size_out < sizeof(fw_info))
+		return -EINVAL;
+
+	memcpy(cmd->payload_out, &fw_info, sizeof(fw_info));
+	return 0;
+}
+
+static int mock_transfer_fw(struct cxl_dev_state *cxlds,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_transfer_fw *transfer = cmd->payload_in;
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
+	void *fw = mdata->fw;
+	size_t offset, length;
+
+	offset = le32_to_cpu(transfer->offset) * CXL_FW_TRANSFER_ALIGNMENT;
+	length = cmd->size_in - sizeof(*transfer);
+	if (offset + length > FW_SIZE)
+		return -EINVAL;
+
+	switch (transfer->action) {
+	case CXL_FW_TRANSFER_ACTION_FULL:
+		if (offset != 0)
+			return -EINVAL;
+		fallthrough;
+	case CXL_FW_TRANSFER_ACTION_END:
+		if (transfer->slot == 0 || transfer->slot > FW_SLOTS)
+			return -EINVAL;
+		mdata->fw_size = offset + length;
+		break;
+	case CXL_FW_TRANSFER_ACTION_INITIATE:
+	case CXL_FW_TRANSFER_ACTION_CONTINUE:
+		break;
+	case CXL_FW_TRANSFER_ACTION_ABORT:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	memcpy(fw + offset, transfer->data, length);
+	return 0;
+}
+
+static int mock_activate_fw(struct cxl_dev_state *cxlds,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_activate_fw *activate = cmd->payload_in;
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
+
+	if (activate->slot == 0 || activate->slot > FW_SLOTS)
+		return -EINVAL;
+
+	switch (activate->action) {
+	case CXL_FW_ACTIVATE_ONLINE:
+		mdata->fw_slot = activate->slot;
+		mdata->fw_staged = 0;
+		break;
+	case CXL_FW_ACTIVATE_OFFLINE:
+		mdata->fw_staged = activate->slot;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
 {
 	struct device *dev = cxlds->dev;
@@ -1194,6 +1298,15 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
 	case CXL_MBOX_OP_CLEAR_POISON:
 		rc = mock_clear_poison(cxlds, cmd);
 		break;
+	case CXL_MBOX_OP_GET_FW_INFO:
+		rc = mock_fw_info(cxlds, cmd);
+		break;
+	case CXL_MBOX_OP_TRANSFER_FW:
+		rc = mock_transfer_fw(cxlds, cmd);
+		break;
+	case CXL_MBOX_OP_ACTIVATE_FW:
+		rc = mock_activate_fw(cxlds, cmd);
+		break;
 	default:
 		break;
 	}
@@ -1209,6 +1322,11 @@ static void label_area_release(void *lsa)
 	vfree(lsa);
 }
 
+static void fw_buf_release(void *buf)
+{
+	vfree(buf);
+}
+
 static bool is_rcd(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -1241,10 +1359,19 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	mdata->lsa = vmalloc(LSA_SIZE);
 	if (!mdata->lsa)
 		return -ENOMEM;
+	mdata->fw = vmalloc(FW_SIZE);
+	if (!mdata->fw)
+		return -ENOMEM;
+	mdata->fw_slot = 2;
+
 	rc = devm_add_action_or_reset(dev, label_area_release, mdata->lsa);
 	if (rc)
 		return rc;
 
+	rc = devm_add_action_or_reset(dev, fw_buf_release, mdata->fw);
+	if (rc)
+		return rc;
+
 	cxlds = cxl_dev_state_create(dev);
 	if (IS_ERR(cxlds))
 		return PTR_ERR(cxlds);
@@ -1286,6 +1413,10 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
+	rc = cxl_memdev_setup_fw_upload(cxlds);
+	if (rc)
+		return rc;
+
 	cxl_mem_get_event_records(cxlds, CXLDEV_EVENT_STATUS_ALL);
 
 	return 0;
@@ -1324,9 +1455,40 @@ static ssize_t security_lock_store(struct device *dev, struct device_attribute *
 
 static DEVICE_ATTR_RW(security_lock);
 
+static ssize_t fw_buf_checksum_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
+	u8 hash[SHA256_DIGEST_SIZE];
+	unsigned char *hstr, *hptr;
+	struct sha256_state sctx;
+	ssize_t written = 0;
+	int i;
+
+	sha256_init(&sctx);
+	sha256_update(&sctx, mdata->fw, mdata->fw_size);
+	sha256_final(&sctx, hash);
+
+	hstr = kzalloc((SHA256_DIGEST_SIZE * 2) + 1, GFP_KERNEL);
+	if (!hstr)
+		return -ENOMEM;
+
+	hptr = hstr;
+	for (i = 0; i < SHA256_DIGEST_SIZE; i++)
+		hptr += sprintf(hptr, "%02x", hash[i]);
+
+	written = sysfs_emit(buf, "%s\n", hstr);
+
+	kfree(hstr);
+	return written;
+}
+
+static DEVICE_ATTR_RO(fw_buf_checksum);
+
 static struct attribute *cxl_mock_mem_attrs[] = {
 	&dev_attr_security_lock.attr,
 	&dev_attr_event_trigger.attr,
+	&dev_attr_fw_buf_checksum.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(cxl_mock_mem);

-- 
2.40.1

Re: [PATCH v2 2/4] tools/testing/cxl: Fix command effects for inject/clear poison

[Alison Schofield]

On Mon, Jun 05, 2023 at 02:20:23PM -0600, Vishal Verma wrote:
> The CXL spec (3.0, section 8.2.9.8.4) Lists Inject Poison and Clear
> Poison as having the effects of "Immediate Data Change". Fix this in the
> mock driver so that the command effect log is populated correctly.
> 
> Fixes: 371c16101ee8 ("tools/testing/cxl: Mock the Inject Poison mailbox command")
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Hi Vishal,
I took a look at this, wondering if we should promote it as a 6.4 fix.
I came up with a no. It has no user impact of inject/clear usage in the
mock driver environment.

Thanks for fixing!
Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> ---
>  tools/testing/cxl/test/mem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 34b48027b3de..403cd3608772 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -52,11 +52,11 @@ static struct cxl_cel_entry mock_cel[] = {
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_INJECT_POISON),
> -		.effect = cpu_to_le16(0),
> +		.effect = cpu_to_le16(EFFECT(2)),
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_CLEAR_POISON),
> -		.effect = cpu_to_le16(0),
> +		.effect = cpu_to_le16(EFFECT(2)),
>  	},
>  };
>  
> 
> -- 
> 2.40.1
> 

Re: [PATCH v2 3/4] tools/testing/cxl: Use named effects for the Command Effect Log

[Alison Schofield]

On Mon, Jun 05, 2023 at 02:20:24PM -0600, Vishal Verma wrote:
> As more emulated mailbox commands are added to cxl_test, it is a pain
> point to look up command effect numbers for each effect. Replace the
> bare numbers in the mock driver with an enum that lists all possible
> effects.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ben Widawsky <bwidawsk@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  tools/testing/cxl/test/mem.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 403cd3608772..68668d8df1cd 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -21,42 +21,56 @@
>  
>  static unsigned int poison_inject_dev_max = MOCK_INJECT_DEV_MAX;
>  
> +enum cxl_command_effects {
> +	CONF_CHANGE_COLD_RESET = 0,
> +	CONF_CHANGE_IMMEDIATE,
> +	DATA_CHANGE_IMMEDIATE,
> +	POLICY_CHANGE_IMMEDIATE,
> +	LOG_CHANGE_IMMEDIATE,
> +	SECURITY_CHANGE_IMMEDIATE,
> +	BACKGROUND_OP,
> +	SECONDARY_MBOX_SUPPORTED,
> +};
> +
> +#define CXL_CMD_EFFECT_NONE cpu_to_le16(0)
> +
>  static struct cxl_cel_entry mock_cel[] = {
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_SUPPORTED_LOGS),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_IDENTIFY),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_LSA),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_PARTITION_INFO),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_SET_LSA),
> -		.effect = cpu_to_le16(EFFECT(1) | EFFECT(2)),
> +		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_IMMEDIATE) |
> +				      EFFECT(DATA_CHANGE_IMMEDIATE)),
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_POISON),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_INJECT_POISON),
> -		.effect = cpu_to_le16(EFFECT(2)),
> +		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE)),
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_CLEAR_POISON),
> -		.effect = cpu_to_le16(EFFECT(2)),
> +		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE)),
>  	},
>  };
>  
> 
> -- 
> 2.40.1
> 

Re: [PATCH v2 2/4] tools/testing/cxl: Fix command effects for inject/clear poison

[Jonathan Cameron]

On Wed, 7 Jun 2023 12:18:55 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Mon, Jun 05, 2023 at 02:20:23PM -0600, Vishal Verma wrote:
> > The CXL spec (3.0, section 8.2.9.8.4) Lists Inject Poison and Clear
> > Poison as having the effects of "Immediate Data Change". Fix this in the
> > mock driver so that the command effect log is populated correctly.
> > 
> > Fixes: 371c16101ee8 ("tools/testing/cxl: Mock the Inject Poison mailbox command")
> > Cc: Alison Schofield <alison.schofield@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>  
> 
> Hi Vishal,
> I took a look at this, wondering if we should promote it as a 6.4 fix.
> I came up with a no. It has no user impact of inject/clear usage in the
> mock driver environment.
> 
> Thanks for fixing!
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> 
Agreed on patch and Alison's comment

FWIW

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> > ---
> >  tools/testing/cxl/test/mem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > index 34b48027b3de..403cd3608772 100644
> > --- a/tools/testing/cxl/test/mem.c
> > +++ b/tools/testing/cxl/test/mem.c
> > @@ -52,11 +52,11 @@ static struct cxl_cel_entry mock_cel[] = {
> >  	},
> >  	{
> >  		.opcode = cpu_to_le16(CXL_MBOX_OP_INJECT_POISON),
> > -		.effect = cpu_to_le16(0),
> > +		.effect = cpu_to_le16(EFFECT(2)),
> >  	},
> >  	{
> >  		.opcode = cpu_to_le16(CXL_MBOX_OP_CLEAR_POISON),
> > -		.effect = cpu_to_le16(0),
> > +		.effect = cpu_to_le16(EFFECT(2)),
> >  	},
> >  };
> >  
> > 
> > -- 
> > 2.40.1
> >   
> 

Re: [PATCH v2 3/4] tools/testing/cxl: Use named effects for the Command Effect Log

[Jonathan Cameron]

On Mon, 05 Jun 2023 14:20:24 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> As more emulated mailbox commands are added to cxl_test, it is a pain
> point to look up command effect numbers for each effect. Replace the
> bare numbers in the mock driver with an enum that lists all possible
> effects.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ben Widawsky <bwidawsk@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
LGTM 

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  tools/testing/cxl/test/mem.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 403cd3608772..68668d8df1cd 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -21,42 +21,56 @@
>  
>  static unsigned int poison_inject_dev_max = MOCK_INJECT_DEV_MAX;
>  
> +enum cxl_command_effects {
> +	CONF_CHANGE_COLD_RESET = 0,
> +	CONF_CHANGE_IMMEDIATE,
> +	DATA_CHANGE_IMMEDIATE,
> +	POLICY_CHANGE_IMMEDIATE,
> +	LOG_CHANGE_IMMEDIATE,
> +	SECURITY_CHANGE_IMMEDIATE,
> +	BACKGROUND_OP,
> +	SECONDARY_MBOX_SUPPORTED,
> +};
> +
> +#define CXL_CMD_EFFECT_NONE cpu_to_le16(0)
> +
>  static struct cxl_cel_entry mock_cel[] = {
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_SUPPORTED_LOGS),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_IDENTIFY),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_LSA),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_PARTITION_INFO),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_SET_LSA),
> -		.effect = cpu_to_le16(EFFECT(1) | EFFECT(2)),
> +		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_IMMEDIATE) |
> +				      EFFECT(DATA_CHANGE_IMMEDIATE)),
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_POISON),
> -		.effect = cpu_to_le16(0),
> +		.effect = CXL_CMD_EFFECT_NONE,
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_INJECT_POISON),
> -		.effect = cpu_to_le16(EFFECT(2)),
> +		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE)),
>  	},
>  	{
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_CLEAR_POISON),
> -		.effect = cpu_to_le16(EFFECT(2)),
> +		.effect = cpu_to_le16(EFFECT(DATA_CHANGE_IMMEDIATE)),
>  	},
>  };
>  
> 

Re: [PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Jonathan Cameron]

On Mon, 05 Jun 2023 14:20:22 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> The sysfs based firmware loader mechanism was created to easily allow
> userspace to upload firmware images to FPGA cards. This also happens to
> be pretty suitable to create a user-initiated but kernel-controlled
> firmware update mechanism for CXL devices, using the CXL specified
> mailbox commands.
> 
> Since firmware update commands can be long-running, and can be processed
> in the background by the endpoint device, it is desirable to have the
> ability to chunk the firmware transfer down to smaller pieces, so that
> one operation does not monopolize the mailbox, locking out any other
> long running background commands entirely - e.g. security commands like
> 'sanitize' or poison scanning operations.
> 
> The firmware loader mechanism allows a natural way to perform this
> chunking, as after each mailbox command, that is restricted to the
> maximum mailbox payload size, the cxl memdev driver relinquishes control
> back to the fw_loader system and awaits the next chunk of data to
> transfer. This opens opportunities for other background commands to
> access the mailbox and send their own slices of background commands.
> 
> Add the necessary helpers and state tracking to be able to perform the
> 'Get FW Info', 'Transfer FW', and 'Activate FW' mailbox commands as
> described in the CXL spec. Wire these up to the firmware loader
> callbacks, and register with that system to create the memX/firmware/
> sysfs ABI.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ben Widawsky <bwidawsk@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Hi Vishal,

Some comments inline

Jonathan

> ---
>  drivers/cxl/cxlmem.h                    |  85 +++++++++
>  drivers/cxl/core/memdev.c               | 309 +++++++++++++++++++++++++++++++-
>  drivers/cxl/pci.c                       |   4 +
>  Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
>  drivers/cxl/Kconfig                     |   1 +
>  5 files changed, 409 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 1d8e81c87c6a..835b544812bc 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -49,6 +49,7 @@ struct cxl_memdev {
>  	struct work_struct detach_work;
>  	struct cxl_nvdimm_bridge *cxl_nvb;
>  	struct cxl_nvdimm *cxl_nvd;
> +	const char *fw_name;
Left over from a refactoring?
Side note, structure has docs which are missing if this should be here.

>  	int id;
>  	int depth;
>  };
> @@ -83,6 +84,7 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  }
>  


> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 057a43267290..f45c8b174d9d 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. */
>  
> +#include <linux/firmware.h>
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/idr.h>
> @@ -441,6 +442,7 @@ static void cxl_memdev_unregister(void *_cxlmd)
>  	struct cxl_memdev *cxlmd = _cxlmd;
>  	struct device *dev = &cxlmd->dev;
>  
> +	kfree(cxlmd->fw_name);

Never allocated that I can spot.

>  	cxl_memdev_shutdown(dev);
>  	cdev_device_del(&cxlmd->cdev, dev);
>  	put_device(dev);
> @@ -542,6 +544,311 @@ static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
>


> +
> +static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
> +				       u32 offset, u32 size, u32 *written)
> +{
> +	struct cxl_dev_state *cxlds = fwl->dd_handle;
> +	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> +	struct cxl_mbox_transfer_fw *transfer;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	u32 cur_size, remaining;
> +	size_t size_in;
> +	int rc;
> +
> +	*written = 0;
> +
> +	/* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
> +	if (!IS_ALIGNED(offset, CXL_FW_TRANSFER_ALIGNMENT)) {
> +		dev_err(&cxlmd->dev,
> +			"misaligned offset for FW transfer slice (%u)\n",
> +			offset);
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	}
> +
> +	/* Pick transfer size based on cxlds->payload_size */
> +	cur_size = min_t(size_t, size, cxlds->payload_size - sizeof(*transfer));

If size > cxlds->payload_size - sizeof(*transfer) what ensures that the step
we take forwards results in the next read having an offset that is 128B aligned?

I think cur_size needs to be forced to be a multiple of 128Bytes as well.

> +	remaining = size - cur_size;
> +	size_in = struct_size(transfer, data, cur_size);
> +
> +	if (test_and_clear_bit(CXL_FW_CANCEL, cxlds->fw.state))
> +		return cxl_fw_do_cancel(fwl);
> +
> +	/*
> +	 * Slot numbers are 1-indexed
> +	 * cur_slot is the 0-indexed next_slot (i.e. 'cur_slot - 1 + 1')
> +	 * Check for rollover using modulo, and 1-index it by adding 1
> +	 */
> +	cxlds->fw.next_slot = (cxlds->fw.cur_slot % cxlds->fw.num_slots) + 1;
> +
> +	/* Do the transfer via mailbox cmd */
> +	transfer = kzalloc(size_in, GFP_KERNEL);
> +	if (!transfer)
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +
> +	transfer->offset = cpu_to_le32(offset / CXL_FW_TRANSFER_ALIGNMENT);
> +	memcpy(transfer->data, data + offset, cur_size);
> +	if (cxlds->fw.oneshot) {
> +		transfer->action = CXL_FW_TRANSFER_ACTION_FULL;
> +		transfer->slot = cxlds->fw.next_slot;
> +	} else {
> +		if (offset == 0) {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_INITIATE;
> +		} else if (remaining == 0) {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_END;
> +			transfer->slot = cxlds->fw.next_slot;
> +		} else {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_CONTINUE;
> +		}
> +	}
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_TRANSFER_FW,
> +		.size_in = size_in,
> +		.payload_in = transfer,
> +		.poll_interval_ms = 1000,
> +		.poll_count = 30,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +	if (rc < 0) {
> +		kfree(transfer);
> +		rc = FW_UPLOAD_ERR_RW_ERROR;
> +		goto out_free;
> +	}
> +
> +	*written = cur_size;
> +
> +	/* Activate FW if oneshot or if the last slice was written */
> +	if (cxlds->fw.oneshot || remaining == 0) {
> +		dev_dbg(&cxlmd->dev, "Activating firmware slot: %d\n",
> +			cxlds->fw.next_slot);
> +		rc = cxl_mem_activate_fw(cxlds, cxlds->fw.next_slot);
> +		if (rc < 0) {
> +			dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
> +				rc);
> +			rc = FW_UPLOAD_ERR_HW_ERROR;
> +			goto out_free;
> +		}
> +	}
> +
> +	rc = FW_UPLOAD_ERR_NONE;
> +
> +out_free:
> +	kfree(transfer);
> +	return rc;
> +}



> +
> +int cxl_memdev_setup_fw_upload(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_memdev *cxlmd = cxlds->cxlmd;

cxlmd.dev is only thing used, so I'd have a local variable
for that instead of cxlmd.


> +	struct fw_upload *fwl;
> +	int rc;
> +
> +	if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, cxlds->enabled_cmds))
> +		return 0;
> +
> +	fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev,
> +				       dev_name(&cxlmd->dev),
> +				       &cxl_memdev_fw_ops, cxlds);
> +	if (IS_ERR(fwl)) {
> +		dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> +		return PTR_ERR(fwl);

It's called from probe only so could use dev_err_probe() for slight
simplification.

> +	}
> +
> +	cxlds->fwl = fwl;

What is cxlds->fwl for?  I'm not seeing it being used except just below which
can use the local variable instead.


> +	rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload,
> +				      cxlds->fwl);
> +	if (rc)
> +		dev_err(&cxlmd->dev,
> +			"Failed to add firmware loader remove action: %d\n",
> +			rc);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
> +
>  static const struct file_operations cxl_memdev_fops = {
>  	.owner = THIS_MODULE,
>  	.unlocked_ioctl = cxl_memdev_ioctl,
> @@ -581,7 +888,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  
>  	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
>  	if (rc)
> -		return ERR_PTR(rc);
> +		goto err;

Why is this change here?   Fairly sure it results in a duplicate release.

>  	return cxlmd;
>  
>  err:
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index a78e40e6d0e0..ef0b4821b312 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -842,6 +842,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> +	rc = cxl_memdev_setup_fw_upload(cxlds);
> +	if (rc)
> +		return rc;
> +
>  	rc = cxl_event_config(host_bridge, cxlds);
>  	if (rc)
>  		return rc;

Re: [PATCH v2 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Jonathan Cameron]

On Mon, 05 Jun 2023 14:20:25 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
> CXL mailbox commands to the cxl_test emulated memdevs to enable
> end-to-end unit testing of a firmware update flow. For now, only
> advertise an 'offline activation' capability as that is all the CXL
> memdev driver currently implements.
> 
> Add some canned values for the serial number fields, and create a
> platform device sysfs knob to calculate the sha256sum of the firmware
> image that was received, so a unit test can compare it with the original
> file that was uploaded.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Cc: Russ Weight <russell.h.weight@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ben Widawsky <bwidawsk@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

One trivial comment inline.

I've somewhat lost track of the test utils, so less comfortable on reviewing
them than the kernel code.  With that in mind this looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  tools/testing/cxl/test/mem.c | 162 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 162 insertions(+)
> 


> +static int mock_activate_fw(struct cxl_dev_state *cxlds,
> +			    struct cxl_mbox_cmd *cmd)
> +{
> +	struct cxl_mbox_activate_fw *activate = cmd->payload_in;
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
> +
> +	if (activate->slot == 0 || activate->slot > FW_SLOTS)
> +		return -EINVAL;
> +
> +	switch (activate->action) {
> +	case CXL_FW_ACTIVATE_ONLINE:
> +		mdata->fw_slot = activate->slot;
> +		mdata->fw_staged = 0;
> +		break;
> +	case CXL_FW_ACTIVATE_OFFLINE:
> +		mdata->fw_staged = activate->slot;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;

Might as well push the return 0 up to the cases instead of breaking out.


>

Re: [PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Verma, Vishal L]

On Thu, 2023-06-08 at 15:49 +0100, Jonathan Cameron wrote:
> 
<..>
> > +
> > +static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
> > +                                      u32 offset, u32 size, u32 *written)
> > +{
> > +       struct cxl_dev_state *cxlds = fwl->dd_handle;
> > +       struct cxl_memdev *cxlmd = cxlds->cxlmd;
> > +       struct cxl_mbox_transfer_fw *transfer;
> > +       struct cxl_mbox_cmd mbox_cmd;
> > +       u32 cur_size, remaining;
> > +       size_t size_in;
> > +       int rc;
> > +
> > +       *written = 0;
> > +
> > +       /* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
> > +       if (!IS_ALIGNED(offset, CXL_FW_TRANSFER_ALIGNMENT)) {
> > +               dev_err(&cxlmd->dev,
> > +                       "misaligned offset for FW transfer slice (%u)\n",
> > +                       offset);
> > +               return FW_UPLOAD_ERR_RW_ERROR;
> > +       }
> > +
> > +       /* Pick transfer size based on cxlds->payload_size */
> > +       cur_size = min_t(size_t, size, cxlds->payload_size - sizeof(*transfer));
> 
> If size > cxlds->payload_size - sizeof(*transfer) what ensures that the step
> we take forwards results in the next read having an offset that is 128B aligned?
> 
> I think cur_size needs to be forced to be a multiple of 128Bytes as well.

The fact that sizeof(*transfer) is 128 bytes, and payload_size is a
power of 2 starting with 256 should ensure alignment. Dan noted this
here, before which I did force alignment explicitly:

https://lore.kernel.org/linux-cxl/646c313f20907_33fb329412@dwillia2-xfh.jf.intel.com.notmuch/

This probably deserves a comment though - I'll add that.

> 
<..>

> > +
> > +int cxl_memdev_setup_fw_upload(struct cxl_dev_state *cxlds)
> > +{
> > +       struct cxl_memdev *cxlmd = cxlds->cxlmd;
> 
> cxlmd.dev is only thing used, so I'd have a local variable
> for that instead of cxlmd.
> 
> 
> > +       struct fw_upload *fwl;
> > +       int rc;
> > +
> > +       if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, cxlds->enabled_cmds))
> > +               return 0;
> > +
> > +       fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev,
> > +                                      dev_name(&cxlmd->dev),
> > +                                      &cxl_memdev_fw_ops, cxlds);
> > +       if (IS_ERR(fwl)) {
> > +               dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> > +               return PTR_ERR(fwl);
> 
> It's called from probe only so could use dev_err_probe() for slight
> simplification.

From what I can tell, this ends up looking like:

	fwl = firmware_upload_register(THIS_MODULE, dev,
dev_name(dev),
				       &cxl_memdev_fw_ops, cxlds);
	rc = dev_err_probe(dev, PTR_ERR(fwl),
			   "Failed to register firmware loader\n");
	if (rc)
		return rc;

Is that what you meant? Happy to make the change if so.

> 
> > @@ -581,7 +888,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> >  
> >         rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
> >         if (rc)
> > -               return ERR_PTR(rc);
> > +               goto err;
> 
> Why is this change here?   Fairly sure it results in a duplicate release.

Ah yep I think an artifact from the previous rev where I had the fw
setup happening in this function.

Also agree with all other comments that I didn't address, making those
changes for v3.

Thanks for the review!

Re: [PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Verma, Vishal L]

On Thu, 2023-06-08 at 20:15 +0000, Verma, Vishal L wrote:
> 
> > > +
> > > +       fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev,
> > > +                                      dev_name(&cxlmd->dev),
> > > +                                      &cxl_memdev_fw_ops, cxlds);
> > > +       if (IS_ERR(fwl)) {
> > > +               dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> > > +               return PTR_ERR(fwl);
> > 
> > It's called from probe only so could use dev_err_probe() for slight
> > simplification.
> 
> From what I can tell, this ends up looking like:
> 
>         fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
>                                        &cxl_memdev_fw_ops, cxlds);
>         rc = dev_err_probe(dev, PTR_ERR(fwl),
>                            "Failed to register firmware loader\n");
>         if (rc)
>                 return rc;
> 
> Is that what you meant? Happy to make the change if so.
> 
> 
Actually I can't drop the IS_ERR() check - so unless I'm missing
something, this doesn't look like much of a simplification:


	if (IS_ERR(fwl)) {
		rc = dev_err_probe(dev, PTR_ERR(fwl),
				   "Failed to register firmware loader\n");
		if (rc)
			return rc;
	}

Re: [PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Jonathan Cameron]

> >   
> > > +       struct fw_upload *fwl;
> > > +       int rc;
> > > +
> > > +       if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, cxlds->enabled_cmds))
> > > +               return 0;
> > > +
> > > +       fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev,
> > > +                                      dev_name(&cxlmd->dev),
> > > +                                      &cxl_memdev_fw_ops, cxlds);
> > > +       if (IS_ERR(fwl)) {
> > > +               dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> > > +               return PTR_ERR(fwl);  
> > 
> > It's called from probe only so could use dev_err_probe() for slight
> > simplification.  
> 
> From what I can tell, this ends up looking like:
> 
> 	fwl = firmware_upload_register(THIS_MODULE, dev,
> dev_name(dev),
> 				       &cxl_memdev_fw_ops, cxlds);
> 	rc = dev_err_probe(dev, PTR_ERR(fwl),
> 			   "Failed to register firmware loader\n");
> 	if (rc)
> 		return rc;
> 
> Is that what you meant? Happy to make the change if so.

	fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
				       &cxl_memdev_fw_ops, cxlds);
	if (IS_ERR(fwl)
		return dev_err_probe(dev, PTR_ERR(fwl),
				     "Failed to register firmware loader\n");

Re: [PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Jonathan Cameron]

On Thu, 8 Jun 2023 20:26:43 +0000
"Verma, Vishal L" <vishal.l.verma@intel.com> wrote:

> On Thu, 2023-06-08 at 20:15 +0000, Verma, Vishal L wrote:
> >   
> > > > +
> > > > +       fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev,
> > > > +                                      dev_name(&cxlmd->dev),
> > > > +                                      &cxl_memdev_fw_ops, cxlds);
> > > > +       if (IS_ERR(fwl)) {
> > > > +               dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> > > > +               return PTR_ERR(fwl);  
> > > 
> > > It's called from probe only so could use dev_err_probe() for slight
> > > simplification.  
> > 
> > From what I can tell, this ends up looking like:
> > 
> >         fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
> >                                        &cxl_memdev_fw_ops, cxlds);
> >         rc = dev_err_probe(dev, PTR_ERR(fwl),
> >                            "Failed to register firmware loader\n");
> >         if (rc)
> >                 return rc;
> > 
> > Is that what you meant? Happy to make the change if so.
> > 
> >   
> Actually I can't drop the IS_ERR() check - so unless I'm missing
> something, this doesn't look like much of a simplification:
> 
> 
> 	if (IS_ERR(fwl)) {
> 		rc = dev_err_probe(dev, PTR_ERR(fwl),
> 				   "Failed to register firmware loader\n");
> 		if (rc)
> 			return rc;
> 	}
> 

Ah. I replied to previous. It's simpler than that as you know rc != 0 as
it's IS_ERR(fwl)

dev_err_probe() does two helpful things over dev_err()
1. Handles stashing the debug messages for the deferred probe cases (not
   relevant here but harmless)
2. Returns the variable you pass in as second argument to allow
   return dev_err_probe()


Jonathan