From 4b4b3cca9f120ca69bad08227041242fa6da1f06 Mon Sep 17 00:00:00 2001 From: Evgeniy Kochetov Date: Wed, 1 Apr 2020 12:30:11 +0300 Subject: [PATCH] nvme/ctrlr: Allow targets not supporting Keep Alive Timer feature ID NVMe spec defines "Keep Alive Timer" feature ID as optional and there are targets that do not support this. SPDK fails to connect to such targets. This patch allows Get Feature "Keep Alive" target to fail with INVALID_FIELD status. In this case we just continue with keep alive timer value stored in controller opts structure. This value is already communicated to target in CONNECT command. Fixes #1328 Signed-off-by: Evgeniy Kochetov Change-Id: I52e7ea3cb66073ce6cc168a169989bd179041618 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1625 Tested-by: SPDK CI Jenkins Community-CI: Broadcom CI Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- lib/nvme/nvme_ctrlr.c | 27 +++++++----- .../lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 41 ++++++++++++++++++- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index a05e2ee13a..c4509fb217 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -1789,20 +1789,25 @@ nvme_ctrlr_set_keep_alive_timeout_done(void *arg, const struct spdk_nvme_cpl *cp struct spdk_nvme_ctrlr *ctrlr = (struct spdk_nvme_ctrlr *)arg; if (spdk_nvme_cpl_is_error(cpl)) { - SPDK_ERRLOG("Keep alive timeout Get Feature failed: SC %x SCT %x\n", - cpl->status.sc, cpl->status.sct); - ctrlr->opts.keep_alive_timeout_ms = 0; - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_ERROR, NVME_TIMEOUT_INFINITE); - return; - } + if ((cpl->status.sct == SPDK_NVME_SCT_GENERIC) && + (cpl->status.sc == SPDK_NVME_SC_INVALID_FIELD)) { + SPDK_DEBUGLOG(SPDK_LOG_NVME, "Keep alive timeout Get Feature is not supported\n"); + } else { + SPDK_ERRLOG("Keep alive timeout Get Feature failed: SC %x SCT %x\n", + cpl->status.sc, cpl->status.sct); + ctrlr->opts.keep_alive_timeout_ms = 0; + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_ERROR, NVME_TIMEOUT_INFINITE); + return; + } + } else { + if (ctrlr->opts.keep_alive_timeout_ms != cpl->cdw0) { + SPDK_DEBUGLOG(SPDK_LOG_NVME, "Controller adjusted keep alive timeout to %u ms\n", + cpl->cdw0); + } - if (ctrlr->opts.keep_alive_timeout_ms != cpl->cdw0) { - SPDK_DEBUGLOG(SPDK_LOG_NVME, "Controller adjusted keep alive timeout to %u ms\n", - cpl->cdw0); + ctrlr->opts.keep_alive_timeout_ms = cpl->cdw0; } - ctrlr->opts.keep_alive_timeout_ms = cpl->cdw0; - keep_alive_interval_ms = ctrlr->opts.keep_alive_timeout_ms / 2; if (keep_alive_interval_ms == 0) { keep_alive_interval_ms = 1; diff --git a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c index 03c9cf1586..adebc4bfbb 100644 --- a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c +++ b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c @@ -234,8 +234,8 @@ spdk_nvme_ctrlr_cmd_get_feature(struct spdk_nvme_ctrlr *ctrlr, uint8_t feature, uint32_t cdw11, void *payload, uint32_t payload_size, spdk_nvme_cmd_cb cb_fn, void *cb_arg) { - CU_ASSERT(0); - return -1; + fake_cpl_sc(cb_fn, cb_arg); + return 0; } int @@ -2065,6 +2065,42 @@ test_nvme_ctrlr_init_set_num_queues(void) nvme_ctrlr_destruct(&ctrlr); } +static void +test_nvme_ctrlr_init_set_keep_alive_timeout(void) +{ + DECLARE_AND_CONSTRUCT_CTRLR(); + + ctrlr.opts.keep_alive_timeout_ms = 60000; + ctrlr.cdata.kas = 1; + ctrlr.state = NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT; + fake_cpl.cdw0 = 120000; + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> SET_HOST_ID */ + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_HOST_ID); + CU_ASSERT(ctrlr.opts.keep_alive_timeout_ms == 120000); + fake_cpl.cdw0 = 0; + + /* Target does not support Get Feature "Keep Alive Timer" */ + ctrlr.opts.keep_alive_timeout_ms = 60000; + ctrlr.cdata.kas = 1; + ctrlr.state = NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT; + set_status_code = SPDK_NVME_SC_INVALID_FIELD; + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> SET_HOST_ID */ + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_HOST_ID); + CU_ASSERT(ctrlr.opts.keep_alive_timeout_ms == 60000); + set_status_code = SPDK_NVME_SC_SUCCESS; + + /* Target fails Get Feature "Keep Alive Timer" for another reason */ + ctrlr.opts.keep_alive_timeout_ms = 60000; + ctrlr.cdata.kas = 1; + ctrlr.state = NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT; + set_status_code = SPDK_NVME_SC_INTERNAL_DEVICE_ERROR; + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> ERROR */ + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ERROR); + set_status_code = SPDK_NVME_SC_SUCCESS; + + nvme_ctrlr_destruct(&ctrlr); +} + int main(int argc, char **argv) { CU_pSuite suite = NULL; @@ -2102,6 +2138,7 @@ int main(int argc, char **argv) CU_ADD_TEST(suite, test_spdk_nvme_ctrlr_set_trid); CU_ADD_TEST(suite, test_nvme_ctrlr_init_set_nvmf_ioccsz); CU_ADD_TEST(suite, test_nvme_ctrlr_init_set_num_queues); + CU_ADD_TEST(suite, test_nvme_ctrlr_init_set_keep_alive_timeout); CU_basic_set_mode(CU_BRM_VERBOSE); CU_basic_run_tests();