[PATCH 0/7] cxl: Activate FW and userspace bgcmds

[PATCH 0/7] cxl: Activate FW and userspace bgcmds

[Davidlohr Bueso]

Hello,

The following addresses two of the requests by Micron, both for
general userspace bgcmd support as well as the more recent sync
meeting discussion around activate FW taking too long and timing
out.

This builds on the previous proposal:
https://lore.kernel.org/linux-cxl/20241022031809.242591-1-dave@stgolabs.net/

... with 2 main differences in this series (i) added the Activate
FW work; and (ii) prevents kernel canceling another kernel bgcmd -
this may later be extended for kernel policies, if required.

Overall this expands on the monopolizing side of things (async to
the driver):
 - user bgcmds are only enabled if they can be canceled
 - activate fw is not timeslicable, but the expectation was that it
 will not run for long (online activation is currently unsupported).
 This is not the case apparently.

Patch 1 is generic to all further functionality.
Patch 2 moves activate FW to the monopolizing (aka sanitize)
category.
Patch 3+4 are prep patches for canceling bg commands.
Patch 5 adds the cancelable machinery, no actual commands yet.
Patch 6 adds a sensible warning instead of silently failing.
Patch 7 adds the Populate Log command to the uapi.

Testing has been done via qemu, similar to the previous proposal
(hacking the code further to disable the userspace only policy).

Micron folks: this does require further adhoc testing, of course,
so feedback welcome.

Jonathan: does this satisfy the Maintanence requirements?

Applies against the next branch in the cxl.git tree.

Thanks!

Davidlohr Bueso (6):
  cxl/mbox: Track background commands from CEL
  cxl/mbox: Handle Activate FW as async bg
  cxl/pci: Lockless background synchronous polling
  cxl/mbox: Stronger cxl_mbox_background_complete() semantics
  cxl/mbox: Allow userspace background commands
  cxl/mbox: Shout upon async bgcmd race

Ravi Shankar (1):
  cxl/mbox: Add Populate Log support

 drivers/cxl/core/hdm.c       |   2 +-
 drivers/cxl/core/mbox.c      |  41 +++++-
 drivers/cxl/core/memdev.c    |   4 +-
 drivers/cxl/cxlmem.h         |  44 +++++-
 drivers/cxl/pci.c            | 269 +++++++++++++++++++++++++----------
 include/cxl/mailbox.h        |   4 +
 include/uapi/linux/cxl_mem.h |   1 +
 7 files changed, 276 insertions(+), 89 deletions(-)

--
2.39.5

[PATCH 1/7] cxl/mbox: Track background commands from CEL

[Davidlohr Bueso]

Remember whether or not a command is background-capable, per the CEL.
This will later be used to consult the bitmap to evaluate incoming
mbox commands.

Further, "enable" any background commands that didn't fall in the
uapi, poison or security categories, such as fw related. This prevents
situations where the command is incorrectly listed as unsupported by
the driver.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c | 30 ++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h    | 28 ++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 2689e6453c5a..4ea02f7a5b6b 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -202,6 +202,31 @@ static void cxl_set_poison_cmd_enabled(struct cxl_poison_state *poison,
 	}
 }
 
+static bool cxl_set_bgcmd_enabled(struct cxl_background_state *bg, u16 opcode)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_TRANSFER_FW:
+		set_bit(CXL_BG_ENABLED_TRANSFER_FW, bg->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_ACTIVATE_FW:
+		set_bit(CXL_BG_ENABLED_ACTIVATE_FW, bg->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_DO_MAINTENANCE:
+		set_bit(CXL_BG_ENABLED_DO_MAINTENANCE, bg->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_SCAN_MEDIA:
+		set_bit(CXL_BG_ENABLED_SCAN_MEDIA, bg->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_SANITIZE:
+		set_bit(CXL_BG_ENABLED_SANITIZE, bg->enabled_cmds);
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
 static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
 {
 	struct cxl_mem_command *c;
@@ -757,6 +782,7 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 
 	for (i = 0; i < cel_entries; i++) {
 		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
+		u16 effect = le16_to_cpu(cel_entry[i].effect);
 		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
 		int enabled = 0;
 
@@ -778,6 +804,10 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 			enabled++;
 		}
 
+		if (effect & CXL_CEL_BACKGROUND_OPERATION)
+			if (cxl_set_bgcmd_enabled(&mds->bg, opcode))
+				enabled++; /* might be redundant */
+
 		dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
 			enabled ? "enabled" : "unsupported by driver");
 	}
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 551b0ba2caa1..5c7fd4a6704c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -248,6 +248,16 @@ enum security_cmd_enabled_bits {
 	CXL_SEC_ENABLED_MAX
 };
 
+/* Device enabled background commands */
+enum background_cmd_enabled_bits {
+	CXL_BG_ENABLED_TRANSFER_FW,
+	CXL_BG_ENABLED_ACTIVATE_FW,
+	CXL_BG_ENABLED_DO_MAINTENANCE,
+	CXL_BG_ENABLED_SCAN_MEDIA,
+	CXL_BG_ENABLED_SANITIZE,
+	CXL_BG_ENABLED_MAX
+};
+
 /**
  * struct cxl_poison_state - Driver poison state info
  *
@@ -365,6 +375,16 @@ struct cxl_security_state {
 	struct kernfs_node *sanitize_node;
 };
 
+/**
+ * struct cxl_background_state - Driver background operation state info
+ *
+ * @enabled_cmds: All background commands enabled in the CEL
+ */
+struct cxl_background_state {
+	DECLARE_BITMAP(enabled_cmds, CXL_BG_ENABLED_MAX);
+};
+
+
 /*
  * enum cxl_devtype - delineate type-2 from a generic type-3 device
  * @CXL_DEVTYPE_DEVMEM - Vendor specific CXL Type-2 device implementing HDM-D or
@@ -484,6 +504,7 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
  * @poison: poison driver state info
  * @security: security driver state info
  * @fw: firmware upload / activation state
+ * @bg: background operation state
  * @mce_notifier: MCE notifier
  *
  * See CXL 3.0 8.2.9.8.2 Capacity Configuration and Label Storage for
@@ -504,6 +525,7 @@ struct cxl_memdev_state {
 	struct cxl_poison_state poison;
 	struct cxl_security_state security;
 	struct cxl_fw_state fw;
+	struct cxl_background_state bg;
 	struct notifier_block mce_notifier;
 };
 
@@ -580,6 +602,12 @@ struct cxl_mbox_get_supported_logs {
 	} __packed entry[];
 }  __packed;
 
+/*
+ * Command Effects Log (CEL)
+ * CXL 3.2 Section 8.2.10.5.2.1; Table 8-87
+ */
+#define CXL_CEL_BACKGROUND_OPERATION BIT(6)
+
 struct cxl_cel_entry {
 	__le16 opcode;
 	__le16 effect;
-- 
2.39.5

[PATCH 2/7] cxl/mbox: Handle Activate FW as async bg

[Davidlohr Bueso]

Allow Activate FW to behave as Sanitize by running in an async
context. These commands cannot be timesliced, and while it was
the expectation that hardware vendors perform (offline) activating
FW in a timely fashion, this might not always be the case.

This further reduces the timeout scenarios when handling the
bg command synchronously.

Decouple the async background handling from security.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/hdm.c    |  2 +-
 drivers/cxl/core/memdev.c |  4 +-
 drivers/cxl/cxlmem.h      | 13 +++---
 drivers/cxl/pci.c         | 91 ++++++++++++++++++++++++++-------------
 4 files changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index ab1007495f6b..0f6402b2faa6 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -795,7 +795,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 		struct cxl_memdev_state *mds =
 			to_cxl_memdev_state(cxlmd->cxlds);
 
-		if (mds && mds->security.sanitize_active) {
+		if (mds && mds->bg.opcode == CXL_MBOX_OP_SANITIZE) {
 			dev_dbg(&cxlmd->dev,
 				"attempted to commit %s during sanitize\n",
 				dev_name(&cxld->dev));
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index f88a13adf7fa..d82f8ce182d8 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -140,7 +140,7 @@ static ssize_t security_state_show(struct device *dev,
 
 	/* sync with latest submission state */
 	mutex_lock(&cxl_mbox->mbox_mutex);
-	if (mds->security.sanitize_active)
+	if (mds->bg.opcode == CXL_MBOX_OP_SANITIZE)
 		rc = sysfs_emit(buf, "sanitize\n");
 	mutex_unlock(&cxl_mbox->mbox_mutex);
 	if (rc)
@@ -1097,7 +1097,7 @@ static void sanitize_teardown_notifier(void *data)
 	mds->security.sanitize_node = NULL;
 	mutex_unlock(&cxl_mbox->mbox_mutex);
 
-	cancel_delayed_work_sync(&mds->security.poll_dwork);
+	cancel_delayed_work_sync(&mds->bg.poll_dwork);
 	sysfs_put(state);
 }
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 5c7fd4a6704c..6fe42871fbf4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -361,17 +361,11 @@ struct cxl_fw_state {
  *
  * @state: state of last security operation
  * @enabled_cmds: All security commands enabled in the CEL
- * @poll_tmo_secs: polling timeout
- * @sanitize_active: sanitize completion pending
- * @poll_dwork: polling work item
  * @sanitize_node: sanitation sysfs file to notify
  */
 struct cxl_security_state {
 	unsigned long state;
 	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
-	int poll_tmo_secs;
-	bool sanitize_active;
-	struct delayed_work poll_dwork;
 	struct kernfs_node *sanitize_node;
 };
 
@@ -379,12 +373,17 @@ struct cxl_security_state {
  * struct cxl_background_state - Driver background operation state info
  *
  * @enabled_cmds: All background commands enabled in the CEL
+ * @poll_dwork: polling background work item
+ * @poll_tmo_secs: polling timeout
+ * @opcode: async running background command
  */
 struct cxl_background_state {
 	DECLARE_BITMAP(enabled_cmds, CXL_BG_ENABLED_MAX);
+	struct delayed_work poll_dwork;
+	int poll_tmo_secs;
+	int opcode;
 };
 
-
 /*
  * enum cxl_devtype - delineate type-2 from a generic type-3 device
  * @CXL_DEVTYPE_DEVMEM - Vendor specific CXL Type-2 device implementing HDM-D or
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index bd100ac31672..0d8f3605dc3f 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -133,10 +133,11 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
 
 	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
 	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK, reg);
-	if (opcode == CXL_MBOX_OP_SANITIZE) {
+	if (opcode == CXL_MBOX_OP_SANITIZE ||
+	    opcode == CXL_MBOX_OP_ACTIVATE_FW) {
 		mutex_lock(&cxl_mbox->mbox_mutex);
-		if (mds->security.sanitize_node)
-			mod_delayed_work(system_wq, &mds->security.poll_dwork, 0);
+		if (mds->bg.opcode)
+			mod_delayed_work(system_wq, &mds->bg.poll_dwork, 0);
 		mutex_unlock(&cxl_mbox->mbox_mutex);
 	} else {
 		/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
@@ -149,30 +150,51 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
 /*
  * Sanitization operation polling mode.
  */
-static void cxl_mbox_sanitize_work(struct work_struct *work)
+static void cxl_mbox_bgpoll_work(struct work_struct *work)
 {
 	struct cxl_memdev_state *mds =
-		container_of(work, typeof(*mds), security.poll_dwork.work);
+		container_of(work, typeof(*mds), bg.poll_dwork.work);
 	struct cxl_dev_state *cxlds = &mds->cxlds;
 	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
 
 	mutex_lock(&cxl_mbox->mbox_mutex);
 	if (cxl_mbox_background_complete(cxlds)) {
-		mds->security.poll_tmo_secs = 0;
-		if (mds->security.sanitize_node)
+		mds->bg.poll_tmo_secs = 0;
+		if (mds->bg.opcode == CXL_MBOX_OP_SANITIZE)
 			sysfs_notify_dirent(mds->security.sanitize_node);
-		mds->security.sanitize_active = false;
 
-		dev_dbg(cxlds->dev, "Sanitization operation ended\n");
+		dev_dbg(cxlds->dev,
+			"Mailbox background operation (0x%04x) ended\n",
+			mds->bg.opcode);
+		mds->bg.opcode = 0;
 	} else {
-		int timeout = mds->security.poll_tmo_secs + 10;
+		int timeout = mds->bg.poll_tmo_secs + 10;
 
-		mds->security.poll_tmo_secs = min(15 * 60, timeout);
-		schedule_delayed_work(&mds->security.poll_dwork, timeout * HZ);
+		mds->bg.poll_tmo_secs = min(15 * 60, timeout);
+		schedule_delayed_work(&mds->bg.poll_dwork, timeout * HZ);
 	}
 	mutex_unlock(&cxl_mbox->mbox_mutex);
 }
 
+static inline bool cxl_is_background_cmd(struct cxl_memdev_state *mds,
+					 u16 opcode)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_TRANSFER_FW:
+		return test_bit(CXL_BG_ENABLED_TRANSFER_FW, mds->bg.enabled_cmds);
+	case CXL_MBOX_OP_ACTIVATE_FW:
+		return test_bit(CXL_BG_ENABLED_ACTIVATE_FW, mds->bg.enabled_cmds);
+	case CXL_MBOX_OP_DO_MAINTENANCE:
+		return test_bit(CXL_BG_ENABLED_DO_MAINTENANCE, mds->bg.enabled_cmds);
+	case CXL_MBOX_OP_SCAN_MEDIA:
+		return test_bit(CXL_BG_ENABLED_SCAN_MEDIA, mds->bg.enabled_cmds);
+	case CXL_MBOX_OP_SANITIZE:
+		return test_bit(CXL_BG_ENABLED_SANITIZE, mds->bg.enabled_cmds);
+	default:
+		return false;
+	}
+}
+
 /**
  * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
  * @cxl_mbox: CXL mailbox context
@@ -236,13 +258,25 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 	}
 
 	/*
-	 * With sanitize polling, hardware might be done and the poller still
-	 * not be in sync. Ensure no new command comes in until so. Keep the
-	 * hardware semantics and only allow device health status.
+	 * With async bg polling, hardware might be done and the poller still
+	 * not be in sync. At the very least, prevent any incoming bg-capable
+	 * commands during this window.
 	 */
-	if (mds->security.poll_tmo_secs > 0) {
+	switch (mds->bg.opcode) {
+	case CXL_MBOX_OP_SANITIZE:
+		/*
+		 * Ensure no new command comes in until so. Keep the
+		 * hardware semantics and only allow device health status.
+		 */
 		if (mbox_cmd->opcode != CXL_MBOX_OP_GET_HEALTH_INFO)
 			return -EBUSY;
+		break;
+	case CXL_MBOX_OP_ACTIVATE_FW:
+		if (cxl_is_background_cmd(mds, mbox_cmd->opcode))
+			return -EBUSY;
+		break;
+	default:
+		break;
 	}
 
 	cmd_reg = FIELD_PREP(CXLDEV_MBOX_CMD_COMMAND_OPCODE_MASK,
@@ -295,28 +329,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 		u64 bg_status_reg;
 		int i, timeout;
 
+		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
+			mbox_cmd->opcode);
+
 		/*
-		 * Sanitization is a special case which monopolizes the device
-		 * and cannot be timesliced. Handle asynchronously instead,
-		 * and allow userspace to poll(2) for completion.
+		 * Special cases which monopolize the device and cannot be
+		 * timesliced. Handle asynchronously instead.
 		 */
-		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE) {
-			if (mds->security.sanitize_active)
+		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE ||
+		    mbox_cmd->opcode == CXL_MBOX_OP_ACTIVATE_FW) {
+			if (mds->bg.opcode)
 				return -EBUSY;
 
 			/* give first timeout a second */
 			timeout = 1;
-			mds->security.poll_tmo_secs = timeout;
-			mds->security.sanitize_active = true;
-			schedule_delayed_work(&mds->security.poll_dwork,
+			mds->bg.poll_tmo_secs = timeout;
+			mds->bg.opcode = mbox_cmd->opcode;
+			schedule_delayed_work(&mds->bg.poll_dwork,
 					      timeout * HZ);
-			dev_dbg(dev, "Sanitization operation started\n");
 			goto success;
 		}
 
-		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
-			mbox_cmd->opcode);
-
 		timeout = mbox_cmd->poll_interval_ms;
 		for (i = 0; i < mbox_cmd->poll_count; i++) {
 			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
@@ -442,7 +475,7 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds, bool irq_avail)
 
 	dev_dbg(dev, "Mailbox payload sized %zu", cxl_mbox->payload_size);
 
-	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mbox_sanitize_work);
+	INIT_DELAYED_WORK(&mds->bg.poll_dwork, cxl_mbox_bgpoll_work);
 
 	/* background command interrupts are optional */
 	if (!(cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ) || !irq_avail)
-- 
2.39.5

[PATCH 3/7] cxl/pci: Lockless background synchronous polling

[Davidlohr Bueso]

For timesliced background commands we rely on holding the mbox_mutex
throughout the duration of the operation. This causes other incoming
commands to queue up behind, and interleaving executions while the
background command is timesliced by the user.

However, in order to support the mbox request cancel background
operation command, the lock will need to be available to actually
perform the request. Semantically this allows other commands to
many times be at the mercy of hardware returning the respective error.
Potentially users would be exposed to changes in the form of errors
instead of commands taking longer to run - but this is not foreseen
as a problem.

In order to not loose sync with the hardware, introduce a mailbox
atomic that blocks any other incoming bg operations while the driver
is still polling (synchronously) on the current one.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c |   1 +
 drivers/cxl/pci.c       | 112 ++++++++++++++++++++++++----------------
 include/cxl/mailbox.h   |   2 +
 3 files changed, 70 insertions(+), 45 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 4ea02f7a5b6b..aa1b04d8a78e 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1515,6 +1515,7 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
 
 	cxl_mbox->host = host;
 	mutex_init(&cxl_mbox->mbox_mutex);
+	atomic_set(&cxl_mbox->poll_bgop, 0);
 	rcuwait_init(&cxl_mbox->mbox_wait);
 
 	return 0;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 0d8f3605dc3f..3e6c231e9a8b 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -312,23 +312,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 	mbox_cmd->return_code =
 		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
 
-	/*
-	 * Handle the background command in a synchronous manner.
-	 *
-	 * All other mailbox commands will serialize/queue on the mbox_mutex,
-	 * which we currently hold. Furthermore this also guarantees that
-	 * cxl_mbox_background_complete() checks are safe amongst each other,
-	 * in that no new bg operation can occur in between.
-	 *
-	 * Background operations are timesliced in accordance with the nature
-	 * of the command. In the event of timeout, the mailbox state is
-	 * indeterminate until the next successful command submission and the
-	 * driver can get back in sync with the hardware state.
-	 */
 	if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
-		u64 bg_status_reg;
-		int i, timeout;
-
 		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
 			mbox_cmd->opcode);
 
@@ -338,6 +322,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 		 */
 		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE ||
 		    mbox_cmd->opcode == CXL_MBOX_OP_ACTIVATE_FW) {
+			int timeout;
+
 			if (mds->bg.opcode)
 				return -EBUSY;
 
@@ -347,41 +333,17 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 			mds->bg.opcode = mbox_cmd->opcode;
 			schedule_delayed_work(&mds->bg.poll_dwork,
 					      timeout * HZ);
-			goto success;
+		} else {
+			/* pairs with release/acquire semantics */
+			WARN_ON_ONCE(atomic_xchg(&cxl_mbox->poll_bgop,
+						 mbox_cmd->opcode));
 		}
-
-		timeout = mbox_cmd->poll_interval_ms;
-		for (i = 0; i < mbox_cmd->poll_count; i++) {
-			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
-						       cxl_mbox_background_complete(cxlds),
-						       TASK_UNINTERRUPTIBLE,
-						       msecs_to_jiffies(timeout)) > 0)
-				break;
-		}
-
-		if (!cxl_mbox_background_complete(cxlds)) {
-			dev_err(dev, "timeout waiting for background (%d ms)\n",
-				timeout * mbox_cmd->poll_count);
-			return -ETIMEDOUT;
-		}
-
-		bg_status_reg = readq(cxlds->regs.mbox +
-				      CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
-		mbox_cmd->return_code =
-			FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
-				  bg_status_reg);
-		dev_dbg(dev,
-			"Mailbox background operation (0x%04x) completed\n",
-			mbox_cmd->opcode);
-	}
-
-	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
+	} else if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
 		dev_dbg(dev, "Mailbox operation had an error: %s\n",
 			cxl_mbox_cmd_rc2str(mbox_cmd));
 		return 0; /* completed but caller must check return_code */
 	}
 
-success:
 	/* #7 */
 	cmd_reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_CMD_OFFSET);
 	out_len = FIELD_GET(CXLDEV_MBOX_CMD_PAYLOAD_LENGTH_MASK, cmd_reg);
@@ -411,11 +373,71 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 			     struct cxl_mbox_cmd *cmd)
 {
 	int rc;
+	struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
+	struct device *dev = cxlds->dev;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
 
 	mutex_lock(&cxl_mbox->mbox_mutex);
+	/*
+	 * Ensure cxl_mbox_background_complete() checks are safe amongst
+	 * each other: no new bg operation can occur in between while polling.
+	 */
+	if (cxl_is_background_cmd(mds, cmd->opcode)) {
+		if (atomic_read_acquire(&cxl_mbox->poll_bgop)) {
+			mutex_unlock(&cxl_mbox->mbox_mutex);
+			return -EBUSY;
+		}
+	}
+
 	rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
 	mutex_unlock(&cxl_mbox->mbox_mutex);
 
+	if (cmd->return_code != CXL_MBOX_CMD_RC_BACKGROUND)
+		return rc;
+
+	/*
+	 * Handle the background command in a synchronous manner. Background
+	 * operations are timesliced in accordance with the nature of the
+	 * command.
+	 */
+	if (cmd->opcode != CXL_MBOX_OP_SANITIZE &&
+	    cmd->opcode != CXL_MBOX_OP_ACTIVATE_FW) {
+		int i, timeout;
+		u64 bg_status_reg;
+
+		timeout = cmd->poll_interval_ms;
+		for (i = 0; i < cmd->poll_count; i++) {
+			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
+				       cxl_mbox_background_complete(cxlds),
+				       TASK_UNINTERRUPTIBLE,
+				       msecs_to_jiffies(timeout)) > 0)
+				break;
+		}
+
+		/*
+		 * In the event of timeout, the mailbox state is indeterminate
+		 * until the next successful command submission and the driver
+		 * can get back in sync with the hardware state.
+		 */
+		if (!cxl_mbox_background_complete(cxlds)) {
+			dev_err(dev, "timeout waiting for background (%d ms)\n",
+				timeout * cmd->poll_count);
+			rc = -ETIMEDOUT;
+			goto done;
+		}
+
+		bg_status_reg = readq(cxlds->regs.mbox +
+				      CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
+		cmd->return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
+					     bg_status_reg);
+		dev_dbg(dev,
+			"Mailbox background operation (0x%04x) completed\n",
+			cmd->opcode);
+done:
+		/* ensure clearing poll_bop is the last operation */
+		atomic_set_release(&cxl_mbox->poll_bgop, 0);
+	}
+
 	return rc;
 }
 
diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
index c4e99e2e3a9d..f3c60158bfca 100644
--- a/include/cxl/mailbox.h
+++ b/include/cxl/mailbox.h
@@ -51,6 +51,7 @@ struct cxl_mbox_cmd {
  *                (CXL 3.1 8.2.8.4.3 Mailbox Capabilities Register)
  * @mbox_mutex: mutex protects device mailbox and firmware
  * @mbox_wait: rcuwait for mailbox
+ * @poll_bgop: current background operation being polled on
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  * @feat_cap: Features capability
  */
@@ -61,6 +62,7 @@ struct cxl_mailbox {
 	size_t payload_size;
 	struct mutex mbox_mutex; /* lock to protect mailbox context */
 	struct rcuwait mbox_wait;
+	atomic_t poll_bgop;
 	int (*mbox_send)(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd *cmd);
 	enum cxl_features_capability feat_cap;
 };
-- 
2.39.5

[PATCH 4/7] cxl/mbox: Stronger cxl_mbox_background_complete() semantics

[Davidlohr Bueso]

Account for all situations where hw has no on-going background
command. Current semantics do no change with this patch, but will
serve for later supporting cancel requests. As such rename the
helper accordingly.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/pci.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 3e6c231e9a8b..3648efcc7c89 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -111,12 +111,12 @@ static int cxl_request_irq(struct cxl_dev_state *cxlds, int irq,
 					 dev_id);
 }
 
-static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
+static bool cxl_mbox_background_done(struct cxl_dev_state *cxlds)
 {
 	u64 reg;
 
-	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
-	return FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, reg) == 100;
+	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_STATUS_OFFSET);
+	return FIELD_GET(CXLDEV_MBOX_STATUS_BG_CMD, reg) == 0;
 }
 
 static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
@@ -128,7 +128,7 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
 	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
 
-	if (!cxl_mbox_background_complete(cxlds))
+	if (!cxl_mbox_background_done(cxlds))
 		return IRQ_NONE;
 
 	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
@@ -158,7 +158,7 @@ static void cxl_mbox_bgpoll_work(struct work_struct *work)
 	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
 
 	mutex_lock(&cxl_mbox->mbox_mutex);
-	if (cxl_mbox_background_complete(cxlds)) {
+	if (cxl_mbox_background_done(cxlds)) {
 		mds->bg.poll_tmo_secs = 0;
 		if (mds->bg.opcode == CXL_MBOX_OP_SANITIZE)
 			sysfs_notify_dirent(mds->security.sanitize_node);
@@ -379,7 +379,7 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 
 	mutex_lock(&cxl_mbox->mbox_mutex);
 	/*
-	 * Ensure cxl_mbox_background_complete() checks are safe amongst
+	 * Ensure cxl_mbox_background_done() checks are safe amongst
 	 * each other: no new bg operation can occur in between while polling.
 	 */
 	if (cxl_is_background_cmd(mds, cmd->opcode)) {
@@ -408,7 +408,7 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 		timeout = cmd->poll_interval_ms;
 		for (i = 0; i < cmd->poll_count; i++) {
 			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
-				       cxl_mbox_background_complete(cxlds),
+				       cxl_mbox_background_done(cxlds),
 				       TASK_UNINTERRUPTIBLE,
 				       msecs_to_jiffies(timeout)) > 0)
 				break;
@@ -419,7 +419,7 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 		 * until the next successful command submission and the driver
 		 * can get back in sync with the hardware state.
 		 */
-		if (!cxl_mbox_background_complete(cxlds)) {
+		if (!cxl_mbox_background_done(cxlds)) {
 			dev_err(dev, "timeout waiting for background (%d ms)\n",
 				timeout * cmd->poll_count);
 			rc = -ETIMEDOUT;
-- 
2.39.5

[PATCH 5/7] cxl/mbox: Allow userspace background commands

[Davidlohr Bueso]

CXL 3.1 introduced the ability to request that the current on-going
background command be aborted. As such userspace may be allowed to
monopolize the background operation capability if there are no
other bg commands that want to run.

The policy here are as follows:

1. Give bg-capable commands from the kernel higher priority than those
from userspace (albeit currently no command). Therefore a userspace
on-going command can be canceled so a kernel one may run instead.

2. Userspace bg commands may cancel each other on demand. It is not
up to the kernel to put any policies in place here.

3. Any previously synchronous polling timedout command that is still
running may also be aborted by any command. This window is expected
to be small.

The context of doing the cancellation request is the same as the new
incoming command, and will always hold the mbox_mutex, guaranteeing
that any successful cancel does not race with a third thread coming
in and stealing the effort.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c |  9 ++++--
 drivers/cxl/cxlmem.h    |  4 +++
 drivers/cxl/pci.c       | 72 +++++++++++++++++++++++++++++++++++++----
 include/cxl/mailbox.h   |  2 ++
 4 files changed, 78 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index aa1b04d8a78e..abb6bb77a43d 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -382,6 +382,7 @@ static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox_cmd,
 	*mbox_cmd = (struct cxl_mbox_cmd) {
 		.opcode = opcode,
 		.size_in = in_size,
+		.user = true,
 	};
 
 	if (in_size) {
@@ -787,8 +788,12 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 		int enabled = 0;
 
 		if (cmd) {
-			set_bit(cmd->info.id, cxl_mbox->enabled_cmds);
-			enabled++;
+			/* only enable uapi bgcmds that may be canceled */
+			if (!(effect & CXL_CEL_BACKGROUND_OPERATION) ||
+			    (effect & CXL_CEL_REQ_ABORT_BACKGROUND_SUPPORTED)) {
+				set_bit(cmd->info.id, cxl_mbox->enabled_cmds);
+				enabled++;
+			}
 		}
 
 		enabled += check_features_opcodes(opcode, &ro_cmds,
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 6fe42871fbf4..ebd7679143dd 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -376,12 +376,14 @@ struct cxl_security_state {
  * @poll_dwork: polling background work item
  * @poll_tmo_secs: polling timeout
  * @opcode: async running background command
+ * @user: whether or not operation was initialized by userspace (uapi)
  */
 struct cxl_background_state {
 	DECLARE_BITMAP(enabled_cmds, CXL_BG_ENABLED_MAX);
 	struct delayed_work poll_dwork;
 	int poll_tmo_secs;
 	int opcode;
+	bool user;
 };
 
 /*
@@ -539,6 +541,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
 enum cxl_opcode {
 	CXL_MBOX_OP_INVALID		= 0x0000,
 	CXL_MBOX_OP_RAW			= CXL_MBOX_OP_INVALID,
+	CXL_MBOX_OP_REQUEST_ABORT_BG_OP = 0x0005,
 	CXL_MBOX_OP_GET_EVENT_RECORD	= 0x0100,
 	CXL_MBOX_OP_CLEAR_EVENT_RECORD	= 0x0101,
 	CXL_MBOX_OP_GET_EVT_INT_POLICY	= 0x0102,
@@ -606,6 +609,7 @@ struct cxl_mbox_get_supported_logs {
  * CXL 3.2 Section 8.2.10.5.2.1; Table 8-87
  */
 #define CXL_CEL_BACKGROUND_OPERATION BIT(6)
+#define CXL_CEL_REQ_ABORT_BACKGROUND_SUPPORTED BIT(8)
 
 struct cxl_cel_entry {
 	__le16 opcode;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 3648efcc7c89..6ada0f502914 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -134,7 +134,7 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
 	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
 	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK, reg);
 	if (opcode == CXL_MBOX_OP_SANITIZE ||
-	    opcode == CXL_MBOX_OP_ACTIVATE_FW) {
+	    opcode == CXL_MBOX_OP_ACTIVATE_FW || mds->bg.user) {
 		mutex_lock(&cxl_mbox->mbox_mutex);
 		if (mds->bg.opcode)
 			mod_delayed_work(system_wq, &mds->bg.poll_dwork, 0);
@@ -167,6 +167,7 @@ static void cxl_mbox_bgpoll_work(struct work_struct *work)
 			"Mailbox background operation (0x%04x) ended\n",
 			mds->bg.opcode);
 		mds->bg.opcode = 0;
+		mds->bg.user = false;
 	} else {
 		int timeout = mds->bg.poll_tmo_secs + 10;
 
@@ -263,6 +264,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 	 * commands during this window.
 	 */
 	switch (mds->bg.opcode) {
+	case 0:
+		break;
 	case CXL_MBOX_OP_SANITIZE:
 		/*
 		 * Ensure no new command comes in until so. Keep the
@@ -271,12 +274,10 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 		if (mbox_cmd->opcode != CXL_MBOX_OP_GET_HEALTH_INFO)
 			return -EBUSY;
 		break;
-	case CXL_MBOX_OP_ACTIVATE_FW:
+	default:
 		if (cxl_is_background_cmd(mds, mbox_cmd->opcode))
 			return -EBUSY;
 		break;
-	default:
-		break;
 	}
 
 	cmd_reg = FIELD_PREP(CXLDEV_MBOX_CMD_COMMAND_OPCODE_MASK,
@@ -321,7 +322,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 		 * timesliced. Handle asynchronously instead.
 		 */
 		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE ||
-		    mbox_cmd->opcode == CXL_MBOX_OP_ACTIVATE_FW) {
+		    mbox_cmd->opcode == CXL_MBOX_OP_ACTIVATE_FW ||
+		    mbox_cmd->user) {
 			int timeout;
 
 			if (mds->bg.opcode)
@@ -331,6 +333,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 			timeout = 1;
 			mds->bg.poll_tmo_secs = timeout;
 			mds->bg.opcode = mbox_cmd->opcode;
+			mds->bg.user = mbox_cmd->user;
 			schedule_delayed_work(&mds->bg.poll_dwork,
 					      timeout * HZ);
 		} else {
@@ -369,6 +372,59 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 	return 0;
 }
 
+/*
+ * Attempt to cancel an on-going background command. Return true guarantees
+ * that no such operations are running and any leftover context is gone.
+ */
+static bool cxl_try_to_cancel_background(struct cxl_mailbox *cxl_mbox)
+{
+	int rc;
+	struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+	struct device *dev = cxlds->dev;
+	struct cxl_mbox_cmd cmd = {
+		.opcode = CXL_MBOX_OP_REQUEST_ABORT_BG_OP,
+	};
+
+	lockdep_assert_held(&cxl_mbox->mbox_mutex);
+
+	/* everything is finished, new bgcmd may proceed */
+	if (cxl_mbox_background_done(cxlds))
+		goto done;
+	if (!mds->bg.user) {
+		/*
+		 * Policy is around userpace being abortable:
+		 *   incoming user bgcmd -> abort on-going user bgcmd
+		 *   incoming kernel bgcmd -> abort on-going user bgcmd
+		 */
+		return false;
+	}
+
+	rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &cmd);
+	if (rc) {
+		dev_dbg(dev, "Failed to send abort request : %d\n", rc);
+		return false;
+	}
+
+	if (!cxl_mbox_background_done(cxlds))
+		return false;
+
+	dev_dbg(cxlds->dev, "Mailbox background operation aborted\n");
+done:
+	if (mds->bg.opcode) {
+		/*
+		 * Cancel the work and cleanup on its behalf - we hold
+		 * the mbox_mutex, cannot race with cxl_mbox_bgpoll_work().
+		 */
+		cancel_delayed_work_sync(&mds->bg.poll_dwork);
+		mds->bg.poll_tmo_secs = 0;
+		mds->bg.opcode = 0;
+		mds->bg.user = false;
+	}
+
+	return true;
+}
+
 static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 			     struct cxl_mbox_cmd *cmd)
 {
@@ -381,9 +437,11 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 	/*
 	 * Ensure cxl_mbox_background_done() checks are safe amongst
 	 * each other: no new bg operation can occur in between while polling.
+	 *
 	 */
 	if (cxl_is_background_cmd(mds, cmd->opcode)) {
-		if (atomic_read_acquire(&cxl_mbox->poll_bgop)) {
+		if (atomic_read_acquire(&cxl_mbox->poll_bgop) ||
+		    !cxl_try_to_cancel_background(cxl_mbox)) {
 			mutex_unlock(&cxl_mbox->mbox_mutex);
 			return -EBUSY;
 		}
@@ -401,7 +459,7 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
 	 * command.
 	 */
 	if (cmd->opcode != CXL_MBOX_OP_SANITIZE &&
-	    cmd->opcode != CXL_MBOX_OP_ACTIVATE_FW) {
+	    cmd->opcode != CXL_MBOX_OP_ACTIVATE_FW && !cmd->user) {
 		int i, timeout;
 		u64 bg_status_reg;
 
diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
index f3c60158bfca..4a4119f49d3b 100644
--- a/include/cxl/mailbox.h
+++ b/include/cxl/mailbox.h
@@ -19,6 +19,7 @@
  *            variable sized output commands, it tells the exact number of bytes
  *            written.
  * @min_out: (input) internal command output payload size validation
+ * @user: (input) Whether or not command was submitted from userspace (uapi).
  * @poll_count: (input) Number of timeouts to attempt.
  * @poll_interval_ms: (input) Time between mailbox background command polling
  *                    interval timeouts.
@@ -37,6 +38,7 @@ struct cxl_mbox_cmd {
 	size_t size_in;
 	size_t size_out;
 	size_t min_out;
+	bool user;
 	int poll_count;
 	int poll_interval_ms;
 	u16 return_code;
-- 
2.39.5

[PATCH 6/7] cxl/mbox: Shout upon async bgcmd race

[Davidlohr Bueso]

Make the condition a warning, as the mailbox will be corrupted
on such cases, which may trigger harder to debug issues down the
line when simply returning a common error.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 6ada0f502914..6404b01b64c9 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -326,7 +326,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
 		    mbox_cmd->user) {
 			int timeout;
 
-			if (mds->bg.opcode)
+			if (WARN_ON_ONCE(mds->bg.opcode))
 				return -EBUSY;
 
 			/* give first timeout a second */
-- 
2.39.5

Re: [PATCH 0/7] cxl: Activate FW and userspace bgcmds

[Davidlohr Bueso]

On Tue, 17 Jun 2025, Davidlohr Bueso wrote:

>Testing has been done via qemu, similar to the previous proposal
>(hacking the code further to disable the userspace only policy).

I should say that this fixlet is also required on the qemu side:

https://lore.kernel.org/linux-cxl/20250617214832.579960-1-dave@stgolabs.net/

[PATCH 7/7] cxl/mbox: Add Populate Log support

[Davidlohr Bueso]

From: Ravi Shankar <ravis.opensrc@micron.com>

Adding UAPI support for
  CXL r3.1 8.2.9.5.5 Populate Log.

Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
Apologies, this one had not gone out.

  drivers/cxl/core/mbox.c      | 1 +
  drivers/cxl/cxlmem.h         | 1 +
  include/uapi/linux/cxl_mem.h | 1 +
  3 files changed, 3 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index abb6bb77a43d..ef737f9dacf2 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -60,6 +60,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
  	CXL_CMD(GET_LOG_CAPS, 0x10, 0x4, 0),
  	CXL_CMD(CLEAR_LOG, 0x10, 0, 0),
  	CXL_CMD(GET_SUP_LOG_SUBLIST, 0x2, CXL_VARIABLE_PAYLOAD, 0),
+	CXL_CMD(POPULATE_LOG, 0x10, 0, 0),
  	CXL_CMD(SET_PARTITION_INFO, 0x0a, 0, 0),
  	CXL_CMD(SET_LSA, CXL_VARIABLE_PAYLOAD, 0, 0),
  	CXL_CMD(GET_ALERT_CONFIG, 0, 0x10, 0),
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ebd7679143dd..56d5f9ea823a 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -555,6 +555,7 @@ enum cxl_opcode {
  	CXL_MBOX_OP_GET_LOG		= 0x0401,
  	CXL_MBOX_OP_GET_LOG_CAPS	= 0x0402,
  	CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
+	CXL_MBOX_OP_POPULATE_LOG        = 0x0404,
  	CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
  	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
  	CXL_MBOX_OP_GET_FEATURE		= 0x0501,
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index c6c0fe27495d..ca7fbd1be873 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -50,6 +50,7 @@
  	___C(GET_LOG_CAPS, "Get Log Capabilities"),			  \
  	___C(CLEAR_LOG, "Clear Log"),					  \
  	___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),	  \
+	___C(POPULATE_LOG, "Populate Log"),				  \
  	___C(MAX, "invalid / last command")
  
  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
-- 
2.39.5

Re: [PATCH 1/7] cxl/mbox: Track background commands from CEL

[Jonathan Cameron]

On Tue, 17 Jun 2025 12:36:05 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Remember whether or not a command is background-capable, per the CEL.
> This will later be used to consult the bitmap to evaluate incoming
> mbox commands.
> 
> Further, "enable" any background commands that didn't fall in the
> uapi, poison or security categories, such as fw related. This prevents
> situations where the command is incorrectly listed as unsupported by
> the driver.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c | 30 ++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h    | 28 ++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2689e6453c5a..4ea02f7a5b6b 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -202,6 +202,31 @@ static void cxl_set_poison_cmd_enabled(struct cxl_poison_state *poison,
>  	}
>  }
>  
> +static bool cxl_set_bgcmd_enabled(struct cxl_background_state *bg, u16 opcode)
> +{
> +	switch (opcode) {
> +	case CXL_MBOX_OP_TRANSFER_FW:
> +		set_bit(CXL_BG_ENABLED_TRANSFER_FW, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_ACTIVATE_FW:
> +		set_bit(CXL_BG_ENABLED_ACTIVATE_FW, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_DO_MAINTENANCE:
> +		set_bit(CXL_BG_ENABLED_DO_MAINTENANCE, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SCAN_MEDIA:
> +		set_bit(CXL_BG_ENABLED_SCAN_MEDIA, bg->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SANITIZE:
> +		set_bit(CXL_BG_ENABLED_SANITIZE, bg->enabled_cmds);
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
>  {
>  	struct cxl_mem_command *c;
> @@ -757,6 +782,7 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  
>  	for (i = 0; i < cel_entries; i++) {
>  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> +		u16 effect = le16_to_cpu(cel_entry[i].effect);
>  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>  		int enabled = 0;
>  
> @@ -778,6 +804,10 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  			enabled++;
>  		}
>  
> +		if (effect & CXL_CEL_BACKGROUND_OPERATION)
> +			if (cxl_set_bgcmd_enabled(&mds->bg, opcode))
> +				enabled++; /* might be redundant */

I suspect we'll forget in the long run what 'redundant' means here.
Perhaps make it more explicit.
/* May already have been detected as enabled */

The whole incrementing of enabled pattern hides that we don't really care
about transitions other than 0->1.  Maybe we should consider making it
a bool and using
enabled |= check_features_opcode() a few lines above this.



> +
>  		dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
>  			enabled ? "enabled" : "unsupported by driver");
>  	}
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 551b0ba2caa1..5c7fd4a6704c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -248,6 +248,16 @@ enum security_cmd_enabled_bits {
>  	CXL_SEC_ENABLED_MAX
>  };
>  
> +/* Device enabled background commands */

What does 'Device enabled' refer to? Perhaps reword.

> +enum background_cmd_enabled_bits {
> +	CXL_BG_ENABLED_TRANSFER_FW,
> +	CXL_BG_ENABLED_ACTIVATE_FW,
> +	CXL_BG_ENABLED_DO_MAINTENANCE,
> +	CXL_BG_ENABLED_SCAN_MEDIA,
> +	CXL_BG_ENABLED_SANITIZE,
> +	CXL_BG_ENABLED_MAX
> +};
> +
>  /**
>   * struct cxl_poison_state - Driver poison state info
>   *
> @@ -365,6 +375,16 @@ struct cxl_security_state {
>  	struct kernfs_node *sanitize_node;
>  };
>  
> +/**
> + * struct cxl_background_state - Driver background operation state info
> + *
> + * @enabled_cmds: All background commands enabled in the CEL
> + */
> +struct cxl_background_state {
> +	DECLARE_BITMAP(enabled_cmds, CXL_BG_ENABLED_MAX);
> +};
> +
> +
>  /*
>   * enum cxl_devtype - delineate type-2 from a generic type-3 device
>   * @CXL_DEVTYPE_DEVMEM - Vendor specific CXL Type-2 device implementing HDM-D or
> @@ -484,6 +504,7 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
>   * @poison: poison driver state info
>   * @security: security driver state info
>   * @fw: firmware upload / activation state
> + * @bg: background operation state
>   * @mce_notifier: MCE notifier
>   *
>   * See CXL 3.0 8.2.9.8.2 Capacity Configuration and Label Storage for
> @@ -504,6 +525,7 @@ struct cxl_memdev_state {
>  	struct cxl_poison_state poison;
>  	struct cxl_security_state security;
>  	struct cxl_fw_state fw;
> +	struct cxl_background_state bg;
>  	struct notifier_block mce_notifier;
>  };
>  
> @@ -580,6 +602,12 @@ struct cxl_mbox_get_supported_logs {
>  	} __packed entry[];
>  }  __packed;
>  
> +/*
> + * Command Effects Log (CEL)
> + * CXL 3.2 Section 8.2.10.5.2.1; Table 8-87
> + */
> +#define CXL_CEL_BACKGROUND_OPERATION BIT(6)
> +
>  struct cxl_cel_entry {
>  	__le16 opcode;
>  	__le16 effect;

Re: [PATCH 2/7] cxl/mbox: Handle Activate FW as async bg

[Jonathan Cameron]

On Tue, 17 Jun 2025 12:36:06 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Allow Activate FW to behave as Sanitize by running in an async
> context. These commands cannot be timesliced, and while it was
> the expectation that hardware vendors perform (offline) activating
> FW in a timely fashion, this might not always be the case.
> 
> This further reduces the timeout scenarios when handling the
> bg command synchronously.
> 
> Decouple the async background handling from security.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Trivial comment inline.

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


> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 5c7fd4a6704c..6fe42871fbf4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -361,17 +361,11 @@ struct cxl_fw_state {
>   *
>   * @state: state of last security operation
>   * @enabled_cmds: All security commands enabled in the CEL
> - * @poll_tmo_secs: polling timeout
> - * @sanitize_active: sanitize completion pending
> - * @poll_dwork: polling work item
>   * @sanitize_node: sanitation sysfs file to notify
>   */
>  struct cxl_security_state {
>  	unsigned long state;
>  	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
> -	int poll_tmo_secs;
> -	bool sanitize_active;
> -	struct delayed_work poll_dwork;
>  	struct kernfs_node *sanitize_node;
>  };
>  
> @@ -379,12 +373,17 @@ struct cxl_security_state {
>   * struct cxl_background_state - Driver background operation state info
>   *
>   * @enabled_cmds: All background commands enabled in the CEL
> + * @poll_dwork: polling background work item
> + * @poll_tmo_secs: polling timeout
> + * @opcode: async running background command
>   */
>  struct cxl_background_state {
>  	DECLARE_BITMAP(enabled_cmds, CXL_BG_ENABLED_MAX);
> +	struct delayed_work poll_dwork;
> +	int poll_tmo_secs;
> +	int opcode;
>  };
>  
> -
Stray change?
>  /*
>   * enum cxl_devtype - delineate type-2 from a generic type-3 device
>   * @CXL_DEVTYPE_DEVMEM - Vendor specific CXL Type-2 device implementing HDM-D or

Re: [PATCH 3/7] cxl/pci: Lockless background synchronous polling

[Jonathan Cameron]

On Tue, 17 Jun 2025 12:36:07 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> For timesliced background commands we rely on holding the mbox_mutex
> throughout the duration of the operation. This causes other incoming
> commands to queue up behind, and interleaving executions while the
> background command is timesliced by the user.
> 
> However, in order to support the mbox request cancel background
> operation command, the lock will need to be available to actually
> perform the request. Semantically this allows other commands to
> many times be at the mercy of hardware returning the respective error.
> Potentially users would be exposed to changes in the form of errors
> instead of commands taking longer to run - but this is not foreseen
> as a problem.
> 
> In order to not loose sync with the hardware, introduce a mailbox
> atomic that blocks any other incoming bg operations while the driver
> is still polling (synchronously) on the current one.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Minor comments inline.

Jonathan


> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0d8f3605dc3f..3e6c231e9a8b 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -312,23 +312,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  	mbox_cmd->return_code =
>  		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>  
> -	/*
> -	 * Handle the background command in a synchronous manner.
> -	 *
> -	 * All other mailbox commands will serialize/queue on the mbox_mutex,
> -	 * which we currently hold. Furthermore this also guarantees that
> -	 * cxl_mbox_background_complete() checks are safe amongst each other,
> -	 * in that no new bg operation can occur in between.
> -	 *
> -	 * Background operations are timesliced in accordance with the nature
> -	 * of the command. In the event of timeout, the mailbox state is
> -	 * indeterminate until the next successful command submission and the
> -	 * driver can get back in sync with the hardware state.
> -	 */
>  	if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
> -		u64 bg_status_reg;
> -		int i, timeout;
> -
>  		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
>  			mbox_cmd->opcode);
>  
> @@ -338,6 +322,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  		 */
>  		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE ||
>  		    mbox_cmd->opcode == CXL_MBOX_OP_ACTIVATE_FW) {
> +			int timeout;
> +
>  			if (mds->bg.opcode)
>  				return -EBUSY;
>  
> @@ -347,41 +333,17 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  			mds->bg.opcode = mbox_cmd->opcode;
>  			schedule_delayed_work(&mds->bg.poll_dwork,
>  					      timeout * HZ);
> -			goto success;
> +		} else {
> +			/* pairs with release/acquire semantics */
> +			WARN_ON_ONCE(atomic_xchg(&cxl_mbox->poll_bgop,
> +						 mbox_cmd->opcode));
>  		}
> -
> -		timeout = mbox_cmd->poll_interval_ms;
> -		for (i = 0; i < mbox_cmd->poll_count; i++) {
> -			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
> -						       cxl_mbox_background_complete(cxlds),
> -						       TASK_UNINTERRUPTIBLE,
> -						       msecs_to_jiffies(timeout)) > 0)
> -				break;
> -		}
> -
> -		if (!cxl_mbox_background_complete(cxlds)) {
> -			dev_err(dev, "timeout waiting for background (%d ms)\n",
> -				timeout * mbox_cmd->poll_count);
> -			return -ETIMEDOUT;
> -		}
> -
> -		bg_status_reg = readq(cxlds->regs.mbox +
> -				      CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> -		mbox_cmd->return_code =
> -			FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
> -				  bg_status_reg);
> -		dev_dbg(dev,
> -			"Mailbox background operation (0x%04x) completed\n",
> -			mbox_cmd->opcode);
> -	}
> -
> -	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> +	} else if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
>  		dev_dbg(dev, "Mailbox operation had an error: %s\n",
>  			cxl_mbox_cmd_rc2str(mbox_cmd));
>  		return 0; /* completed but caller must check return_code */
>  	}
>  
> -success:
>  	/* #7 */
>  	cmd_reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_CMD_OFFSET);
>  	out_len = FIELD_GET(CXLDEV_MBOX_CMD_PAYLOAD_LENGTH_MASK, cmd_reg);
> @@ -411,11 +373,71 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>  			     struct cxl_mbox_cmd *cmd)
>  {
>  	int rc;
> +	struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
> +	struct device *dev = cxlds->dev;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>  
>  	mutex_lock(&cxl_mbox->mbox_mutex);
Perhaps scoped_guard() appropriate here to avoid need to unlock explicitly
in the error path.

> +	/*
> +	 * Ensure cxl_mbox_background_complete() checks are safe amongst
> +	 * each other: no new bg operation can occur in between while polling.
> +	 */
> +	if (cxl_is_background_cmd(mds, cmd->opcode)) {
> +		if (atomic_read_acquire(&cxl_mbox->poll_bgop)) {
If you use scoped_guard() the indent will be getting a bit high, so maybe
combine the previous two conditions.
> +			mutex_unlock(&cxl_mbox->mbox_mutex);
> +			return -EBUSY;
> +		}
> +	}
> +
>  	rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
>  	mutex_unlock(&cxl_mbox->mbox_mutex);
>  
> +	if (cmd->return_code != CXL_MBOX_CMD_RC_BACKGROUND)
> +		return rc;
> +
> +	/*
> +	 * Handle the background command in a synchronous manner. Background
> +	 * operations are timesliced in accordance with the nature of the
> +	 * command.
> +	 */
> +	if (cmd->opcode != CXL_MBOX_OP_SANITIZE &&
> +	    cmd->opcode != CXL_MBOX_OP_ACTIVATE_FW) {
> +		int i, timeout;
> +		u64 bg_status_reg;
> +
> +		timeout = cmd->poll_interval_ms;
> +		for (i = 0; i < cmd->poll_count; i++) {
> +			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
> +				       cxl_mbox_background_complete(cxlds),
> +				       TASK_UNINTERRUPTIBLE,
> +				       msecs_to_jiffies(timeout)) > 0)
> +				break;
> +		}
> +
> +		/*
> +		 * In the event of timeout, the mailbox state is indeterminate
> +		 * until the next successful command submission and the driver
> +		 * can get back in sync with the hardware state.
> +		 */
> +		if (!cxl_mbox_background_complete(cxlds)) {
> +			dev_err(dev, "timeout waiting for background (%d ms)\n",
> +				timeout * cmd->poll_count);
> +			rc = -ETIMEDOUT;
> +			goto done;
> +		}
> +
> +		bg_status_reg = readq(cxlds->regs.mbox +
> +				      CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> +		cmd->return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
> +					     bg_status_reg);
> +		dev_dbg(dev,
> +			"Mailbox background operation (0x%04x) completed\n",
> +			cmd->opcode);
> +done:
> +		/* ensure clearing poll_bop is the last operation */
> +		atomic_set_release(&cxl_mbox->poll_bgop, 0);
> +	}
> +
>  	return rc;
>  }

Re: [PATCH 4/7] cxl/mbox: Stronger cxl_mbox_background_complete() semantics

[Jonathan Cameron]

On Tue, 17 Jun 2025 12:36:08 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Account for all situations where hw has no on-going background
> command. Current semantics do no change with this patch, but will

do not

> serve for later supporting cancel requests. As such rename the
> helper accordingly.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH 5/7] cxl/mbox: Allow userspace background commands

[Jonathan Cameron]

On Tue, 17 Jun 2025 12:36:09 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> CXL 3.1 introduced the ability to request that the current on-going
> background command be aborted. As such userspace may be allowed to
> monopolize the background operation capability if there are no
> other bg commands that want to run.
> 
> The policy here are as follows:
> 
> 1. Give bg-capable commands from the kernel higher priority than those
> from userspace (albeit currently no command). Therefore a userspace
> on-going command can be canceled so a kernel one may run instead.
> 
> 2. Userspace bg commands may cancel each other on demand. It is not
> up to the kernel to put any policies in place here.
> 
> 3. Any previously synchronous polling timedout command that is still
> running may also be aborted by any command. This window is expected
> to be small.
> 
> The context of doing the cancellation request is the same as the new
> incoming command, and will always hold the mbox_mutex, guaranteeing
> that any successful cancel does not race with a third thread coming
> in and stealing the effort.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c |  9 ++++--
>  drivers/cxl/cxlmem.h    |  4 +++
>  drivers/cxl/pci.c       | 72 +++++++++++++++++++++++++++++++++++++----
>  include/cxl/mailbox.h   |  2 ++
>  4 files changed, 78 insertions(+), 9 deletions(-)

> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 3648efcc7c89..6ada0f502914 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c

> @@ -263,6 +264,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  	 * commands during this window.
>  	 */
>  	switch (mds->bg.opcode) {
> +	case 0:

I assume this is the race?  Add a comment so we can figure this out in future.

> +		break;
>  	case CXL_MBOX_OP_SANITIZE:
>  		/*
>  		 * Ensure no new command comes in until so. Keep the


>  static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>  			     struct cxl_mbox_cmd *cmd)
>  {
> @@ -381,9 +437,11 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>  	/*
>  	 * Ensure cxl_mbox_background_done() checks are safe amongst
>  	 * each other: no new bg operation can occur in between while polling.
> +	 *

Stray change.

>  	 */

Re: [PATCH 6/7] cxl/mbox: Shout upon async bgcmd race

[Jonathan Cameron]

On Tue, 17 Jun 2025 12:36:10 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Make the condition a warning, as the mailbox will be corrupted
> on such cases, which may trigger harder to debug issues down the
> line when simply returning a common error.

This is a little confusing.  I assume it is meant to be impossible to hit
this race?
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 6ada0f502914..6404b01b64c9 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -326,7 +326,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  		    mbox_cmd->user) {
>  			int timeout;
>  
> -			if (mds->bg.opcode)
> +			if (WARN_ON_ONCE(mds->bg.opcode))
>  				return -EBUSY;
>  
>  			/* give first timeout a second */

Re: [PATCH 7/7] cxl/mbox: Add Populate Log support

[Jonathan Cameron]

On Tue, 17 Jun 2025 15:15:09 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> From: Ravi Shankar <ravis.opensrc@micron.com>
> 
> Adding UAPI support for
>   CXL r3.1 8.2.9.5.5 Populate Log.
> 
> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
This looks fine to me.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH 6/7] cxl/mbox: Shout upon async bgcmd race

[Davidlohr Bueso]

On Mon, 23 Jun 2025, Jonathan Cameron wrote:

>On Tue, 17 Jun 2025 12:36:10 -0700
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Make the condition a warning, as the mailbox will be corrupted
>> on such cases, which may trigger harder to debug issues down the
>> line when simply returning a common error.
>
>This is a little confusing.  I assume it is meant to be impossible to hit
>this race?

Correct, this is just being overly cautious.

Re: [PATCH 1/7] cxl/mbox: Track background commands from CEL

[Davidlohr Bueso]

On Mon, 23 Jun 2025, Jonathan Cameron wrote:

>On Tue, 17 Jun 2025 12:36:05 -0700
>Davidlohr Bueso <dave@stgolabs.net> wrote:

>> @@ -778,6 +804,10 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>>			enabled++;
>>		}
>>
>> +		if (effect & CXL_CEL_BACKGROUND_OPERATION)
>> +			if (cxl_set_bgcmd_enabled(&mds->bg, opcode))
>> +				enabled++; /* might be redundant */
>
>I suspect we'll forget in the long run what 'redundant' means here.
>Perhaps make it more explicit.
>/* May already have been detected as enabled */

Sure.

>The whole incrementing of enabled pattern hides that we don't really care
>about transitions other than 0->1.  Maybe we should consider making it
>a bool and using
>enabled |= check_features_opcode() a few lines above this.

Yeah, will consider as a post series cleanup.

Re: [PATCH 3/7] cxl/pci: Lockless background synchronous polling

[Davidlohr Bueso]

On Mon, 23 Jun 2025, Jonathan Cameron wrote:

>On Tue, 17 Jun 2025 12:36:07 -0700
>Davidlohr Bueso <dave@stgolabs.net> wrote:

>> @@ -411,11 +373,71 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>>			     struct cxl_mbox_cmd *cmd)
>>  {
>>	int rc;
>> +	struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
>> +	struct device *dev = cxlds->dev;
>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>>
>>	mutex_lock(&cxl_mbox->mbox_mutex);
>Perhaps scoped_guard() appropriate here to avoid need to unlock explicitly
>in the error path.

I'm not too fond of these, but yeah, I will convert to the new wrappers as most
of the other lock calls in the cxl code base.