mi: Fix status extraction from cdw3 field

Currently, we extract the NVMe status value from the top two bytes of
cdw3 of the response.

However, the Status Code (SC) field actually starts at bit 17 (where the
lsbit is bit 0), so we need to be shifting down by 17 instead. This
makes the status align with the enum nvme_status_field values.

Add an explicit test for this, and clarify that the existing admin_err
test is for the MI header status field.

Fixes: 108d696 ("mi: Introduce a helper for response status, unify values with ioctls")
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

diff --git a/src/nvme/mi.c b/src/nvme/mi.c
index 6ff0a6f68d1421beae0de2605153b8619b4b2ba5..6040bcd9cb4dbaa73f0948444fe207dc7306cdd4 100644 (file)
--- a/src/nvme/mi.c
+++ b/src/nvme/mi.c
@@ -333,7 +333,12 @@ static int nvme_mi_admin_parse_status(struct nvme_mi_resp *resp, __u32 *result)
 
        admin_hdr = (struct nvme_mi_admin_resp_hdr *)resp->hdr;
        nvme_result = le32_to_cpu(admin_hdr->cdw0);
-       nvme_status = le32_to_cpu(admin_hdr->cdw3) >> 16;
+
+       /* Shift down 17 here: the SC starts at bit 17, and the NVME_SC_*
+        * definitions align to this bit (and up). The CRD, MORE and DNR
+        * bits are defined accordingly (eg., DNR is 0x4000).
+        */
+       nvme_status = le32_to_cpu(admin_hdr->cdw3) >> 17;
 
        /* the result pointer, optionally stored if the caller needs it */
        if (result)

diff --git a/test/mi.c b/test/mi.c
index c21f9dbbb541ca85d74c00a6a7f3a1f10a66c6cd..2ce92a28052201eddbc4a8ffb90cb00c7ef8a7f5 100644 (file)
--- a/test/mi.c
+++ b/test/mi.c
@@ -442,8 +442,8 @@ static void test_admin_id(nvme_mi_ep_t ep)
        assert(rc == 0);
 }
 
-/* test: simple NVMe error response */
-static int test_admin_err_resp_cb(struct nvme_mi_ep *ep,
+/* test: simple NVMe error response, error reported in the MI header */
+static int test_admin_err_mi_resp_cb(struct nvme_mi_ep *ep,
                                  struct nvme_mi_req *req,
                                  struct nvme_mi_resp *resp,
                                  void *data)
@@ -481,19 +481,85 @@ static int test_admin_err_resp_cb(struct nvme_mi_ep *ep,
        return 0;
 }
 
-static void test_admin_err_resp(nvme_mi_ep_t ep)
+static void test_admin_err_mi_resp(nvme_mi_ep_t ep)
 {
        struct nvme_id_ctrl id;
        nvme_mi_ctrl_t ctrl;
        int rc;
 
-       test_set_transport_callback(ep, test_admin_err_resp_cb, NULL);
+       test_set_transport_callback(ep, test_admin_err_mi_resp_cb, NULL);
 
        ctrl = nvme_mi_init_ctrl(ep, 1);
        assert(ctrl);
 
        rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
        assert(rc != 0);
+       assert(nvme_status_get_type(rc) == NVME_STATUS_TYPE_MI);
+       assert(nvme_status_get_value(rc) == NVME_MI_RESP_INTERNAL_ERR);
+}
+
+/* test: NVMe Admin error, with the error reported in the Admin response */
+static int test_admin_err_nvme_resp_cb(struct nvme_mi_ep *ep,
+                                 struct nvme_mi_req *req,
+                                 struct nvme_mi_resp *resp,
+                                 void *data)
+{
+       __u8 ror, mt, *hdr;
+
+       assert(req->hdr->type == NVME_MI_MSGTYPE_NVME);
+
+       ror = req->hdr->nmp >> 7;
+       mt = req->hdr->nmp >> 3 & 0x7;
+       assert(ror == NVME_MI_ROR_REQ);
+       assert(mt == NVME_MI_MT_ADMIN);
+
+       /* do we have enough for a mi header? */
+       assert(req->hdr_len == sizeof(struct nvme_mi_admin_req_hdr));
+
+       /* inspect response as raw bytes */
+       hdr = (__u8 *)req->hdr;
+       assert(hdr[4] == nvme_admin_identify);
+
+       /* we need at least 8 bytes for error information */
+       assert(resp->hdr_len >= sizeof(struct nvme_mi_admin_resp_hdr));
+
+       /* create error response */
+       hdr = (__u8 *)resp->hdr;
+       hdr[4] = 0; /* MI status: success */
+       hdr[5] = 0;
+       hdr[6] = 0;
+       hdr[7] = 0;
+
+       hdr[16] = 0; /* cdw3: SC: internal, SCT: generic, DNR */
+       hdr[17] = 0;
+       hdr[18] = 0x0c;
+       hdr[19] = 0x80;
+
+       resp->hdr_len = sizeof(struct nvme_mi_admin_resp_hdr);
+       resp->data_len = 0;
+
+       test_transport_resp_calc_mic(resp);
+
+       return 0;
+}
+
+static void test_admin_err_nvme_resp(nvme_mi_ep_t ep)
+{
+       struct nvme_id_ctrl id;
+       nvme_mi_ctrl_t ctrl;
+       int rc;
+
+       test_set_transport_callback(ep, test_admin_err_nvme_resp_cb, NULL);
+
+       ctrl = nvme_mi_init_ctrl(ep, 1);
+       assert(ctrl);
+
+       rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
+       assert(rc != 0);
+       assert(nvme_status_get_type(rc) == NVME_STATUS_TYPE_NVME);
+       assert(nvme_status_get_value(rc) ==
+              (NVME_SC_INTERNAL | (NVME_SCT_GENERIC << NVME_SCT_SHIFT)
+               | NVME_SC_DNR));
 }
 
 /* invalid Admin command transfers */
@@ -1792,7 +1858,8 @@ struct test {
        DEFINE_TEST(scan_ctrl_list),
        DEFINE_TEST(invalid_crc),
        DEFINE_TEST(admin_id),
-       DEFINE_TEST(admin_err_resp),
+       DEFINE_TEST(admin_err_mi_resp),
+       DEFINE_TEST(admin_err_nvme_resp),
        DEFINE_TEST(admin_invalid_formats),
        DEFINE_TEST(resp_req),
        DEFINE_TEST(resp_hdr_small),