Merge pull request #460 from CodeConstruct/mpr

mi: Allow Admin-message sized More Processing Required responses

Author: dwsuse

src/nvme/mi-mctp.c

@@ -179,36 +179,58 @@ struct nvme_mi_msg_resp_mpr {
  * populate the worst-case expected processing time, given in milliseconds.
  */
 static bool nvme_mi_mctp_resp_is_mpr(struct nvme_mi_resp *resp, size_t len,
-				     unsigned int *mpr_time)
+				     __le32 mic, unsigned int *mpr_time)
 {
+	struct nvme_mi_admin_resp_hdr *admin_msg;
 	struct nvme_mi_msg_resp_mpr *msg;
-	__le32 mic;
+	size_t clen;
 	__u32 crc;
 
-	if (len != sizeof(*msg) + sizeof(mic))
+	/* We need at least the minimal header plus checksum */
+	if (len < sizeof(*msg) + sizeof(mic))
 		return false;
 
 	msg = (struct nvme_mi_msg_resp_mpr *)resp->hdr;
 
 	if (msg->status != NVME_MI_RESP_MPR)
 		return false;
 
-	/* We can't use verify_resp_mic here, as the response structure has
-	 * not been laid-out properly in resp yet (this is deferred until
-	 * we have the actual response).
+	/* Find and verify the MIC from the response, which may not be laid out
+	 * in resp as we expect. We have to preserve resp->hdr_len and
+	 * resp->data_len, as we will need them for the eventual reply message.
+	 * Because of that, we can't use verify_resp_mic here.
 	 *
-	 * We know the data is a fixed size, and linear in the hdr buf, so
-	 * calculation is fairly simple. We do need to find the MIC data
-	 * though, which could either be in the header buf (if the original
-	 * header was larger than the minimal header message), or the start of
-	 * the data buf (otherwise).
+	 * If the packet was at the expected response size, then mic will
+	 * be set already; if not, find it within the header/data buffers.
+	 */
+
+	/* Devices may send a MPR response as a full-sized Admin response,
+	 * rather than the minimal MI-only header. Allow this, but only if the
+	 * type indicates admin, and the allocated response header is the
+	 * correct size for an Admin response.
+	 */
+	if (((msg->hdr.nmp >> 3) & 0xf) == NVME_MI_MT_ADMIN &&
+	    len == sizeof(*admin_msg) + sizeof(mic) &&
+	    resp->hdr_len == sizeof(*admin_msg)) {
+		if (resp->data_len)
+			mic = *(__le32 *)resp->data;
+	} else if (len == sizeof(*msg) + sizeof(mic)) {
+		if (resp->hdr_len > sizeof(*msg))
+			mic = *(__le32 *)(msg + 1);
+		else if (resp->data_len)
+			mic = *(__le32 *)(resp->data);
+	} else {
+		return false;
+	}
+
+	/* Since our response is just a header, we're guaranteed to have
+	 * all data in resp->hdr. The response may be shorter than the expected
+	 * header though, so clamp to len.
 	 */
-	if (resp->hdr_len > sizeof(*msg))
-		mic = *(__le32 *)(msg + 1);
-	else
-		mic = *(__le32 *)(resp->data);
+	len -= sizeof(mic);
+	clen = len < resp->hdr_len ? len : resp->hdr_len;
 
-	crc = ~nvme_mi_crc32_update(0xffffffff, msg, sizeof(*msg));
+	crc = ~nvme_mi_crc32_update(0xffffffff, resp->hdr, clen);
 	if (le32_to_cpu(mic) != crc)
 		return false;
 
@@ -369,7 +391,7 @@ static int nvme_mi_mctp_submit(struct nvme_mi_ep *ep,
 	 * header fields. However, we need to do this in the transport in order
 	 * to keep the tag allocated and retry the recvmsg
 	 */
-	if (nvme_mi_mctp_resp_is_mpr(resp, len, &mpr_time)) {
+	if (nvme_mi_mctp_resp_is_mpr(resp, len, mic, &mpr_time)) {
 		nvme_msg(ep->root, LOG_DEBUG,
 			 "Received More Processing Required, waiting for response\n");
 

test/mi-mctp.c

@@ -407,6 +407,7 @@ static void test_poll_timeout(nvme_mi_ep_t ep, struct test_peer *peer)
 /* test: send a More Processing Required response, then the actual response */
 struct mpr_tx_info {
 	int msg_no;
+	bool admin_quirk;
 	size_t final_len;
 };
 
@@ -422,6 +423,9 @@ static int tx_mpr(struct test_peer *peer, void *buf, size_t len)
 	case 1:
 		peer->tx_buf[4] = NVME_MI_RESP_MPR;
 		peer->tx_buf_len = 8;
+		if (tx_info->admin_quirk) {
+			peer->tx_buf_len = 20;
+		}
 		break;
 	case 2:
 		peer->tx_buf[4] = NVME_MI_RESP_SUCCESS;
@@ -446,6 +450,7 @@ static void test_mpr_mi(nvme_mi_ep_t ep, struct test_peer *peer)
 
 	tx_info.msg_no = 1;
 	tx_info.final_len = sizeof(struct nvme_mi_mi_resp_hdr) + sizeof(ss_info);
+	tx_info.admin_quirk = false;
 
 	peer->tx_fn = tx_mpr;
 	peer->tx_data = &tx_info;
@@ -463,6 +468,32 @@ static void test_mpr_admin(nvme_mi_ep_t ep, struct test_peer *peer)
 
 	tx_info.msg_no = 1;
 	tx_info.final_len = sizeof(struct nvme_mi_admin_resp_hdr) + sizeof(id);
+	tx_info.admin_quirk = false;
+
+	peer->tx_fn = tx_mpr;
+	peer->tx_data = &tx_info;
+
+	ctrl = nvme_mi_init_ctrl(ep, 1);
+
+	rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
+	assert(rc == 0);
+
+	nvme_mi_close_ctrl(ctrl);
+}
+
+/* We have seen drives that send a MPR response as a full Admin message,
+ * rather than a MI message; these have a larger message body
+ */
+static void test_mpr_admin_quirked(nvme_mi_ep_t ep, struct test_peer *peer)
+{
+	struct mpr_tx_info tx_info;
+	struct nvme_id_ctrl id;
+	nvme_mi_ctrl_t ctrl;
+	int rc;
+
+	tx_info.msg_no = 1;
+	tx_info.final_len = sizeof(struct nvme_mi_admin_resp_hdr) + sizeof(id);
+	tx_info.admin_quirk = true;
 
 	peer->tx_fn = tx_mpr;
 	peer->tx_data = &tx_info;
@@ -638,6 +669,7 @@ struct test {
 	DEFINE_TEST(poll_timeout),
 	DEFINE_TEST(mpr_mi),
 	DEFINE_TEST(mpr_admin),
+	DEFINE_TEST(mpr_admin_quirked),
 	DEFINE_TEST(mpr_timeouts),
 	DEFINE_TEST(mpr_timeout_clamp),
 	DEFINE_TEST(mpr_mprt_zero),