From 5cd7634939dee68044c491e50b8845eab6e917de Mon Sep 17 00:00:00 2001 From: Seth Howell Date: Fri, 18 Oct 2019 08:42:20 -0700 Subject: [PATCH] nvme_ctrlr: enable the admin qpair before init. The driver has historically waited until we have to do a listen before enabling the admin qpair. That is a very PCIe-centric mindset. For fabric controllers, a lot of the early initialization operations such as get_cc and set_cc are handled through the admin qpair so it should be enabled before we begin the initialization process. As a side effect of this cahnge, the internal API nvme_ctrlr_enable_admin_qpair has been removed. It would have turned into a one-liner. Change-Id: Icd162657d01a85c227a3f20c295d0208e07ce44d Signed-off-by: Seth Howell Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/471743 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Jim Harris Reviewed-by: Changpeng Liu Reviewed-by: Shuhei Matsumoto Reviewed-by: Alexey Marchuk --- lib/nvme/nvme.c | 1 + lib/nvme/nvme_ctrlr.c | 18 ++++++------------ lib/nvme/nvme_internal.h | 4 ++-- test/unit/lib/nvme/nvme.c/nvme_ut.c | 9 +++++++++ .../unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 16 ++++++++-------- 5 files changed, 26 insertions(+), 22 deletions(-) diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index af08c0636..55d44cea6 100644 --- a/lib/nvme/nvme.c +++ b/lib/nvme/nvme.c @@ -429,6 +429,7 @@ nvme_ctrlr_probe(const struct spdk_nvme_transport_id *trid, ctrlr->remove_cb = probe_ctx->remove_cb; ctrlr->cb_ctx = probe_ctx->cb_ctx; + nvme_qpair_enable(ctrlr->adminq); TAILQ_INSERT_TAIL(&probe_ctx->init_ctrlrs, ctrlr, tailq); return 0; } diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index 524fbb8a7..af5e3fc44 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -769,8 +769,8 @@ nvme_ctrlr_state_string(enum nvme_ctrlr_state state) return "enable controller by writing CC.EN = 1"; case NVME_CTRLR_STATE_ENABLE_WAIT_FOR_READY_1: return "wait for CSTS.RDY = 1"; - case NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE: - return "enable admin queue"; + case NVME_CTRLR_STATE_RESET_ADMIN_QUEUE: + return "reset admin queue"; case NVME_CTRLR_STATE_IDENTIFY: return "identify controller"; case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY: @@ -1006,6 +1006,7 @@ nvme_ctrlr_reset(struct spdk_nvme_ctrlr *ctrlr) /* Set the state back to INIT to cause a full hardware reset. */ nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_INIT, NVME_TIMEOUT_INFINITE); + nvme_qpair_enable(ctrlr->adminq); while (ctrlr->state != NVME_CTRLR_STATE_READY) { if (nvme_ctrlr_process_init(ctrlr) != 0) { SPDK_ERRLOG("controller reinitialization failed\n"); @@ -2083,13 +2084,6 @@ nvme_ctrlr_proc_get_devhandle(struct spdk_nvme_ctrlr *ctrlr) return devhandle; } -static void -nvme_ctrlr_enable_admin_queue(struct spdk_nvme_ctrlr *ctrlr) -{ - nvme_transport_qpair_reset(ctrlr->adminq); - nvme_qpair_enable(ctrlr->adminq); -} - /** * This function will be called repeatedly during initialization until the controller is ready. */ @@ -2232,14 +2226,14 @@ nvme_ctrlr_process_init(struct spdk_nvme_ctrlr *ctrlr) * The controller has been enabled. * Perform the rest of initialization serially. */ - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_RESET_ADMIN_QUEUE, ctrlr->opts.admin_timeout_ms); return 0; } break; - case NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE: - nvme_ctrlr_enable_admin_queue(ctrlr); + case NVME_CTRLR_STATE_RESET_ADMIN_QUEUE: + nvme_transport_qpair_reset(ctrlr->adminq); nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY, ctrlr->opts.admin_timeout_ms); break; diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index b3e6434cb..79a4d2d7a 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -433,9 +433,9 @@ enum nvme_ctrlr_state { NVME_CTRLR_STATE_ENABLE_WAIT_FOR_READY_1, /** - * Enable the Admin queue of the controller. + * Reset the Admin queue of the controller. */ - NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE, + NVME_CTRLR_STATE_RESET_ADMIN_QUEUE, /** * Identify Controller command will be sent to then controller. diff --git a/test/unit/lib/nvme/nvme.c/nvme_ut.c b/test/unit/lib/nvme/nvme.c/nvme_ut.c index a4acc9196..d637c7add 100644 --- a/test/unit/lib/nvme/nvme.c/nvme_ut.c +++ b/test/unit/lib/nvme/nvme.c/nvme_ut.c @@ -68,6 +68,12 @@ nvme_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr) ut_destruct_called = true; } +void +nvme_qpair_enable(struct spdk_nvme_qpair *qpair) +{ + qpair->is_enabled = true; +} + void spdk_nvme_ctrlr_get_default_ctrlr_opts(struct spdk_nvme_ctrlr_opts *opts, size_t opts_size) { @@ -722,12 +728,15 @@ test_nvme_ctrlr_probe(void) { int rc = 0; struct spdk_nvme_ctrlr ctrlr = {}; + struct spdk_nvme_qpair qpair = {}; const struct spdk_nvme_transport_id trid = {}; struct spdk_nvme_probe_ctx probe_ctx = {}; void *devhandle = NULL; void *cb_ctx = NULL; struct spdk_nvme_ctrlr *dummy = NULL; + ctrlr.adminq = &qpair; + TAILQ_INIT(&probe_ctx.init_ctrlrs); nvme_driver_init(); 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 0d7c89193..5f038a92c 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 @@ -525,7 +525,7 @@ test_nvme_ctrlr_init_en_1_rdy_0(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY. @@ -579,7 +579,7 @@ test_nvme_ctrlr_init_en_1_rdy_1(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY. @@ -754,7 +754,7 @@ test_nvme_ctrlr_init_en_0_rdy_0_ams_rr(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY. @@ -931,7 +931,7 @@ test_nvme_ctrlr_init_en_0_rdy_0_ams_wrr(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY. @@ -1107,7 +1107,7 @@ test_nvme_ctrlr_init_en_0_rdy_0_ams_vs(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY. @@ -1153,7 +1153,7 @@ test_nvme_ctrlr_init_en_0_rdy_0(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY. @@ -1205,7 +1205,7 @@ test_nvme_ctrlr_init_en_0_rdy_1(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY. @@ -1829,7 +1829,7 @@ test_nvme_ctrlr_init_delay(void) */ g_ut_nvme_regs.csts.bits.rdy = 1; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_ENABLE_ADMIN_QUEUE); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_RESET_ADMIN_QUEUE); /* * Transition to READY.