[PATCH 0/5] cxl/mbox: Robustify handling of mbox_cmd.return_code

[PATCH 0/5] cxl/mbox: Robustify handling of mbox_cmd.return_code

[Davidlohr Bueso]

Hello,

These are some patchlets that came up while going through the code.

Currently the return_code from a completed mbox command is handled as
either successful or not. This series teaches the driver to better deal
with the different returns from the hardware, allowing better debugging
and mapping to proper kernel errno semantics (which are left unchanged
for now) as well as more ad-hoc handling.

Patches 1 and 2 are small nits.
Patch 3, 4 and 5 implement and use the new calls.

Applies on top of linux-cxl's next branch but have not really been
very tested as I'm not sure how to actually send mbox commands but
thought I'd post regardless.

Davidlohr Bueso (5):
  cxl/mbox: Move mbox_mutex usage comment
  cxl/pci: Use CXL_MBOX_SUCCESS to check against mbox_cmd return code
  cxl/mbox: Improve handling of mbox_cmd return codes
  cxl/mbox: Use new return_code handling
  cxl/mbox: Retry sending mbox command upon RETRY return_code

 drivers/cxl/core/mbox.c | 23 +++++++----
 drivers/cxl/cxlmem.h    | 90 ++++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/pci.c       |  7 ++--
 3 files changed, 108 insertions(+), 12 deletions(-)

--
2.26.2

[PATCH 1/5] cxl/mbox: Move mbox_mutex usage comment

[Davidlohr Bueso]

... this is better served in the callback that actually
grabs the lock.

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

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index be61a0d8016b..778b04a0fb0a 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -136,7 +136,7 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
  * @out: Caller allocated buffer for the output.
  * @out_size: Expected size of output.
  *
- * Context: Any context. Will acquire and release mbox_mutex.
+ * Context: Any context.
  * Return:
  *  * %>=0	- Number of bytes returned in @out.
  *  * %-E2BIG	- Payload is too large for hardware.
@@ -165,6 +165,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 	if (out_size > cxlds->payload_size)
 		return -E2BIG;
 
+	/* acquire and releases the mbox_mutex */
 	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
 	if (rc)
 		return rc;
-- 
2.26.2

[PATCH 2/5] cxl/pci: Use CXL_MBOX_SUCCESS to check against mbox_cmd return code

[Davidlohr Bueso]

Also mention the need for the caller to check against any
errors from the hardware in return_code.

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

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 8a7267d116b7..c77e06aff8dc 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -177,9 +177,9 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 	mbox_cmd->return_code =
 		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
 
-	if (mbox_cmd->return_code != 0) {
+	if (mbox_cmd->return_code != CXL_MBOX_SUCCESS) {
 		dev_dbg(dev, "Mailbox operation had an error\n");
-		return 0;
+		return 0;  /* completed but caller must check return_code */
 	}
 
 	/* #7 */
-- 
2.26.2

[PATCH 3/5] cxl/mbox: Improve handling of mbox_cmd return codes

[Davidlohr Bueso]

Upon a completed command the caller is still expected to check
the actual return_code register for to ensure it succeed. This
adds, per the spec, the potential command return codes. It maps
the hardware return code with the kernel's errno style, and by
default continues to use -ENXIO (Command completed, but device
reported an error).

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c |  2 +-
 drivers/cxl/cxlmem.h    | 90 ++++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/pci.c       |  2 +-
 3 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 778b04a0fb0a..d4d4a16820b7 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -171,7 +171,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 		return rc;
 
 	/* TODO: Map return code to proper kernel style errno */
-	if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
+	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
 		return -ENXIO;
 
 	/*
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 5d33ce24fe09..268597d1ff33 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -85,9 +85,97 @@ struct cxl_mbox_cmd {
 	size_t size_in;
 	size_t size_out;
 	u16 return_code;
-#define CXL_MBOX_SUCCESS 0
 };
 
+/*
+ * Per CXL 2.0 Section 8.2.8.4.5.1
+ */
+enum {
+	CXL_MBOX_CMD_RC_SUCCESS = 0,
+	CXL_MBOX_CMD_RC_BACKGROUND,
+	CXL_MBOX_CMD_RC_INVINPUT,
+	CXL_MBOX_CMD_RC_UNSUPPORTED,
+	CXL_MBOX_CMD_RC_INTERNAL,
+	CXL_MBOX_CMD_RC_RETRY,
+	CXL_MBOX_CMD_RC_BUSY,
+	CXL_MBOX_CMD_RC_MEDIADISABLED,
+	CXL_MBOX_CMD_RC_FWINPROGRESS,
+	CXL_MBOX_CMD_RC_FWOOO,
+	CXL_MBOX_CMD_RC_FWAUTH,
+	CXL_MBOX_CMD_RC_INVSLOT,
+	CXL_MBOX_CMD_RC_FWROLLBACK,
+	CXL_MBOX_CMD_RC_FWRESET,
+	CXL_MBOX_CMD_RC_INVHANDLE,
+	CXL_MBOX_CMD_RC_INVPA,
+	CXL_MBOX_CMD_RC_INJPOISON,
+	CXL_MBOX_CMD_RC_MEDIAFAILURE,
+	CXL_MBOX_CMD_RC_ABORT,
+	CXL_MBOX_CMD_RC_INVSECURITY,
+	CXL_MBOX_CMD_RC_PASSPHRASE,
+	CXL_MBOX_CMD_RC_MBOXUNSUPPORTED,
+	CXL_MBOX_CMD_RC_INVPAYLOAD,
+};
+
+#define CXL_CMD_RC(_value, errno, str)		\
+	[CXL_MBOX_CMD_RC_##_value] = {		\
+		.err = errno,			\
+		.desc =	str,			\
+	}
+
+/*
+ * mbox cmd hardware return_code handling
+ */
+struct cxl_mbox_cmd_rc {
+	int err; /* proper kernel style errno */
+	const char *desc;
+};
+
+static const struct cxl_mbox_cmd_rc cxl_mbox_cmd_rctable[] = {
+	CXL_CMD_RC(SUCCESS, 0, NULL),
+	CXL_CMD_RC(BACKGROUND, ENXIO, "background cmd started successfully"),
+	/* errors here on out */
+	CXL_CMD_RC(INVINPUT, ENXIO, "cmd input was invalid"),
+	CXL_CMD_RC(UNSUPPORTED, ENXIO, "cmd is not supported"),
+	CXL_CMD_RC(INTERNAL, ENXIO, "internal device error"),
+	CXL_CMD_RC(RETRY, ENXIO, "temporary error, retry once"),
+	CXL_CMD_RC(BUSY, ENXIO, "ongoing background operation"),
+	CXL_CMD_RC(MEDIADISABLED, ENXIO, "media access is disabled"),
+	CXL_CMD_RC(FWINPROGRESS, ENXIO,
+			"only one FW package can be transferred at a time"),
+	CXL_CMD_RC(FWOOO, ENXIO,
+			"FW package content was transferred out of order"),
+	CXL_CMD_RC(FWAUTH, ENXIO, "FW package authentication failed"),
+	CXL_CMD_RC(INVSLOT, ENXIO,
+			"FW slot specified is not supported for requested op"),
+	CXL_CMD_RC(FWROLLBACK, ENXIO,
+			"rolled back to the previous active FW"),
+	CXL_CMD_RC(FWRESET, ENXIO, "FW failed to activate, needs cold reset"),
+	CXL_CMD_RC(INVHANDLE, ENXIO,
+			"one or more Event Record Handles were invalid"),
+	CXL_CMD_RC(INVPA, ENXIO, "physical address specified is invalid"),
+	CXL_CMD_RC(INJPOISON, ENXIO,
+			"allowed poison injection has been reached"),
+	CXL_CMD_RC(MEDIAFAILURE, ENXIO, "permanent issue with the media"),
+	CXL_CMD_RC(ABORT, ENXIO, "background cmd was aborted by device"),
+	CXL_CMD_RC(INVSECURITY, ENXIO,
+		   "not valid in the current security state"),
+	CXL_CMD_RC(PASSPHRASE, ENXIO,
+			"passphrase doesn't match current passphrase"),
+	CXL_CMD_RC(MBOXUNSUPPORTED, ENXIO,
+			"not supported on the mailbox it was issued on"),
+	CXL_CMD_RC(INVPAYLOAD, ENXIO, "invalid payload length"),
+};
+
+static inline const char *cxl_mbox_cmd_rc2str(struct cxl_mbox_cmd *mbox_cmd)
+{
+	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].desc;
+}
+
+static inline int cxl_mbox_cmd_rc2errno(struct cxl_mbox_cmd *mbox_cmd)
+{
+	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].err;
+}
+
 /*
  * CXL 2.0 - Memory capacity multiplier
  * See Section 8.2.9.5
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index c77e06aff8dc..0c36d111232b 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -177,7 +177,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 	mbox_cmd->return_code =
 		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
 
-	if (mbox_cmd->return_code != CXL_MBOX_SUCCESS) {
+	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
 		dev_dbg(dev, "Mailbox operation had an error\n");
 		return 0;  /* completed but caller must check return_code */
 	}
-- 
2.26.2

[PATCH 4/5] cxl/mbox: Use new return_code handling

[Davidlohr Bueso]

Use the global cxl_mbox_cmd_rc table to improve debug messaging
in __cxl_pci_mbox_send_cmd() and allow cxl_mbox_send_cmd()
to map to proper kernel style errno codes - this patch
continues to use -ENXIO only so no change in semantics.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c | 7 ++++---
 drivers/cxl/pci.c       | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d4d4a16820b7..fa9e7043f158 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -170,9 +170,10 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 	if (rc)
 		return rc;
 
-	/* TODO: Map return code to proper kernel style errno */
-	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
-		return -ENXIO;
+	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS) {
+		int err = cxl_mbox_cmd_rc2errno(&mbox_cmd);
+		return -err;
+	}
 
 	/*
 	 * Variable sized commands can't be validated and so it's up to the
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 0c36d111232b..67ec0220826f 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -178,7 +178,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
 
 	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
-		dev_dbg(dev, "Mailbox operation had an error\n");
+		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 */
 	}
 
-- 
2.26.2

[PATCH 5/5] cxl/mbox: Retry sending mbox command upon RETRY return_code

[Davidlohr Bueso]

As mentioned in CXL 2.0 specs: the hardware can set a Retry
Required if the the command was not completed due to a
temporary error. An optional single retry may resolve the
issue. This is the only retry case as in general they are
not recommended for commands that return an error.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index fa9e7043f158..567987052b68 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -160,20 +160,25 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 		.size_out = out_size,
 		.payload_out = out,
 	};
-	int rc;
+	int rc, retry = 1;
 
 	if (out_size > cxlds->payload_size)
 		return -E2BIG;
 
-	/* acquire and releases the mbox_mutex */
-	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
-	if (rc)
-		return rc;
+	do {
+		/* acquire and releases the mbox_mutex */
+		rc = cxlds->mbox_send(cxlds, &mbox_cmd);
+		if (rc)
+			return rc;
 
-	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS) {
-		int err = cxl_mbox_cmd_rc2errno(&mbox_cmd);
-		return -err;
-	}
+		if (mbox_cmd.return_code == CXL_MBOX_CMD_RC_SUCCESS)
+			break;
+
+		if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_RETRY || !retry) {
+			int err = cxl_mbox_cmd_rc2errno(&mbox_cmd);
+			return -err;
+		}
+	} while (retry--);
 
 	/*
 	 * Variable sized commands can't be validated and so it's up to the
-- 
2.26.2

Re: [PATCH 1/5] cxl/mbox: Move mbox_mutex usage comment

[Adam Manzanares]

On Thu, Mar 17, 2022 at 04:40:45PM -0700, Davidlohr Bueso wrote:
> ... this is better served in the callback that actually
> grabs the lock.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index be61a0d8016b..778b04a0fb0a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -136,7 +136,7 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
>   * @out: Caller allocated buffer for the output.
>   * @out_size: Expected size of output.
>   *
> - * Context: Any context. Will acquire and release mbox_mutex.
> + * Context: Any context.
>   * Return:
>   *  * %>=0	- Number of bytes returned in @out.
>   *  * %-E2BIG	- Payload is too large for hardware.
> @@ -165,6 +165,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>  	if (out_size > cxlds->payload_size)
>  		return -E2BIG;
>  
> +	/* acquire and releases the mbox_mutex */
>  	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
>  	if (rc)
>  		return rc;
> 
> -- 
> 2.26.2
> 

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>

Re: [PATCH 2/5] cxl/pci: Use CXL_MBOX_SUCCESS to check against mbox_cmd return code

[Adam Manzanares]

On Thu, Mar 17, 2022 at 04:40:46PM -0700, Davidlohr Bueso wrote:
> Also mention the need for the caller to check against any
> errors from the hardware in return_code.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8a7267d116b7..c77e06aff8dc 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -177,9 +177,9 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  	mbox_cmd->return_code =
>  		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>  
> -	if (mbox_cmd->return_code != 0) {
> +	if (mbox_cmd->return_code != CXL_MBOX_SUCCESS) {
>  		dev_dbg(dev, "Mailbox operation had an error\n");
> -		return 0;
> +		return 0;  /* completed but caller must check return_code */
>  	}
>  
>  	/* #7 */
> 
> -- 
> 2.26.2
> 

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>

Re: [PATCH 3/5] cxl/mbox: Improve handling of mbox_cmd return codes

[Adam Manzanares]

On Thu, Mar 17, 2022 at 04:40:47PM -0700, Davidlohr Bueso wrote:
> Upon a completed command the caller is still expected to check
> the actual return_code register for to ensure it succeed. This

Is  MB Status register == return_code register here. s/for to/to
> adds, per the spec, the potential command return codes. It maps
> the hardware return code with the kernel's errno style, and by
> default continues to use -ENXIO (Command completed, but device
> reported an error).
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c |  2 +-
>  drivers/cxl/cxlmem.h    | 90 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/cxl/pci.c       |  2 +-
>  3 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 778b04a0fb0a..d4d4a16820b7 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -171,7 +171,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>  		return rc;
>  
>  	/* TODO: Map return code to proper kernel style errno */
> -	if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
> +	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
>  		return -ENXIO;
>  
>  	/*
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 5d33ce24fe09..268597d1ff33 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -85,9 +85,97 @@ struct cxl_mbox_cmd {
>  	size_t size_in;
>  	size_t size_out;
>  	u16 return_code;
> -#define CXL_MBOX_SUCCESS 0
>  };
>  
> +/*
> + * Per CXL 2.0 Section 8.2.8.4.5.1
> + */
> +enum {
> +	CXL_MBOX_CMD_RC_SUCCESS = 0,
> +	CXL_MBOX_CMD_RC_BACKGROUND,
> +	CXL_MBOX_CMD_RC_INVINPUT,
> +	CXL_MBOX_CMD_RC_UNSUPPORTED,
> +	CXL_MBOX_CMD_RC_INTERNAL,
> +	CXL_MBOX_CMD_RC_RETRY,
> +	CXL_MBOX_CMD_RC_BUSY,
> +	CXL_MBOX_CMD_RC_MEDIADISABLED,
> +	CXL_MBOX_CMD_RC_FWINPROGRESS,
> +	CXL_MBOX_CMD_RC_FWOOO,
> +	CXL_MBOX_CMD_RC_FWAUTH,
> +	CXL_MBOX_CMD_RC_INVSLOT,

Should we keep the FW prefix for INVSLOT, since this is still related to a 
FW operation?

> +	CXL_MBOX_CMD_RC_FWROLLBACK,
> +	CXL_MBOX_CMD_RC_FWRESET,
> +	CXL_MBOX_CMD_RC_INVHANDLE,
> +	CXL_MBOX_CMD_RC_INVPA,
> +	CXL_MBOX_CMD_RC_INJPOISON,

We should indicate that we hit the poison limit, something like the 
following POISONLMT

> +	CXL_MBOX_CMD_RC_MEDIAFAILURE,
> +	CXL_MBOX_CMD_RC_ABORT,
> +	CXL_MBOX_CMD_RC_INVSECURITY,
> +	CXL_MBOX_CMD_RC_PASSPHRASE,
> +	CXL_MBOX_CMD_RC_MBOXUNSUPPORTED,
> +	CXL_MBOX_CMD_RC_INVPAYLOAD,

Might as well be more descriptive here PAYLOADLEN, looking at this more now
maybe we drop all of the INV prefixes since most of the return codes are error
codes. 

> +};
> +
> +#define CXL_CMD_RC(_value, errno, str)		\
> +	[CXL_MBOX_CMD_RC_##_value] = {		\
> +		.err = errno,			\
> +		.desc =	str,			\
> +	}
> +
> +/*
> + * mbox cmd hardware return_code handling
> + */
> +struct cxl_mbox_cmd_rc {
> +	int err; /* proper kernel style errno */
> +	const char *desc;
> +};
> +
> +static const struct cxl_mbox_cmd_rc cxl_mbox_cmd_rctable[] = {
> +	CXL_CMD_RC(SUCCESS, 0, NULL),
> +	CXL_CMD_RC(BACKGROUND, ENXIO, "background cmd started successfully"),
> +	/* errors here on out */
> +	CXL_CMD_RC(INVINPUT, ENXIO, "cmd input was invalid"),
> +	CXL_CMD_RC(UNSUPPORTED, ENXIO, "cmd is not supported"),
> +	CXL_CMD_RC(INTERNAL, ENXIO, "internal device error"),
> +	CXL_CMD_RC(RETRY, ENXIO, "temporary error, retry once"),
> +	CXL_CMD_RC(BUSY, ENXIO, "ongoing background operation"),
> +	CXL_CMD_RC(MEDIADISABLED, ENXIO, "media access is disabled"),
> +	CXL_CMD_RC(FWINPROGRESS, ENXIO,
> +			"only one FW package can be transferred at a time"),
> +	CXL_CMD_RC(FWOOO, ENXIO,
> +			"FW package content was transferred out of order"),
> +	CXL_CMD_RC(FWAUTH, ENXIO, "FW package authentication failed"),
> +	CXL_CMD_RC(INVSLOT, ENXIO,
> +			"FW slot specified is not supported for requested op"),
> +	CXL_CMD_RC(FWROLLBACK, ENXIO,
> +			"rolled back to the previous active FW"),
> +	CXL_CMD_RC(FWRESET, ENXIO, "FW failed to activate, needs cold reset"),
> +	CXL_CMD_RC(INVHANDLE, ENXIO,
> +			"one or more Event Record Handles were invalid"),
> +	CXL_CMD_RC(INVPA, ENXIO, "physical address specified is invalid"),
> +	CXL_CMD_RC(INJPOISON, ENXIO,
> +			"allowed poison injection has been reached"),

injection limit

> +	CXL_CMD_RC(MEDIAFAILURE, ENXIO, "permanent issue with the media"),
> +	CXL_CMD_RC(ABORT, ENXIO, "background cmd was aborted by device"),
> +	CXL_CMD_RC(INVSECURITY, ENXIO,
> +		   "not valid in the current security state"),
> +	CXL_CMD_RC(PASSPHRASE, ENXIO,
> +			"passphrase doesn't match current passphrase"),

current set passphrase

> +	CXL_CMD_RC(MBOXUNSUPPORTED, ENXIO,
> +			"not supported on the mailbox it was issued on"),
> +	CXL_CMD_RC(INVPAYLOAD, ENXIO, "invalid payload length"),
> +};
> +
> +static inline const char *cxl_mbox_cmd_rc2str(struct cxl_mbox_cmd *mbox_cmd)
> +{
> +	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].desc;
> +}
> +
> +static inline int cxl_mbox_cmd_rc2errno(struct cxl_mbox_cmd *mbox_cmd)
> +{
> +	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].err;
> +}
> +
>  /*
>   * CXL 2.0 - Memory capacity multiplier
>   * See Section 8.2.9.5
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index c77e06aff8dc..0c36d111232b 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -177,7 +177,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  	mbox_cmd->return_code =
>  		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>  
> -	if (mbox_cmd->return_code != CXL_MBOX_SUCCESS) {
> +	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
>  		dev_dbg(dev, "Mailbox operation had an error\n");
>  		return 0;  /* completed but caller must check return_code */
>  	}
> 
> -- 
> 2.26.2
>

Other than the minor nits. 
Looks good.

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>

Re: [PATCH 4/5] cxl/mbox: Use new return_code handling

[Adam Manzanares]

On Thu, Mar 17, 2022 at 04:40:48PM -0700, Davidlohr Bueso wrote:
> Use the global cxl_mbox_cmd_rc table to improve debug messaging
> in __cxl_pci_mbox_send_cmd() and allow cxl_mbox_send_cmd()
> to map to proper kernel style errno codes - this patch
> continues to use -ENXIO only so no change in semantics.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c | 7 ++++---
>  drivers/cxl/pci.c       | 3 ++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d4d4a16820b7..fa9e7043f158 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -170,9 +170,10 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>  	if (rc)
>  		return rc;
>  
> -	/* TODO: Map return code to proper kernel style errno */
> -	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
> -		return -ENXIO;
> +	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> +		int err = cxl_mbox_cmd_rc2errno(&mbox_cmd);
> +		return -err;
> +	}
>  
>  	/*
>  	 * Variable sized commands can't be validated and so it's up to the
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0c36d111232b..67ec0220826f 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -178,7 +178,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>  		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>  
>  	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> -		dev_dbg(dev, "Mailbox operation had an error\n");
> +		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 */
>  	}
>  
> 
> -- 
> 2.26.2
> 

Looks good.

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>

Re: [PATCH 5/5] cxl/mbox: Retry sending mbox command upon RETRY return_code

[Adam Manzanares]

On Thu, Mar 17, 2022 at 04:40:49PM -0700, Davidlohr Bueso wrote:
> As mentioned in CXL 2.0 specs: the hardware can set a Retry
> Required if the the command was not completed due to a
> temporary error. An optional single retry may resolve the
> issue. This is the only retry case as in general they are
> not recommended for commands that return an error.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index fa9e7043f158..567987052b68 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -160,20 +160,25 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,

Can you update the documentation cxl_mbox_send_cmd that the function now 
retries commands if the RC indicates it should be done?

>  		.size_out = out_size,
>  		.payload_out = out,
>  	};
> -	int rc;
> +	int rc, retry = 1;
>  
>  	if (out_size > cxlds->payload_size)
>  		return -E2BIG;
>  
> -	/* acquire and releases the mbox_mutex */
> -	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
> -	if (rc)
> -		return rc;
> +	do {
> +		/* acquire and releases the mbox_mutex */
> +		rc = cxlds->mbox_send(cxlds, &mbox_cmd);
> +		if (rc)
> +			return rc;
>  
> -	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> -		int err = cxl_mbox_cmd_rc2errno(&mbox_cmd);
> -		return -err;
> -	}
> +		if (mbox_cmd.return_code == CXL_MBOX_CMD_RC_SUCCESS)
> +			break;
> +
> +		if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_RETRY || !retry) {
> +			int err = cxl_mbox_cmd_rc2errno(&mbox_cmd);
> +			return -err;
> +		}
> +	} while (retry--);
>  
>  	/*
>  	 * Variable sized commands can't be validated and so it's up to the
> 
> -- 
> 2.26.2
>

Other than the doc change for the cxl_mbox_send_cmd function.
LGTM

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>

Re: [PATCH 3/5] cxl/mbox: Improve handling of mbox_cmd return codes

[Davidlohr Bueso]

On Tue, 22 Mar 2022, Adam Manzanares wrote:

>On Thu, Mar 17, 2022 at 04:40:47PM -0700, Davidlohr Bueso wrote:
>> Upon a completed command the caller is still expected to check
>> the actual return_code register for to ensure it succeed. This
>
>Is  MB Status register == return_code register here. s/for to/to
>> adds, per the spec, the potential command return codes. It maps
>> the hardware return code with the kernel's errno style, and by
>> default continues to use -ENXIO (Command completed, but device
>> reported an error).
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>> ---
>>  drivers/cxl/core/mbox.c |  2 +-
>>  drivers/cxl/cxlmem.h    | 90 ++++++++++++++++++++++++++++++++++++++++-
>>  drivers/cxl/pci.c       |  2 +-
>>  3 files changed, 91 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index 778b04a0fb0a..d4d4a16820b7 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -171,7 +171,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>>		return rc;
>>
>>	/* TODO: Map return code to proper kernel style errno */
>> -	if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
>> +	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
>>		return -ENXIO;
>>
>>	/*
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index 5d33ce24fe09..268597d1ff33 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -85,9 +85,97 @@ struct cxl_mbox_cmd {
>>	size_t size_in;
>>	size_t size_out;
>>	u16 return_code;
>> -#define CXL_MBOX_SUCCESS 0
>>  };
>>
>> +/*
>> + * Per CXL 2.0 Section 8.2.8.4.5.1
>> + */
>> +enum {
>> +	CXL_MBOX_CMD_RC_SUCCESS = 0,
>> +	CXL_MBOX_CMD_RC_BACKGROUND,
>> +	CXL_MBOX_CMD_RC_INVINPUT,
>> +	CXL_MBOX_CMD_RC_UNSUPPORTED,
>> +	CXL_MBOX_CMD_RC_INTERNAL,
>> +	CXL_MBOX_CMD_RC_RETRY,
>> +	CXL_MBOX_CMD_RC_BUSY,
>> +	CXL_MBOX_CMD_RC_MEDIADISABLED,
>> +	CXL_MBOX_CMD_RC_FWINPROGRESS,
>> +	CXL_MBOX_CMD_RC_FWOOO,
>> +	CXL_MBOX_CMD_RC_FWAUTH,
>> +	CXL_MBOX_CMD_RC_INVSLOT,
>
>Should we keep the FW prefix for INVSLOT, since this is still related to a
>FW operation?
>
>> +	CXL_MBOX_CMD_RC_FWROLLBACK,
>> +	CXL_MBOX_CMD_RC_FWRESET,
>> +	CXL_MBOX_CMD_RC_INVHANDLE,
>> +	CXL_MBOX_CMD_RC_INVPA,
>> +	CXL_MBOX_CMD_RC_INJPOISON,
>
>We should indicate that we hit the poison limit, something like the
>following POISONLMT
>
>> +	CXL_MBOX_CMD_RC_MEDIAFAILURE,
>> +	CXL_MBOX_CMD_RC_ABORT,
>> +	CXL_MBOX_CMD_RC_INVSECURITY,
>> +	CXL_MBOX_CMD_RC_PASSPHRASE,
>> +	CXL_MBOX_CMD_RC_MBOXUNSUPPORTED,
>> +	CXL_MBOX_CMD_RC_INVPAYLOAD,
>
>Might as well be more descriptive here PAYLOADLEN, looking at this more now
>maybe we drop all of the INV prefixes since most of the return codes are error
>codes.

Yes to all the above.

>
>> +};
>> +
>> +#define CXL_CMD_RC(_value, errno, str)		\
>> +	[CXL_MBOX_CMD_RC_##_value] = {		\
>> +		.err = errno,			\
>> +		.desc =	str,			\
>> +	}
>> +
>> +/*
>> + * mbox cmd hardware return_code handling
>> + */
>> +struct cxl_mbox_cmd_rc {
>> +	int err; /* proper kernel style errno */
>> +	const char *desc;
>> +};
>> +
>> +static const struct cxl_mbox_cmd_rc cxl_mbox_cmd_rctable[] = {
>> +	CXL_CMD_RC(SUCCESS, 0, NULL),
>> +	CXL_CMD_RC(BACKGROUND, ENXIO, "background cmd started successfully"),
>> +	/* errors here on out */
>> +	CXL_CMD_RC(INVINPUT, ENXIO, "cmd input was invalid"),
>> +	CXL_CMD_RC(UNSUPPORTED, ENXIO, "cmd is not supported"),
>> +	CXL_CMD_RC(INTERNAL, ENXIO, "internal device error"),
>> +	CXL_CMD_RC(RETRY, ENXIO, "temporary error, retry once"),
>> +	CXL_CMD_RC(BUSY, ENXIO, "ongoing background operation"),
>> +	CXL_CMD_RC(MEDIADISABLED, ENXIO, "media access is disabled"),
>> +	CXL_CMD_RC(FWINPROGRESS, ENXIO,
>> +			"only one FW package can be transferred at a time"),
>> +	CXL_CMD_RC(FWOOO, ENXIO,
>> +			"FW package content was transferred out of order"),
>> +	CXL_CMD_RC(FWAUTH, ENXIO, "FW package authentication failed"),
>> +	CXL_CMD_RC(INVSLOT, ENXIO,
>> +			"FW slot specified is not supported for requested op"),
>> +	CXL_CMD_RC(FWROLLBACK, ENXIO,
>> +			"rolled back to the previous active FW"),
>> +	CXL_CMD_RC(FWRESET, ENXIO, "FW failed to activate, needs cold reset"),
>> +	CXL_CMD_RC(INVHANDLE, ENXIO,
>> +			"one or more Event Record Handles were invalid"),
>> +	CXL_CMD_RC(INVPA, ENXIO, "physical address specified is invalid"),
>> +	CXL_CMD_RC(INJPOISON, ENXIO,
>> +			"allowed poison injection has been reached"),
>
>injection limit
>
>> +	CXL_CMD_RC(MEDIAFAILURE, ENXIO, "permanent issue with the media"),
>> +	CXL_CMD_RC(ABORT, ENXIO, "background cmd was aborted by device"),
>> +	CXL_CMD_RC(INVSECURITY, ENXIO,
>> +		   "not valid in the current security state"),
>> +	CXL_CMD_RC(PASSPHRASE, ENXIO,
>> +			"passphrase doesn't match current passphrase"),
>
>current set passphrase
>
>> +	CXL_CMD_RC(MBOXUNSUPPORTED, ENXIO,
>> +			"not supported on the mailbox it was issued on"),
>> +	CXL_CMD_RC(INVPAYLOAD, ENXIO, "invalid payload length"),
>> +};
>
>Other than the minor nits.
>Looks good.

Thanks for reviewing. I'll add the changes in a v2 but will wait a bit
for any further feedback.

Re: [PATCH 1/5] cxl/mbox: Move mbox_mutex usage comment

[Dan Williams]

On Thu, Mar 17, 2022 at 4:41 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> ... this is better served in the callback that actually
> grabs the lock.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index be61a0d8016b..778b04a0fb0a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -136,7 +136,7 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
>   * @out: Caller allocated buffer for the output.
>   * @out_size: Expected size of output.
>   *
> - * Context: Any context. Will acquire and release mbox_mutex.
> + * Context: Any context.
>   * Return:
>   *  * %>=0     - Number of bytes returned in @out.
>   *  * %-E2BIG  - Payload is too large for hardware.
> @@ -165,6 +165,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>         if (out_size > cxlds->payload_size)
>                 return -E2BIG;
>
> +       /* acquire and releases the mbox_mutex */

I'm not even sure this comment is necessary, because that's what
lockdep is for, and it's also not true for cxl_mock_mbox_send().
Nothing outside of the ->mbox_send() implementation should be assuming
that ->mbox_send() is enforcing serialization. So let's just drop this
second hunk.

Re: [PATCH 2/5] cxl/pci: Use CXL_MBOX_SUCCESS to check against mbox_cmd return code

[Dan Williams]

On Thu, Mar 17, 2022 at 4:41 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Also mention the need for the caller to check against any
> errors from the hardware in return_code.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8a7267d116b7..c77e06aff8dc 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -177,9 +177,9 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>         mbox_cmd->return_code =
>                 FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>
> -       if (mbox_cmd->return_code != 0) {
> +       if (mbox_cmd->return_code != CXL_MBOX_SUCCESS) {
>                 dev_dbg(dev, "Mailbox operation had an error\n");
> -               return 0;
> +               return 0;  /* completed but caller must check return_code */

Looks like a good clarification to me.

Applied.

...but note it won't appear anywhere until after v5.18-rc1 is out.

Re: [PATCH 3/5] cxl/mbox: Improve handling of mbox_cmd return codes

[Dan Williams]

On Thu, Mar 17, 2022 at 4:41 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Upon a completed command the caller is still expected to check
> the actual return_code register for to ensure it succeed. This
> adds, per the spec, the potential command return codes. It maps
> the hardware return code with the kernel's errno style, and by
> default continues to use -ENXIO (Command completed, but device
> reported an error).
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c |  2 +-
>  drivers/cxl/cxlmem.h    | 90 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/cxl/pci.c       |  2 +-
>  3 files changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 778b04a0fb0a..d4d4a16820b7 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -171,7 +171,7 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>                 return rc;
>
>         /* TODO: Map return code to proper kernel style errno */
> -       if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
> +       if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
>                 return -ENXIO;
>
>         /*
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 5d33ce24fe09..268597d1ff33 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -85,9 +85,97 @@ struct cxl_mbox_cmd {
>         size_t size_in;
>         size_t size_out;
>         u16 return_code;
> -#define CXL_MBOX_SUCCESS 0
>  };
>
> +/*
> + * Per CXL 2.0 Section 8.2.8.4.5.1
> + */
> +enum {
> +       CXL_MBOX_CMD_RC_SUCCESS = 0,
> +       CXL_MBOX_CMD_RC_BACKGROUND,
> +       CXL_MBOX_CMD_RC_INVINPUT,
> +       CXL_MBOX_CMD_RC_UNSUPPORTED,
> +       CXL_MBOX_CMD_RC_INTERNAL,
> +       CXL_MBOX_CMD_RC_RETRY,
> +       CXL_MBOX_CMD_RC_BUSY,
> +       CXL_MBOX_CMD_RC_MEDIADISABLED,
> +       CXL_MBOX_CMD_RC_FWINPROGRESS,
> +       CXL_MBOX_CMD_RC_FWOOO,
> +       CXL_MBOX_CMD_RC_FWAUTH,
> +       CXL_MBOX_CMD_RC_INVSLOT,
> +       CXL_MBOX_CMD_RC_FWROLLBACK,
> +       CXL_MBOX_CMD_RC_FWRESET,
> +       CXL_MBOX_CMD_RC_INVHANDLE,
> +       CXL_MBOX_CMD_RC_INVPA,
> +       CXL_MBOX_CMD_RC_INJPOISON,
> +       CXL_MBOX_CMD_RC_MEDIAFAILURE,
> +       CXL_MBOX_CMD_RC_ABORT,
> +       CXL_MBOX_CMD_RC_INVSECURITY,
> +       CXL_MBOX_CMD_RC_PASSPHRASE,
> +       CXL_MBOX_CMD_RC_MBOXUNSUPPORTED,
> +       CXL_MBOX_CMD_RC_INVPAYLOAD,
> +};
> +
> +#define CXL_CMD_RC(_value, errno, str)         \
> +       [CXL_MBOX_CMD_RC_##_value] = {          \
> +               .err = errno,                   \
> +               .desc = str,                    \
> +       }
> +
> +/*
> + * mbox cmd hardware return_code handling
> + */
> +struct cxl_mbox_cmd_rc {
> +       int err; /* proper kernel style errno */
> +       const char *desc;
> +};
> +
> +static const struct cxl_mbox_cmd_rc cxl_mbox_cmd_rctable[] = {
> +       CXL_CMD_RC(SUCCESS, 0, NULL),
> +       CXL_CMD_RC(BACKGROUND, ENXIO, "background cmd started successfully"),
> +       /* errors here on out */
> +       CXL_CMD_RC(INVINPUT, ENXIO, "cmd input was invalid"),
> +       CXL_CMD_RC(UNSUPPORTED, ENXIO, "cmd is not supported"),
> +       CXL_CMD_RC(INTERNAL, ENXIO, "internal device error"),
> +       CXL_CMD_RC(RETRY, ENXIO, "temporary error, retry once"),
> +       CXL_CMD_RC(BUSY, ENXIO, "ongoing background operation"),
> +       CXL_CMD_RC(MEDIADISABLED, ENXIO, "media access is disabled"),
> +       CXL_CMD_RC(FWINPROGRESS, ENXIO,
> +                       "only one FW package can be transferred at a time"),
> +       CXL_CMD_RC(FWOOO, ENXIO,
> +                       "FW package content was transferred out of order"),
> +       CXL_CMD_RC(FWAUTH, ENXIO, "FW package authentication failed"),
> +       CXL_CMD_RC(INVSLOT, ENXIO,
> +                       "FW slot specified is not supported for requested op"),
> +       CXL_CMD_RC(FWROLLBACK, ENXIO,
> +                       "rolled back to the previous active FW"),
> +       CXL_CMD_RC(FWRESET, ENXIO, "FW failed to activate, needs cold reset"),
> +       CXL_CMD_RC(INVHANDLE, ENXIO,
> +                       "one or more Event Record Handles were invalid"),
> +       CXL_CMD_RC(INVPA, ENXIO, "physical address specified is invalid"),
> +       CXL_CMD_RC(INJPOISON, ENXIO,
> +                       "allowed poison injection has been reached"),
> +       CXL_CMD_RC(MEDIAFAILURE, ENXIO, "permanent issue with the media"),
> +       CXL_CMD_RC(ABORT, ENXIO, "background cmd was aborted by device"),
> +       CXL_CMD_RC(INVSECURITY, ENXIO,
> +                  "not valid in the current security state"),
> +       CXL_CMD_RC(PASSPHRASE, ENXIO,
> +                       "passphrase doesn't match current passphrase"),
> +       CXL_CMD_RC(MBOXUNSUPPORTED, ENXIO,
> +                       "not supported on the mailbox it was issued on"),
> +       CXL_CMD_RC(INVPAYLOAD, ENXIO, "invalid payload length"),
> +};

You can define the table and the enums together with Steven's trick [1]

#define CMD_CMD_RC_TABLE                                                    \
        C(SUCCESS,      0,      NULL),                                   \
        C(BACKGROUND,   -ENXIO, "background cmd started successfully"),  \
                ...
        C(PAYLOADLEN,   -ENXIO, "invalid payload length")
#undef C
#define C(a, b, c) CXL_CMD_RC_##a
enum  { CMD_CMD_RC_TABLE };
#undef C
#define C(a, b, c) { b, c }
static struct {
        int err;
        const char *desc;
} cxl_mbox_cmd_rctable[] = { OPS };
#undef C

Ideally you could align the columns so it's easy to read which
hardware code relates to which errno.

[1]: See "cpp tricks and treats": https://lwn.net/Articles/383362/

Re: [PATCH 2/5] cxl/pci: Use CXL_MBOX_SUCCESS to check against mbox_cmd return code

[Dan Williams]

On Tue, Mar 29, 2022 at 4:42 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Thu, Mar 17, 2022 at 4:41 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
> >
> > Also mention the need for the caller to check against any
> > errors from the hardware in return_code.
> >
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > ---
> >  drivers/cxl/pci.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 8a7267d116b7..c77e06aff8dc 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -177,9 +177,9 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> >         mbox_cmd->return_code =
> >                 FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
> >
> > -       if (mbox_cmd->return_code != 0) {
> > +       if (mbox_cmd->return_code != CXL_MBOX_SUCCESS) {
> >                 dev_dbg(dev, "Mailbox operation had an error\n");
> > -               return 0;
> > +               return 0;  /* completed but caller must check return_code */
>
> Looks like a good clarification to me.
>
> Applied.
>
> ...but note it won't appear anywhere until after v5.18-rc1 is out.

...although given there are already feedback comments on patch 1 and 3
I also don't mind if you resend the full series for v2.

Re: [PATCH 4/5] cxl/mbox: Use new return_code handling

[Dan Williams]

On Thu, Mar 17, 2022 at 4:41 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Use the global cxl_mbox_cmd_rc table to improve debug messaging
> in __cxl_pci_mbox_send_cmd() and allow cxl_mbox_send_cmd()
> to map to proper kernel style errno codes - this patch
> continues to use -ENXIO only so no change in semantics.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/mbox.c | 7 ++++---
>  drivers/cxl/pci.c       | 3 ++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d4d4a16820b7..fa9e7043f158 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -170,9 +170,10 @@ int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>         if (rc)
>                 return rc;
>
> -       /* TODO: Map return code to proper kernel style errno */
> -       if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
> -               return -ENXIO;
> +       if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> +               int err = cxl_mbox_cmd_rc2errno(&mbox_cmd);
> +               return -err;

I don't like the need for a local variable and adding the negation.
Just store the negative value directly and call the helper
cxl_mbox_cmd_rc2err() so that this can just do "return
cxl_mbox_cmd_rc2err(&mbox_cmd);".

> +       }
>
>         /*
>          * Variable sized commands can't be validated and so it's up to the
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0c36d111232b..67ec0220826f 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -178,7 +178,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>                 FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>
>         if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> -               dev_dbg(dev, "Mailbox operation had an error\n");
> +               dev_dbg(dev, "Mailbox operation had an error: %s\n",
> +                       cxl_mbox_cmd_rc2str(mbox_cmd));

Looks good.

Re: [PATCH 5/5] cxl/mbox: Retry sending mbox command upon RETRY return_code

[Dan Williams]

On Thu, Mar 17, 2022 at 4:41 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> As mentioned in CXL 2.0 specs: the hardware can set a Retry
> Required if the the command was not completed due to a
> temporary error. An optional single retry may resolve the
> issue. This is the only retry case as in general they are
> not recommended for commands that return an error.

So I disagree that the driver should be deploying this behaviour by
default. This was also tried for the ACPI NVDIMM commands and it was
helpful for Linux to not play along and get the firmware implementers
to get their act together and detail when a retry is appropriate. So,
this policy is up to the submitter. If that agent has information that
the kernel does not have about a retry being effective, it is free to
attempt the retry after the failure. Otherwise, the spec is overly
hopeful in expecting an implementation to attempt a speculative /
optional retry. Hopefully, a default Linux policy to fail commands
that end in a retry encourages fixes to devices / firmwares that want
to use optional retry as a crutch... or it encourages the discipline
to come back to the specification working group with explicit
per-command updates to define the scenarios when a retry is mandatory
and not optional.