[PATCH 0/2] cxl/mailbox: Replace racy error checking with timeouts

[PATCH 0/2] cxl/mailbox: Replace racy error checking with timeouts

[Dan Williams]

Quoting patch2:

    Device status can change without warning at any point in time. This
    effectively means that no amount of status checking before a command is
    submitted can guarantee that the device is not in an error condition
    when the command is later submitted. The clearest signal that a device
    is not able to process commands is if it fails to process commands.

So while "cxl/pci: Don't poll doorbell for mailbox access" [1] trimmed
cxl_pci_mbox_get() a bit, it still checks status in racy manner. Just
drop it altogether, add a doorbell sanity check to the initial
mailbox setup, and rely on timeouts to report errors.

Also, in anticipation of a 60s not being enough for device-bringup
scenarios, and a module parameter as an override.

This drops patch 3 [2], replaces patch 5 [3], and adds "cxl/pci: Defer
mailbox status checks to command timeouts" to the "CXL port prep work"
series [4]. Jonathan, given the policy tweaks and addition of a module
parameter I did not carry forward your reviewed-by for "cxl/pci:
Implement Interface Ready Timeout", please have another look when you
get a chance.

[1]: https://lore.kernel.org/r/20211129214721.1668325-6-ben.widawsky@intel.com
[2]: https://lore.kernel.org/r/20211129214721.1668325-4-ben.widawsky@intel.com
[3]: https://lore.kernel.org/r/20211129214721.1668325-6-ben.widawsky@intel.com
[4]: https://lore.kernel.org/r/20211129214721.1668325-1-ben.widawsky@intel.com

---

Ben Widawsky (1):
      cxl/pci: Implement Interface Ready Timeout

Dan Williams (1):
      cxl/pci: Defer mailbox status checks to command timeouts


 drivers/cxl/pci.c |  160 +++++++++++++++++++++--------------------------------
 1 file changed, 64 insertions(+), 96 deletions(-)

[PATCH 1/2] cxl/pci: Implement Interface Ready Timeout

[Dan Williams]

From: Ben Widawsky <ben.widawsky@intel.com>

The original driver implementation used the doorbell timeout for the
Mailbox Interface Ready bit to piggy back off of, since the latter does
not have a defined timeout. This functionality, introduced in commit
8adaf747c9f0 ("cxl/mem: Find device capabilities"), needs improvement as
the recent "Add Mailbox Ready Time" ECN timeout indicates that the
mailbox ready time can be significantly longer that 2 seconds.

While the specification limits the maximum timeout to 256s, the cxl_pci
driver gives up on the mailbox after 60s. This value corresponds with
important timeout values already present in the kernel. A module
parameter is provided as an emergency override.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
[djbw: add modparam, drop check_device_status()]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 8dc91fd3396a..519795432708 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/moduleparam.h>
 #include <linux/module.h>
+#include <linux/delay.h>
 #include <linux/sizes.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
@@ -35,6 +37,19 @@
 /* CXL 2.0 - 8.2.8.4 */
 #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
 
+/*
+ * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
+ * dictate how long to wait for the mailbox to become ready. The new
+ * field allows the device to tell software the amount of time to wait
+ * before mailbox ready. This field allows for up to 255 seconds. 255
+ * seconds is unreasonable long, and longer than other default timeouts
+ * in the OS. Use the more sane, 60 seconds instead.
+ */
+static unsigned short mbox_ready_timeout = 60;
+module_param(mbox_ready_timeout, ushort, 0600);
+MODULE_PARM_DESC(mbox_ready_timeout,
+		 "seconds to wait for mailbox ready status");
+
 static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
 {
 	const unsigned long start = jiffies;
@@ -281,6 +296,25 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
 static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 {
 	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
+	unsigned long timeout;
+	u64 md_status;
+
+	timeout = jiffies + mbox_ready_timeout * HZ;
+	do {
+		md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+		if (md_status & CXLMDEV_MBOX_IF_READY)
+			break;
+		if (msleep_interruptible(100))
+			break;
+	} while (!time_after(jiffies, timeout));
+
+	if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
+		dev_err(cxlds->dev,
+			"timeout awaiting mailbox ready, device state:%s%s\n",
+			md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
+			md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
+		return -EIO;
+        }
 
 	cxlds->mbox_send = cxl_pci_mbox_send;
 	cxlds->payload_size =

[PATCH 2/2] cxl/pci: Defer mailbox status checks to command timeouts

[Dan Williams]

Device status can change without warning at any point in time. This
effectively means that no amount of status checking before a command is
submitted can guarantee that the device is not in an error condition
when the command is later submitted. The clearest signal that a device
is not able to process commands is if it fails to process commands.

With the above understanding in hand, update cxl_pci_setup_mailbox() to
validate the readiness of the mailbox once at the beginning of time, and
then use timeouts and busy sequencing errors as the only occasions to
report status.

Just as before, unless and until the driver gains a reset recovery path,
doorbell clearing failures by the device are fatal to mailbox
operations.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |  138 ++++++++++++++---------------------------------------
 1 file changed, 36 insertions(+), 102 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 519795432708..36f80437a11a 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -72,14 +72,16 @@ static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
 	return 0;
 }
 
-static void cxl_pci_mbox_timeout(struct cxl_dev_state *cxlds,
-				 struct cxl_mbox_cmd *mbox_cmd)
-{
-	struct device *dev = cxlds->dev;
+#define report_status(dev, status, msg)                                        \
+	dev_err_ratelimited(dev, msg ", device state %s%s\n",                  \
+			    status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
+			    status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
 
-	dev_dbg(dev, "Mailbox command (opcode: %#x size: %zub) timed out\n",
-		mbox_cmd->opcode, mbox_cmd->size_in);
-}
+#define report_cmd_status(dev, cmd, status, msg)                               \
+	dev_err_ratelimited(dev, msg " (opcode: %#x), device state %s%s\n",    \
+			    (cmd)->opcode,                                     \
+			    status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
+			    status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
 
 /**
  * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
@@ -133,7 +135,11 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 
 	/* #1 */
 	if (cxl_doorbell_busy(cxlds)) {
-		dev_err_ratelimited(dev, "Mailbox re-busy after acquiring\n");
+		u64 md_status =
+			readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+
+		report_cmd_status(cxlds->dev, mbox_cmd, md_status,
+				  "mailbox queue busy");
 		return -EBUSY;
 	}
 
@@ -159,7 +165,10 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 	/* #5 */
 	rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
 	if (rc == -ETIMEDOUT) {
-		cxl_pci_mbox_timeout(cxlds, mbox_cmd);
+		u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+
+		report_cmd_status(cxlds->dev, mbox_cmd, md_status,
+				  "mailbox timeout");
 		return rc;
 	}
 
@@ -197,98 +206,13 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 	return 0;
 }
 
-/**
- * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
- * @cxlds: The device state to gain access to.
- *
- * Context: Any context. Takes the mbox_mutex.
- * Return: 0 if exclusive access was acquired.
- */
-static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
-{
-	struct device *dev = cxlds->dev;
-	u64 md_status;
-	int rc;
-
-	mutex_lock_io(&cxlds->mbox_mutex);
-
-	/*
-	 * XXX: There is some amount of ambiguity in the 2.0 version of the spec
-	 * around the mailbox interface ready (8.2.8.5.1.1).  The purpose of the
-	 * bit is to allow firmware running on the device to notify the driver
-	 * that it's ready to receive commands. It is unclear if the bit needs
-	 * to be read for each transaction mailbox, ie. the firmware can switch
-	 * it on and off as needed. Second, there is no defined timeout for
-	 * mailbox ready, like there is for the doorbell interface.
-	 *
-	 * Assumptions:
-	 * 1. The firmware might toggle the Mailbox Interface Ready bit, check
-	 *    it for every command.
-	 *
-	 * 2. If the doorbell is clear, the firmware should have first set the
-	 *    Mailbox Interface Ready bit. Therefore, waiting for the doorbell
-	 *    to be ready is sufficient.
-	 */
-	rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
-	if (rc) {
-		dev_warn(dev, "Mailbox interface not ready\n");
-		goto out;
-	}
-
-	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
-	if (!(md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status))) {
-		dev_err(dev, "mbox: reported doorbell ready, but not mbox ready\n");
-		rc = -EBUSY;
-		goto out;
-	}
-
-	/*
-	 * Hardware shouldn't allow a ready status but also have failure bits
-	 * set. Spit out an error, this should be a bug report
-	 */
-	rc = -EFAULT;
-	if (md_status & CXLMDEV_DEV_FATAL) {
-		dev_err(dev, "mbox: reported ready, but fatal\n");
-		goto out;
-	}
-	if (md_status & CXLMDEV_FW_HALT) {
-		dev_err(dev, "mbox: reported ready, but halted\n");
-		goto out;
-	}
-	if (CXLMDEV_RESET_NEEDED(md_status)) {
-		dev_err(dev, "mbox: reported ready, but reset needed\n");
-		goto out;
-	}
-
-	/* with lock held */
-	return 0;
-
-out:
-	mutex_unlock(&cxlds->mbox_mutex);
-	return rc;
-}
-
-/**
- * cxl_pci_mbox_put() - Release exclusive access to the mailbox.
- * @cxlds: The device state to communicate with.
- *
- * Context: Any context. Expects mbox_mutex to be held.
- */
-static void cxl_pci_mbox_put(struct cxl_dev_state *cxlds)
-{
-	mutex_unlock(&cxlds->mbox_mutex);
-}
-
 static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
 {
 	int rc;
 
-	rc = cxl_pci_mbox_get(cxlds);
-	if (rc)
-		return rc;
-
+	mutex_lock_io(&cxlds->mbox_mutex);
 	rc = __cxl_pci_mbox_send_cmd(cxlds, cmd);
-	cxl_pci_mbox_put(cxlds);
+	mutex_unlock(&cxlds->mbox_mutex);
 
 	return rc;
 }
@@ -309,12 +233,22 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 	} while (!time_after(jiffies, timeout));
 
 	if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
-		dev_err(cxlds->dev,
-			"timeout awaiting mailbox ready, device state:%s%s\n",
-			md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
-			md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
-		return -EIO;
-        }
+		report_status(cxlds->dev, md_status,
+			      "timeout awaiting mailbox ready");
+		return -ETIMEDOUT;
+	}
+
+	/*
+	 * A command may be in flight from a previous driver instance,
+	 * think kexec, do one doorbell wait so that
+	 * __cxl_pci_mbox_send_cmd() can assume that it is the only
+	 * source for future doorbell busy events.
+	 */
+	if (cxl_pci_mbox_wait_for_doorbell(cxlds) != 0) {
+		report_status(cxlds->dev, md_status,
+			      "timeout awaiting mailbox idle");
+		return -ETIMEDOUT;
+	}
 
 	cxlds->mbox_send = cxl_pci_mbox_send;
 	cxlds->payload_size =

Re: [PATCH 1/2] cxl/pci: Implement Interface Ready Timeout

[Ben Widawsky]

On 21-12-03 11:29:01, Dan Williams wrote:
> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> The original driver implementation used the doorbell timeout for the
> Mailbox Interface Ready bit to piggy back off of, since the latter does
> not have a defined timeout. This functionality, introduced in commit
> 8adaf747c9f0 ("cxl/mem: Find device capabilities"), needs improvement as
> the recent "Add Mailbox Ready Time" ECN timeout indicates that the
> mailbox ready time can be significantly longer that 2 seconds.
> 
> While the specification limits the maximum timeout to 256s, the cxl_pci
> driver gives up on the mailbox after 60s. This value corresponds with
> important timeout values already present in the kernel. A module
> parameter is provided as an emergency override.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> [djbw: add modparam, drop check_device_status()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/pci.c |   34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8dc91fd3396a..519795432708 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/moduleparam.h>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  #include <linux/sizes.h>
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> @@ -35,6 +37,19 @@
>  /* CXL 2.0 - 8.2.8.4 */
>  #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
>  
> +/*
> + * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
> + * dictate how long to wait for the mailbox to become ready. The new
> + * field allows the device to tell software the amount of time to wait
> + * before mailbox ready. This field allows for up to 255 seconds. 255
> + * seconds is unreasonable long, and longer than other default timeouts

s/unreasonable/unreasonably

> + * in the OS. Use the more sane, 60 seconds instead.
> + */
> +static unsigned short mbox_ready_timeout = 60;
> +module_param(mbox_ready_timeout, ushort, 0600);
> +MODULE_PARM_DESC(mbox_ready_timeout,
> +		 "seconds to wait for mailbox ready status");
> +

It's a bit of a weird thing to set as a modparam since it's not module specific,
but device specific. However, I suppose it's better than hardcoded 60s and I
can't come up with a better alternative. Perhaps mention this in the commit
message?

>  static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
>  {
>  	const unsigned long start = jiffies;
> @@ -281,6 +296,25 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
>  static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  {
>  	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> +	unsigned long timeout;
> +	u64 md_status;
> +
> +	timeout = jiffies + mbox_ready_timeout * HZ;
> +	do {
> +		md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +		if (md_status & CXLMDEV_MBOX_IF_READY)
> +			break;
> +		if (msleep_interruptible(100))
> +			break;
> +	} while (!time_after(jiffies, timeout));

One thing I noticed after I wrote these using time_after... time_before_eq()
might be a better fit. I'm slightly annoyed both APIs exist, but...

> +
> +	if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
> +		dev_err(cxlds->dev,
> +			"timeout awaiting mailbox ready, device state:%s%s\n",
> +			md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
> +			md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
> +		return -EIO;
> +        }
>  
>  	cxlds->mbox_send = cxl_pci_mbox_send;
>  	cxlds->payload_size =
> 

All in all, LGTM

Re: [PATCH 2/2] cxl/pci: Defer mailbox status checks to command timeouts

[Ben Widawsky]

On 21-12-03 11:29:06, Dan Williams wrote:
> Device status can change without warning at any point in time. This
> effectively means that no amount of status checking before a command is
> submitted can guarantee that the device is not in an error condition
> when the command is later submitted. The clearest signal that a device
> is not able to process commands is if it fails to process commands.
> 
> With the above understanding in hand, update cxl_pci_setup_mailbox() to
> validate the readiness of the mailbox once at the beginning of time, and
> then use timeouts and busy sequencing errors as the only occasions to
> report status.
> 
> Just as before, unless and until the driver gains a reset recovery path,
> doorbell clearing failures by the device are fatal to mailbox
> operations.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/pci.c |  138 ++++++++++++++---------------------------------------
>  1 file changed, 36 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 519795432708..36f80437a11a 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -72,14 +72,16 @@ static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
>  	return 0;
>  }
>  
> -static void cxl_pci_mbox_timeout(struct cxl_dev_state *cxlds,
> -				 struct cxl_mbox_cmd *mbox_cmd)
> -{
> -	struct device *dev = cxlds->dev;
> +#define report_status(dev, status, msg)                                        \
> +	dev_err_ratelimited(dev, msg ", device state %s%s\n",                  \
> +			    status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
> +			    status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
>  
> -	dev_dbg(dev, "Mailbox command (opcode: %#x size: %zub) timed out\n",
> -		mbox_cmd->opcode, mbox_cmd->size_in);
> -}
> +#define report_cmd_status(dev, cmd, status, msg)                               \
> +	dev_err_ratelimited(dev, msg " (opcode: %#x), device state %s%s\n",    \
> +			    (cmd)->opcode,                                     \
> +			    status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
> +			    status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
>  
>  /**
>   * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
> @@ -133,7 +135,11 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  
>  	/* #1 */
>  	if (cxl_doorbell_busy(cxlds)) {
> -		dev_err_ratelimited(dev, "Mailbox re-busy after acquiring\n");
> +		u64 md_status =
> +			readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +
> +		report_cmd_status(cxlds->dev, mbox_cmd, md_status,
> +				  "mailbox queue busy");
>  		return -EBUSY;
>  	}
>  
> @@ -159,7 +165,10 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  	/* #5 */
>  	rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
>  	if (rc == -ETIMEDOUT) {
> -		cxl_pci_mbox_timeout(cxlds, mbox_cmd);
> +		u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +
> +		report_cmd_status(cxlds->dev, mbox_cmd, md_status,
> +				  "mailbox timeout");
>  		return rc;
>  	}
>  
> @@ -197,98 +206,13 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  	return 0;
>  }
>  
> -/**
> - * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
> - * @cxlds: The device state to gain access to.
> - *
> - * Context: Any context. Takes the mbox_mutex.
> - * Return: 0 if exclusive access was acquired.
> - */
> -static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
> -{
> -	struct device *dev = cxlds->dev;
> -	u64 md_status;
> -	int rc;
> -
> -	mutex_lock_io(&cxlds->mbox_mutex);
> -
> -	/*
> -	 * XXX: There is some amount of ambiguity in the 2.0 version of the spec
> -	 * around the mailbox interface ready (8.2.8.5.1.1).  The purpose of the
> -	 * bit is to allow firmware running on the device to notify the driver
> -	 * that it's ready to receive commands. It is unclear if the bit needs
> -	 * to be read for each transaction mailbox, ie. the firmware can switch
> -	 * it on and off as needed. Second, there is no defined timeout for
> -	 * mailbox ready, like there is for the doorbell interface.
> -	 *
> -	 * Assumptions:
> -	 * 1. The firmware might toggle the Mailbox Interface Ready bit, check
> -	 *    it for every command.
> -	 *
> -	 * 2. If the doorbell is clear, the firmware should have first set the
> -	 *    Mailbox Interface Ready bit. Therefore, waiting for the doorbell
> -	 *    to be ready is sufficient.
> -	 */
> -	rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
> -	if (rc) {
> -		dev_warn(dev, "Mailbox interface not ready\n");
> -		goto out;
> -	}
> -
> -	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> -	if (!(md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status))) {
> -		dev_err(dev, "mbox: reported doorbell ready, but not mbox ready\n");
> -		rc = -EBUSY;
> -		goto out;
> -	}
> -
> -	/*
> -	 * Hardware shouldn't allow a ready status but also have failure bits
> -	 * set. Spit out an error, this should be a bug report
> -	 */
> -	rc = -EFAULT;
> -	if (md_status & CXLMDEV_DEV_FATAL) {
> -		dev_err(dev, "mbox: reported ready, but fatal\n");
> -		goto out;
> -	}
> -	if (md_status & CXLMDEV_FW_HALT) {
> -		dev_err(dev, "mbox: reported ready, but halted\n");
> -		goto out;
> -	}
> -	if (CXLMDEV_RESET_NEEDED(md_status)) {
> -		dev_err(dev, "mbox: reported ready, but reset needed\n");
> -		goto out;
> -	}
> -
> -	/* with lock held */
> -	return 0;
> -
> -out:
> -	mutex_unlock(&cxlds->mbox_mutex);
> -	return rc;
> -}
> -
> -/**
> - * cxl_pci_mbox_put() - Release exclusive access to the mailbox.
> - * @cxlds: The device state to communicate with.
> - *
> - * Context: Any context. Expects mbox_mutex to be held.
> - */
> -static void cxl_pci_mbox_put(struct cxl_dev_state *cxlds)
> -{
> -	mutex_unlock(&cxlds->mbox_mutex);
> -}
> -

I appreciate the goal of reducing the set of functions to initiate a mailbox
command. The idea behind get/put was that it might be desirable to issues
multiple commands in a sequence. I'll agree we have no such usage today and so
we can bring this back as needed. I just wanted to justify the intent :-)

>  static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
>  {
>  	int rc;
>  
> -	rc = cxl_pci_mbox_get(cxlds);
> -	if (rc)
> -		return rc;
> -
> +	mutex_lock_io(&cxlds->mbox_mutex);
>  	rc = __cxl_pci_mbox_send_cmd(cxlds, cmd);
> -	cxl_pci_mbox_put(cxlds);
> +	mutex_unlock(&cxlds->mbox_mutex);
>  
>  	return rc;
>  }
> @@ -309,12 +233,22 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  	} while (!time_after(jiffies, timeout));
>  
>  	if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
> -		dev_err(cxlds->dev,
> -			"timeout awaiting mailbox ready, device state:%s%s\n",
> -			md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
> -			md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
> -		return -EIO;
> -        }
> +		report_status(cxlds->dev, md_status,
> +			      "timeout awaiting mailbox ready");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/*
> +	 * A command may be in flight from a previous driver instance,
> +	 * think kexec, do one doorbell wait so that
> +	 * __cxl_pci_mbox_send_cmd() can assume that it is the only
> +	 * source for future doorbell busy events.
> +	 */
> +	if (cxl_pci_mbox_wait_for_doorbell(cxlds) != 0) {
> +		report_status(cxlds->dev, md_status,
> +			      "timeout awaiting mailbox idle");
> +		return -ETIMEDOUT;
> +	}

Can kexec happen while the mailbox mutex is held? That's really scary. If it
can't I believe this is unnecessary.

>  
>  	cxlds->mbox_send = cxl_pci_mbox_send;
>  	cxlds->payload_size =
> 

Re: [PATCH 1/2] cxl/pci: Implement Interface Ready Timeout

[Dan Williams]

On Fri, Dec 3, 2021 at 5:37 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-12-03 11:29:01, Dan Williams wrote:
> > From: Ben Widawsky <ben.widawsky@intel.com>
> >
> > The original driver implementation used the doorbell timeout for the
> > Mailbox Interface Ready bit to piggy back off of, since the latter does
> > not have a defined timeout. This functionality, introduced in commit
> > 8adaf747c9f0 ("cxl/mem: Find device capabilities"), needs improvement as
> > the recent "Add Mailbox Ready Time" ECN timeout indicates that the
> > mailbox ready time can be significantly longer that 2 seconds.
> >
> > While the specification limits the maximum timeout to 256s, the cxl_pci
> > driver gives up on the mailbox after 60s. This value corresponds with
> > important timeout values already present in the kernel. A module
> > parameter is provided as an emergency override.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > [djbw: add modparam, drop check_device_status()]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/pci.c |   34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 8dc91fd3396a..519795432708 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -1,7 +1,9 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/moduleparam.h>
> >  #include <linux/module.h>
> > +#include <linux/delay.h>
> >  #include <linux/sizes.h>
> >  #include <linux/mutex.h>
> >  #include <linux/list.h>
> > @@ -35,6 +37,19 @@
> >  /* CXL 2.0 - 8.2.8.4 */
> >  #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
> >
> > +/*
> > + * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
> > + * dictate how long to wait for the mailbox to become ready. The new
> > + * field allows the device to tell software the amount of time to wait
> > + * before mailbox ready. This field allows for up to 255 seconds. 255
> > + * seconds is unreasonable long, and longer than other default timeouts
>
> s/unreasonable/unreasonably

yup.

>
> > + * in the OS. Use the more sane, 60 seconds instead.
> > + */
> > +static unsigned short mbox_ready_timeout = 60;
> > +module_param(mbox_ready_timeout, ushort, 0600);
> > +MODULE_PARM_DESC(mbox_ready_timeout,
> > +              "seconds to wait for mailbox ready status");
> > +
>
> It's a bit of a weird thing to set as a modparam since it's not module specific,
> but device specific. However, I suppose it's better than hardcoded 60s and I
> can't come up with a better alternative. Perhaps mention this in the commit
> message?

The individual device timeout doesn't matter.

"This timeout is the singleton 'Linux' policy."

Does that clarify?

>
> >  static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
> >  {
> >       const unsigned long start = jiffies;
> > @@ -281,6 +296,25 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
> >  static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
> >  {
> >       const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> > +     unsigned long timeout;
> > +     u64 md_status;
> > +
> > +     timeout = jiffies + mbox_ready_timeout * HZ;
> > +     do {
> > +             md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> > +             if (md_status & CXLMDEV_MBOX_IF_READY)
> > +                     break;
> > +             if (msleep_interruptible(100))
> > +                     break;
> > +     } while (!time_after(jiffies, timeout));
>
> One thing I noticed after I wrote these using time_after... time_before_eq()
> might be a better fit. I'm slightly annoyed both APIs exist, but...

I don't think an occasional extra 100ms matters.

>
> > +
> > +     if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
> > +             dev_err(cxlds->dev,
> > +                     "timeout awaiting mailbox ready, device state:%s%s\n",
> > +                     md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
> > +                     md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
> > +             return -EIO;
> > +        }
> >
> >       cxlds->mbox_send = cxl_pci_mbox_send;
> >       cxlds->payload_size =
> >
>
> All in all, LGTM

cool.

Re: [PATCH 2/2] cxl/pci: Defer mailbox status checks to command timeouts

[Dan Williams]

On Fri, Dec 3, 2021 at 5:53 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-12-03 11:29:06, Dan Williams wrote:
> > Device status can change without warning at any point in time. This
> > effectively means that no amount of status checking before a command is
> > submitted can guarantee that the device is not in an error condition
> > when the command is later submitted. The clearest signal that a device
> > is not able to process commands is if it fails to process commands.
> >
> > With the above understanding in hand, update cxl_pci_setup_mailbox() to
> > validate the readiness of the mailbox once at the beginning of time, and
> > then use timeouts and busy sequencing errors as the only occasions to
> > report status.
> >
> > Just as before, unless and until the driver gains a reset recovery path,
> > doorbell clearing failures by the device are fatal to mailbox
> > operations.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/pci.c |  138 ++++++++++++++---------------------------------------
> >  1 file changed, 36 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 519795432708..36f80437a11a 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -72,14 +72,16 @@ static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
> >       return 0;
> >  }
> >
> > -static void cxl_pci_mbox_timeout(struct cxl_dev_state *cxlds,
> > -                              struct cxl_mbox_cmd *mbox_cmd)
> > -{
> > -     struct device *dev = cxlds->dev;
> > +#define report_status(dev, status, msg)                                        \
> > +     dev_err_ratelimited(dev, msg ", device state %s%s\n",                  \
> > +                         status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
> > +                         status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
> >
> > -     dev_dbg(dev, "Mailbox command (opcode: %#x size: %zub) timed out\n",
> > -             mbox_cmd->opcode, mbox_cmd->size_in);
> > -}
> > +#define report_cmd_status(dev, cmd, status, msg)                               \
> > +     dev_err_ratelimited(dev, msg " (opcode: %#x), device state %s%s\n",    \
> > +                         (cmd)->opcode,                                     \
> > +                         status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
> > +                         status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
> >
> >  /**
> >   * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
> > @@ -133,7 +135,11 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> >
> >       /* #1 */
> >       if (cxl_doorbell_busy(cxlds)) {
> > -             dev_err_ratelimited(dev, "Mailbox re-busy after acquiring\n");
> > +             u64 md_status =
> > +                     readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> > +
> > +             report_cmd_status(cxlds->dev, mbox_cmd, md_status,
> > +                               "mailbox queue busy");
> >               return -EBUSY;
> >       }
> >
> > @@ -159,7 +165,10 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> >       /* #5 */
> >       rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
> >       if (rc == -ETIMEDOUT) {
> > -             cxl_pci_mbox_timeout(cxlds, mbox_cmd);
> > +             u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> > +
> > +             report_cmd_status(cxlds->dev, mbox_cmd, md_status,
> > +                               "mailbox timeout");
> >               return rc;
> >       }
> >
> > @@ -197,98 +206,13 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> >       return 0;
> >  }
> >
> > -/**
> > - * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
> > - * @cxlds: The device state to gain access to.
> > - *
> > - * Context: Any context. Takes the mbox_mutex.
> > - * Return: 0 if exclusive access was acquired.
> > - */
> > -static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
> > -{
> > -     struct device *dev = cxlds->dev;
> > -     u64 md_status;
> > -     int rc;
> > -
> > -     mutex_lock_io(&cxlds->mbox_mutex);
> > -
> > -     /*
> > -      * XXX: There is some amount of ambiguity in the 2.0 version of the spec
> > -      * around the mailbox interface ready (8.2.8.5.1.1).  The purpose of the
> > -      * bit is to allow firmware running on the device to notify the driver
> > -      * that it's ready to receive commands. It is unclear if the bit needs
> > -      * to be read for each transaction mailbox, ie. the firmware can switch
> > -      * it on and off as needed. Second, there is no defined timeout for
> > -      * mailbox ready, like there is for the doorbell interface.
> > -      *
> > -      * Assumptions:
> > -      * 1. The firmware might toggle the Mailbox Interface Ready bit, check
> > -      *    it for every command.
> > -      *
> > -      * 2. If the doorbell is clear, the firmware should have first set the
> > -      *    Mailbox Interface Ready bit. Therefore, waiting for the doorbell
> > -      *    to be ready is sufficient.
> > -      */
> > -     rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
> > -     if (rc) {
> > -             dev_warn(dev, "Mailbox interface not ready\n");
> > -             goto out;
> > -     }
> > -
> > -     md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> > -     if (!(md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status))) {
> > -             dev_err(dev, "mbox: reported doorbell ready, but not mbox ready\n");
> > -             rc = -EBUSY;
> > -             goto out;
> > -     }
> > -
> > -     /*
> > -      * Hardware shouldn't allow a ready status but also have failure bits
> > -      * set. Spit out an error, this should be a bug report
> > -      */
> > -     rc = -EFAULT;
> > -     if (md_status & CXLMDEV_DEV_FATAL) {
> > -             dev_err(dev, "mbox: reported ready, but fatal\n");
> > -             goto out;
> > -     }
> > -     if (md_status & CXLMDEV_FW_HALT) {
> > -             dev_err(dev, "mbox: reported ready, but halted\n");
> > -             goto out;
> > -     }
> > -     if (CXLMDEV_RESET_NEEDED(md_status)) {
> > -             dev_err(dev, "mbox: reported ready, but reset needed\n");
> > -             goto out;
> > -     }
> > -
> > -     /* with lock held */
> > -     return 0;
> > -
> > -out:
> > -     mutex_unlock(&cxlds->mbox_mutex);
> > -     return rc;
> > -}
> > -
> > -/**
> > - * cxl_pci_mbox_put() - Release exclusive access to the mailbox.
> > - * @cxlds: The device state to communicate with.
> > - *
> > - * Context: Any context. Expects mbox_mutex to be held.
> > - */
> > -static void cxl_pci_mbox_put(struct cxl_dev_state *cxlds)
> > -{
> > -     mutex_unlock(&cxlds->mbox_mutex);
> > -}
> > -
>
> I appreciate the goal of reducing the set of functions to initiate a mailbox
> command. The idea behind get/put was that it might be desirable to issues
> multiple commands in a sequence. I'll agree we have no such usage today and so
> we can bring this back as needed. I just wanted to justify the intent :-)
>

If it came back, why would it be anything more than an open coded
acquisition of the mutex?

> >  static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
> >  {
> >       int rc;
> >
> > -     rc = cxl_pci_mbox_get(cxlds);
> > -     if (rc)
> > -             return rc;
> > -
> > +     mutex_lock_io(&cxlds->mbox_mutex);
> >       rc = __cxl_pci_mbox_send_cmd(cxlds, cmd);
> > -     cxl_pci_mbox_put(cxlds);
> > +     mutex_unlock(&cxlds->mbox_mutex);
> >
> >       return rc;
> >  }
> > @@ -309,12 +233,22 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
> >       } while (!time_after(jiffies, timeout));
> >
> >       if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
> > -             dev_err(cxlds->dev,
> > -                     "timeout awaiting mailbox ready, device state:%s%s\n",
> > -                     md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
> > -                     md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
> > -             return -EIO;
> > -        }
> > +             report_status(cxlds->dev, md_status,
> > +                           "timeout awaiting mailbox ready");
> > +             return -ETIMEDOUT;
> > +     }
> > +
> > +     /*
> > +      * A command may be in flight from a previous driver instance,
> > +      * think kexec, do one doorbell wait so that
> > +      * __cxl_pci_mbox_send_cmd() can assume that it is the only
> > +      * source for future doorbell busy events.
> > +      */
> > +     if (cxl_pci_mbox_wait_for_doorbell(cxlds) != 0) {
> > +             report_status(cxlds->dev, md_status,
> > +                           "timeout awaiting mailbox idle");
> > +             return -ETIMEDOUT;
> > +     }
>
> Can kexec happen while the mailbox mutex is held? That's really scary. If it
> can't I believe this is unnecessary.

Sure. kexec requires no coordination from the kernel being replaced.
It could be triggered by a crash on another CPU.

There is even a possibility userspace driver or BIOS could have been
using the mailbox before the driver was loaded, who knows. cxl_pci
just needs a reasonable assurance that any doorbell status it sees
going forward is either from a command it submitted or an error.

Re: [PATCH 1/2] cxl/pci: Implement Interface Ready Timeout

[Jonathan Cameron]

On Fri, 3 Dec 2021 11:29:01 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> The original driver implementation used the doorbell timeout for the
> Mailbox Interface Ready bit to piggy back off of, since the latter does
> not have a defined timeout. This functionality, introduced in commit
> 8adaf747c9f0 ("cxl/mem: Find device capabilities"), needs improvement as
> the recent "Add Mailbox Ready Time" ECN timeout indicates that the
> mailbox ready time can be significantly longer that 2 seconds.
> 
> While the specification limits the maximum timeout to 256s, the cxl_pci
> driver gives up on the mailbox after 60s. This value corresponds with
> important timeout values already present in the kernel. A module
> parameter is provided as an emergency override.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> [djbw: add modparam, drop check_device_status()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c |   34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8dc91fd3396a..519795432708 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/moduleparam.h>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  #include <linux/sizes.h>
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> @@ -35,6 +37,19 @@
>  /* CXL 2.0 - 8.2.8.4 */
>  #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
>  
> +/*
> + * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
> + * dictate how long to wait for the mailbox to become ready. The new
> + * field allows the device to tell software the amount of time to wait
> + * before mailbox ready. This field allows for up to 255 seconds. 255
> + * seconds is unreasonable long, and longer than other default timeouts
> + * in the OS. Use the more sane, 60 seconds instead.
> + */
> +static unsigned short mbox_ready_timeout = 60;
> +module_param(mbox_ready_timeout, ushort, 0600);
> +MODULE_PARM_DESC(mbox_ready_timeout,
> +		 "seconds to wait for mailbox ready status");
> +
>  static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
>  {
>  	const unsigned long start = jiffies;
> @@ -281,6 +296,25 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
>  static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  {
>  	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> +	unsigned long timeout;
> +	u64 md_status;
> +
> +	timeout = jiffies + mbox_ready_timeout * HZ;
> +	do {
> +		md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +		if (md_status & CXLMDEV_MBOX_IF_READY)
> +			break;
> +		if (msleep_interruptible(100))
> +			break;
> +	} while (!time_after(jiffies, timeout));
> +
> +	if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
> +		dev_err(cxlds->dev,
> +			"timeout awaiting mailbox ready, device state:%s%s\n",
> +			md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
> +			md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
> +		return -EIO;
> +        }
>  
>  	cxlds->mbox_send = cxl_pci_mbox_send;
>  	cxlds->payload_size =
> 

Re: [PATCH 2/2] cxl/pci: Defer mailbox status checks to command timeouts

[Jonathan Cameron]

On Fri, 3 Dec 2021 11:29:06 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Device status can change without warning at any point in time. This
> effectively means that no amount of status checking before a command is
> submitted can guarantee that the device is not in an error condition
> when the command is later submitted. The clearest signal that a device
> is not able to process commands is if it fails to process commands.
> 
> With the above understanding in hand, update cxl_pci_setup_mailbox() to
> validate the readiness of the mailbox once at the beginning of time, and
> then use timeouts and busy sequencing errors as the only occasions to
> report status.
> 
> Just as before, unless and until the driver gains a reset recovery path,
> doorbell clearing failures by the device are fatal to mailbox
> operations.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Just some bike-shedding on naming - up to you whether you think it is worth a tweak.

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

Thanks,

J
> ---
>  drivers/cxl/pci.c |  138 ++++++++++++++---------------------------------------
>  1 file changed, 36 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 519795432708..36f80437a11a 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -72,14 +72,16 @@ static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
>  	return 0;
>  }
>  
> -static void cxl_pci_mbox_timeout(struct cxl_dev_state *cxlds,
> -				 struct cxl_mbox_cmd *mbox_cmd)
> -{
> -	struct device *dev = cxlds->dev;
> +#define report_status(dev, status, msg)                                        \
> +	dev_err_ratelimited(dev, msg ", device state %s%s\n",                  \
> +			    status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
> +			    status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
>  
> -	dev_dbg(dev, "Mailbox command (opcode: %#x size: %zub) timed out\n",
> -		mbox_cmd->opcode, mbox_cmd->size_in);
> -}
> +#define report_cmd_status(dev, cmd, status, msg)                               \

Naming wise, can we hint that this is an error print?  maybe
report_err_cmd_status() or similar?

> +	dev_err_ratelimited(dev, msg " (opcode: %#x), device state %s%s\n",    \
> +			    (cmd)->opcode,                                     \
> +			    status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
> +			    status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
>  
>  /**
>   * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
> @@ -133,7 +135,11 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  
>  	/* #1 */
>  	if (cxl_doorbell_busy(cxlds)) {
> -		dev_err_ratelimited(dev, "Mailbox re-busy after acquiring\n");
> +		u64 md_status =
> +			readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +
> +		report_cmd_status(cxlds->dev, mbox_cmd, md_status,
> +				  "mailbox queue busy");
>  		return -EBUSY;
>  	}
>  
> @@ -159,7 +165,10 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  	/* #5 */
>  	rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
>  	if (rc == -ETIMEDOUT) {
> -		cxl_pci_mbox_timeout(cxlds, mbox_cmd);
> +		u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +
> +		report_cmd_status(cxlds->dev, mbox_cmd, md_status,
> +				  "mailbox timeout");
>  		return rc;
>  	}
>  
> @@ -197,98 +206,13 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  	return 0;
>  }
>  
> -/**
> - * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
> - * @cxlds: The device state to gain access to.
> - *
> - * Context: Any context. Takes the mbox_mutex.
> - * Return: 0 if exclusive access was acquired.
> - */
> -static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
> -{
> -	struct device *dev = cxlds->dev;
> -	u64 md_status;
> -	int rc;
> -
> -	mutex_lock_io(&cxlds->mbox_mutex);
> -
> -	/*
> -	 * XXX: There is some amount of ambiguity in the 2.0 version of the spec
> -	 * around the mailbox interface ready (8.2.8.5.1.1).  The purpose of the
> -	 * bit is to allow firmware running on the device to notify the driver
> -	 * that it's ready to receive commands. It is unclear if the bit needs
> -	 * to be read for each transaction mailbox, ie. the firmware can switch
> -	 * it on and off as needed. Second, there is no defined timeout for
> -	 * mailbox ready, like there is for the doorbell interface.
> -	 *
> -	 * Assumptions:
> -	 * 1. The firmware might toggle the Mailbox Interface Ready bit, check
> -	 *    it for every command.
> -	 *
> -	 * 2. If the doorbell is clear, the firmware should have first set the
> -	 *    Mailbox Interface Ready bit. Therefore, waiting for the doorbell
> -	 *    to be ready is sufficient.
> -	 */
> -	rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
> -	if (rc) {
> -		dev_warn(dev, "Mailbox interface not ready\n");
> -		goto out;
> -	}
> -
> -	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> -	if (!(md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status))) {
> -		dev_err(dev, "mbox: reported doorbell ready, but not mbox ready\n");
> -		rc = -EBUSY;
> -		goto out;
> -	}
> -
> -	/*
> -	 * Hardware shouldn't allow a ready status but also have failure bits
> -	 * set. Spit out an error, this should be a bug report
> -	 */
> -	rc = -EFAULT;
> -	if (md_status & CXLMDEV_DEV_FATAL) {
> -		dev_err(dev, "mbox: reported ready, but fatal\n");
> -		goto out;
> -	}
> -	if (md_status & CXLMDEV_FW_HALT) {
> -		dev_err(dev, "mbox: reported ready, but halted\n");
> -		goto out;
> -	}
> -	if (CXLMDEV_RESET_NEEDED(md_status)) {
> -		dev_err(dev, "mbox: reported ready, but reset needed\n");
> -		goto out;
> -	}
> -
> -	/* with lock held */
> -	return 0;
> -
> -out:
> -	mutex_unlock(&cxlds->mbox_mutex);
> -	return rc;
> -}
> -
> -/**
> - * cxl_pci_mbox_put() - Release exclusive access to the mailbox.
> - * @cxlds: The device state to communicate with.
> - *
> - * Context: Any context. Expects mbox_mutex to be held.
> - */
> -static void cxl_pci_mbox_put(struct cxl_dev_state *cxlds)
> -{
> -	mutex_unlock(&cxlds->mbox_mutex);
> -}
> -
>  static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
>  {
>  	int rc;
>  
> -	rc = cxl_pci_mbox_get(cxlds);
> -	if (rc)
> -		return rc;
> -
> +	mutex_lock_io(&cxlds->mbox_mutex);
>  	rc = __cxl_pci_mbox_send_cmd(cxlds, cmd);
> -	cxl_pci_mbox_put(cxlds);
> +	mutex_unlock(&cxlds->mbox_mutex);
>  
>  	return rc;
>  }
> @@ -309,12 +233,22 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  	} while (!time_after(jiffies, timeout));
>  
>  	if (!(md_status & CXLMDEV_MBOX_IF_READY)) {
> -		dev_err(cxlds->dev,
> -			"timeout awaiting mailbox ready, device state:%s%s\n",
> -			md_status & CXLMDEV_DEV_FATAL ? " fatal" : "",
> -			md_status & CXLMDEV_FW_HALT ? " firmware-halt" : "");
> -		return -EIO;
> -        }
> +		report_status(cxlds->dev, md_status,
> +			      "timeout awaiting mailbox ready");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/*
> +	 * A command may be in flight from a previous driver instance,
> +	 * think kexec, do one doorbell wait so that
> +	 * __cxl_pci_mbox_send_cmd() can assume that it is the only
> +	 * source for future doorbell busy events.
> +	 */
> +	if (cxl_pci_mbox_wait_for_doorbell(cxlds) != 0) {
> +		report_status(cxlds->dev, md_status,
> +			      "timeout awaiting mailbox idle");
> +		return -ETIMEDOUT;
> +	}
>  
>  	cxlds->mbox_send = cxl_pci_mbox_send;
>  	cxlds->payload_size =
> 

Re: [PATCH 2/2] cxl/pci: Defer mailbox status checks to command timeouts

[Dan Williams]

On Mon, Dec 6, 2021 at 2:46 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 3 Dec 2021 11:29:06 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Device status can change without warning at any point in time. This
> > effectively means that no amount of status checking before a command is
> > submitted can guarantee that the device is not in an error condition
> > when the command is later submitted. The clearest signal that a device
> > is not able to process commands is if it fails to process commands.
> >
> > With the above understanding in hand, update cxl_pci_setup_mailbox() to
> > validate the readiness of the mailbox once at the beginning of time, and
> > then use timeouts and busy sequencing errors as the only occasions to
> > report status.
> >
> > Just as before, unless and until the driver gains a reset recovery path,
> > doorbell clearing failures by the device are fatal to mailbox
> > operations.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Just some bike-shedding on naming - up to you whether you think it is worth a tweak.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Thanks,
>
> J
> > ---
> >  drivers/cxl/pci.c |  138 ++++++++++++++---------------------------------------
> >  1 file changed, 36 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 519795432708..36f80437a11a 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -72,14 +72,16 @@ static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
> >       return 0;
> >  }
> >
> > -static void cxl_pci_mbox_timeout(struct cxl_dev_state *cxlds,
> > -                              struct cxl_mbox_cmd *mbox_cmd)
> > -{
> > -     struct device *dev = cxlds->dev;
> > +#define report_status(dev, status, msg)                                        \
> > +     dev_err_ratelimited(dev, msg ", device state %s%s\n",                  \
> > +                         status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
> > +                         status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
> >
> > -     dev_dbg(dev, "Mailbox command (opcode: %#x size: %zub) timed out\n",
> > -             mbox_cmd->opcode, mbox_cmd->size_in);
> > -}
> > +#define report_cmd_status(dev, cmd, status, msg)                               \
>
> Naming wise, can we hint that this is an error print?  maybe
> report_err_cmd_status() or similar?

Sure, I'll go with cxl_err() and cxl_cmd_err().