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

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

[Davidlohr Bueso]

Hi,

Changes from v1 (https://lore.kernel.org/all/20220317234049.69323-1-dave@stgolabs.net/):
 - Patch 1 now just removes the lock comment, instead of moving it. (Dan)
 - Modified pach 3 to use CPP hackery + misc table improvements. (Dan and Adam)
 - Dropped patch 5 per concerns about implications with FW. (Dan)
 - Picked up Adam's review tags.

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.

Applies against linux-cxl's next branch.

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

Thanks!

Davidlohr Bueso (4):
  cxl/mbox: Drop mbox_mutex comment
  cxl/pci: Use CXL_MBOX_SUCCESS to check against mbox_cmd return code
  cxl/mbox: Improve handling of mbox_cmd hw return codes
  cxl/mbox: Use new return_code handling

 drivers/cxl/core/mbox.c |  7 +++---
 drivers/cxl/cxlmem.h    | 53 ++++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/pci.c       |  7 +++---
 3 files changed, 59 insertions(+), 8 deletions(-)

--
2.26.2

[PATCH 1/4] cxl/mbox: Drop mbox_mutex comment

[Davidlohr Bueso]

... we have lockdep for this.

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index be61a0d8016b..a586c0fa9555 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.
-- 
2.26.2

[PATCH 2/4] 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.

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>
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..8b131692ed61 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/4] cxl/mbox: Improve handling of mbox_cmd hw return codes

[Davidlohr Bueso]

Upon a completed command the caller is still expected to check
the actual return_code register 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).

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c |  2 +-
 drivers/cxl/cxlmem.h    | 53 ++++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/pci.c       |  2 +-
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index a586c0fa9555..353b3f97c7e6 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -170,7 +170,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..264a3cbfcad6 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -85,9 +85,60 @@ 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
+ */
+#define CMD_CMD_RC_TABLE							\
+	C(SUCCESS, 0, NULL),							\
+	C(BACKGROUND, -ENXIO, "background cmd started successfully"),           \
+	C(INPUT, -ENXIO, "cmd input was invalid"),				\
+	C(UNSUPPORTED, -ENXIO, "cmd is not supported"),				\
+	C(INTERNAL, -ENXIO, "internal device error"),				\
+	C(RETRY, -ENXIO, "temporary error, retry once"),			\
+	C(BUSY, -ENXIO, "ongoing background operation"),			\
+	C(MEDIADISABLED, -ENXIO, "media access is disabled"),			\
+	C(FWINPROGRESS, -ENXIO,	"one FW package can be transferred at a time"), \
+	C(FWOOO, -ENXIO, "FW package content was transferred out of order"),    \
+	C(FWAUTH, -ENXIO, "FW package authentication failed"),			\
+	C(FWSLOT, -ENXIO, "FW slot is not supported for requested operation"),  \
+	C(FWROLLBACK, -ENXIO, "rolled back to the previous active FW"),         \
+	C(FWRESET, -ENXIO, "FW failed to activate, needs cold reset"),		\
+	C(HANDLE, -ENXIO, "one or more Event Record Handles were invalid"),     \
+	C(PADDR, -ENXIO, "physical address specified is invalid"),		\
+	C(POISONLMT, -ENXIO, "poison injection limit has been reached"),        \
+	C(MEDIAFAILURE, -ENXIO, "permanent issue with the media"),		\
+	C(ABORT, -ENXIO, "background cmd was aborted by device"),               \
+	C(SECURITY, -ENXIO, "not valid in the current security state"),         \
+	C(PASSPHRASE, -ENXIO, "phrase doesn't match current set passphrase"),   \
+	C(MBUNSUPPORTED, -ENXIO, "unsupported on the mailbox it was issued on"),\
+	C(PAYLOADLEN, -ENXIO, "invalid payload length")
+
+#undef C
+#define C(a, b, c) CXL_MBOX_CMD_RC_##a
+enum  { CMD_CMD_RC_TABLE };
+#undef C
+#define C(a, b, c) { b, c }
+struct cxl_mbox_cmd_rc {
+	int err;
+	const char *desc;
+};
+
+static const
+struct cxl_mbox_cmd_rc cxl_mbox_cmd_rctable[] ={ CMD_CMD_RC_TABLE };
+#undef C
+
+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 8b131692ed61..4fbef05e9082 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/4] 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.

Reviewed by: Adam Manzanares <a.manzanares@samsung.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/core/mbox.c | 3 +--
 drivers/cxl/pci.c       | 3 ++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 353b3f97c7e6..d7286c256e1c 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -169,9 +169,8 @@ 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;
+		return cxl_mbox_cmd_rc2errno(&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 4fbef05e9082..18d31fafc856 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

Re: [PATCH v2 0/4] cxl/mbox: Robustify handling of mbox_cmd.return_code

[Dan Williams]

On Sun, Apr 3, 2022 at 7:12 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Hi,
>
> Changes from v1 (https://lore.kernel.org/all/20220317234049.69323-1-dave@stgolabs.net/):
>  - Patch 1 now just removes the lock comment, instead of moving it. (Dan)
>  - Modified pach 3 to use CPP hackery + misc table improvements. (Dan and Adam)
>  - Dropped patch 5 per concerns about implications with FW. (Dan)
>  - Picked up Adam's review tags.
>
> 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.
>
> Applies against linux-cxl's next branch.
>
> Patches 1 and 2 are small nits.
> Patch 3 and 4 implement and use the new calls.
>
> Thanks!

Looks good to me, applied to cxl/pending for 5.19