From 89d35cefd5a98367f9ff10423bfe69e232b325a2 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Wed, 16 Aug 2017 18:13:49 -0700 Subject: [PATCH] nvmf: refactor get_log_page into a common function Both regular NVM controllers and discovery controllers implement the Get Log Page command; combine the implementations into one in ctrlr.c. Change-Id: I7fabf40ec52d8738263ac152afe9cd7773ff7fbd Signed-off-by: Daniel Verkamp Reviewed-on: https://review.gerrithub.io/374555 Tested-by: SPDK Automated Test System Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- lib/nvmf/ctrlr.c | 69 ++++++++++++++++++ lib/nvmf/ctrlr.h | 2 + lib/nvmf/ctrlr_bdev.c | 39 +---------- lib/nvmf/ctrlr_discovery.c | 35 +--------- test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c | 70 ++++++++++++++++++- .../lib/nvmf/ctrlr_bdev.c/ctrlr_bdev_ut.c | 6 ++ .../ctrlr_discovery.c/ctrlr_discovery_ut.c | 23 ++---- 7 files changed, 153 insertions(+), 91 deletions(-) diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index 781b87c485..7902b7de5d 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -818,3 +818,72 @@ spdk_nvmf_ctrlr_async_event_request(struct spdk_nvmf_request *req) ctrlr->aer_req = req; return SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS; } + +int +spdk_nvmf_ctrlr_get_log_page(struct spdk_nvmf_request *req) +{ + struct spdk_nvmf_subsystem *subsystem = req->qpair->ctrlr->subsys; + struct spdk_nvme_cmd *cmd = &req->cmd->nvme_cmd; + struct spdk_nvme_cpl *response = &req->rsp->nvme_cpl; + uint64_t offset, len; + uint32_t numdl, numdu; + uint8_t lid; + + if (req->data == NULL) { + SPDK_ERRLOG("get log command with no buffer\n"); + response->status.sct = SPDK_NVME_SCT_GENERIC; + response->status.sc = SPDK_NVME_SC_INVALID_FIELD; + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + } + + memset(req->data, 0, req->length); + + offset = (uint64_t)cmd->cdw12 | ((uint64_t)cmd->cdw13 << 32); + if (offset & 3) { + SPDK_ERRLOG("Invalid log page offset 0x%" PRIx64 "\n", offset); + response->status.sct = SPDK_NVME_SCT_GENERIC; + response->status.sc = SPDK_NVME_SC_INVALID_FIELD; + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + } + + numdl = (cmd->cdw10 >> 16) & 0xFFFFu; + numdu = (cmd->cdw11) & 0xFFFFu; + len = ((numdu << 16) + numdl + (uint64_t)1) * 4; + if (len > req->length) { + SPDK_ERRLOG("Get log page: len (%" PRIu64 ") > buf size (%u)\n", + len, req->length); + response->status.sct = SPDK_NVME_SCT_GENERIC; + response->status.sc = SPDK_NVME_SC_INVALID_FIELD; + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + } + + lid = cmd->cdw10 & 0xFF; + SPDK_TRACELOG(SPDK_TRACE_NVMF, "Get log page: LID=0x%02X offset=0x%" PRIx64 " len=0x%" PRIx64 "\n", + lid, offset, len); + + if (subsystem->subtype == SPDK_NVMF_SUBTYPE_DISCOVERY) { + switch (lid) { + case SPDK_NVME_LOG_DISCOVERY: + spdk_nvmf_get_discovery_log_page(req->data, offset, len); + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + default: + goto invalid_log_page; + } + } else { + switch (lid) { + case SPDK_NVME_LOG_ERROR: + case SPDK_NVME_LOG_HEALTH_INFORMATION: + case SPDK_NVME_LOG_FIRMWARE_SLOT: + /* TODO: actually fill out log page data */ + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + default: + goto invalid_log_page; + } + } + +invalid_log_page: + SPDK_ERRLOG("Unsupported Get Log Page 0x%02X\n", lid); + response->status.sct = SPDK_NVME_SCT_GENERIC; + response->status.sc = SPDK_NVME_SC_INVALID_FIELD; + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; +} diff --git a/lib/nvmf/ctrlr.h b/lib/nvmf/ctrlr.h index 53656657ad..6fb976978c 100644 --- a/lib/nvmf/ctrlr.h +++ b/lib/nvmf/ctrlr.h @@ -134,6 +134,8 @@ int spdk_nvmf_ctrlr_get_features_async_event_configuration(struct spdk_nvmf_requ int spdk_nvmf_ctrlr_async_event_request(struct spdk_nvmf_request *req); +int spdk_nvmf_ctrlr_get_log_page(struct spdk_nvmf_request *req); + void spdk_nvmf_ctrlr_set_dsm(struct spdk_nvmf_ctrlr *ctrlr); #endif diff --git a/lib/nvmf/ctrlr_bdev.c b/lib/nvmf/ctrlr_bdev.c index 8bfd788399..d32bc1f7c7 100644 --- a/lib/nvmf/ctrlr_bdev.c +++ b/lib/nvmf/ctrlr_bdev.c @@ -105,43 +105,6 @@ nvmf_bdev_ctrlr_complete_cmd(struct spdk_bdev_io *bdev_io, bool success, spdk_bdev_free_io(bdev_io); } -static int -nvmf_bdev_ctrlr_get_log_page(struct spdk_nvmf_request *req) -{ - uint8_t lid; - struct spdk_nvme_cmd *cmd = &req->cmd->nvme_cmd; - struct spdk_nvme_cpl *response = &req->rsp->nvme_cpl; - uint64_t log_page_offset; - - if (req->data == NULL) { - SPDK_ERRLOG("get log command with no buffer\n"); - response->status.sc = SPDK_NVME_SC_INVALID_FIELD; - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } - - memset(req->data, 0, req->length); - - log_page_offset = (uint64_t)cmd->cdw12 | ((uint64_t)cmd->cdw13 << 32); - if (log_page_offset & 3) { - SPDK_ERRLOG("Invalid log page offset 0x%" PRIx64 "\n", log_page_offset); - response->status.sc = SPDK_NVME_SC_INVALID_FIELD; - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } - - lid = cmd->cdw10 & 0xFF; - switch (lid) { - case SPDK_NVME_LOG_ERROR: - case SPDK_NVME_LOG_HEALTH_INFORMATION: - case SPDK_NVME_LOG_FIRMWARE_SLOT: - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - default: - SPDK_ERRLOG("Unsupported Get Log Page 0x%02X\n", lid); - response->status.sct = SPDK_NVME_SCT_COMMAND_SPECIFIC; - response->status.sc = SPDK_NVME_SC_INVALID_LOG_PAGE; - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } -} - static int identify_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_nvme_cmd *cmd, @@ -357,7 +320,7 @@ nvmf_bdev_ctrlr_process_admin_cmd(struct spdk_nvmf_request *req) switch (cmd->opc) { case SPDK_NVME_OPC_GET_LOG_PAGE: - return nvmf_bdev_ctrlr_get_log_page(req); + return spdk_nvmf_ctrlr_get_log_page(req); case SPDK_NVME_OPC_IDENTIFY: return nvmf_bdev_ctrlr_identify(req); case SPDK_NVME_OPC_ABORT: diff --git a/lib/nvmf/ctrlr_discovery.c b/lib/nvmf/ctrlr_discovery.c index 3e52628fb7..d5b92b440b 100644 --- a/lib/nvmf/ctrlr_discovery.c +++ b/lib/nvmf/ctrlr_discovery.c @@ -144,22 +144,12 @@ spdk_nvmf_get_discovery_log_page(void *buffer, uint64_t offset, uint32_t length) assert(copy_len + zero_len == length); } -static inline uint32_t -nvmf_get_log_page_len(struct spdk_nvme_cmd *cmd) -{ - uint32_t numdl = (cmd->cdw10 >> 16) & 0xFFFFu; - uint32_t numdu = (cmd->cdw11) & 0xFFFFu; - return ((numdu << 16) + numdl + 1) * sizeof(uint32_t); -} - static int nvmf_discovery_ctrlr_process_admin_cmd(struct spdk_nvmf_request *req) { struct spdk_nvmf_ctrlr *ctrlr = req->qpair->ctrlr; struct spdk_nvme_cmd *cmd = &req->cmd->nvme_cmd; struct spdk_nvme_cpl *response = &req->rsp->nvme_cpl; - uint64_t log_page_offset; - uint32_t len; /* pre-set response details for this command */ response->status.sc = SPDK_NVME_SC_SUCCESS; @@ -184,30 +174,7 @@ nvmf_discovery_ctrlr_process_admin_cmd(struct spdk_nvmf_request *req) } break; case SPDK_NVME_OPC_GET_LOG_PAGE: - log_page_offset = (uint64_t)cmd->cdw12 | ((uint64_t)cmd->cdw13 << 32); - if (log_page_offset & 3) { - SPDK_ERRLOG("Invalid log page offset 0x%" PRIx64 "\n", log_page_offset); - response->status.sc = SPDK_NVME_SC_INVALID_FIELD; - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } - - len = nvmf_get_log_page_len(cmd); - if (len > req->length) { - SPDK_ERRLOG("Get log page: len (%u) > buf size (%u)\n", - len, req->length); - response->status.sc = SPDK_NVME_SC_INVALID_FIELD; - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } - - if ((cmd->cdw10 & 0xFF) == SPDK_NVME_LOG_DISCOVERY) { - spdk_nvmf_get_discovery_log_page(req->data, log_page_offset, len); - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } else { - SPDK_ERRLOG("Unsupported log page %u\n", cmd->cdw10 & 0xFF); - response->status.sc = SPDK_NVME_SC_INVALID_FIELD; - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } - break; + return spdk_nvmf_ctrlr_get_log_page(req); default: SPDK_ERRLOG("Unsupported Opcode 0x%x for Discovery service\n", cmd->opc); response->status.sc = SPDK_NVME_SC_INVALID_OPCODE; diff --git a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c index 2746440119..32ec55c135 100644 --- a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c +++ b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c @@ -108,9 +108,10 @@ spdk_nvmf_ctrlr_set_dsm(struct spdk_nvmf_ctrlr *ctrlr) abort(); } -static void -test_foobar(void) +void +spdk_nvmf_get_discovery_log_page(void *buffer, uint64_t offset, uint32_t length) { + abort(); } int @@ -119,6 +120,69 @@ spdk_nvmf_request_complete(struct spdk_nvmf_request *req) return -1; } +static void +test_get_log_page(void) +{ + struct spdk_nvmf_subsystem subsystem = {}; + struct spdk_nvmf_request req = {}; + struct spdk_nvmf_qpair qpair = {}; + struct spdk_nvmf_ctrlr ctrlr = {}; + union nvmf_h2c_msg cmd = {}; + union nvmf_c2h_msg rsp = {}; + char data[4096]; + + subsystem.subtype = SPDK_NVMF_SUBTYPE_NVME; + + ctrlr.subsys = &subsystem; + + qpair.ctrlr = &ctrlr; + + req.qpair = &qpair; + req.cmd = &cmd; + req.rsp = &rsp; + req.data = &data; + req.length = sizeof(data); + + /* Get Log Page - all valid */ + memset(&cmd, 0, sizeof(cmd)); + memset(&rsp, 0, sizeof(rsp)); + cmd.nvme_cmd.opc = SPDK_NVME_OPC_GET_LOG_PAGE; + cmd.nvme_cmd.cdw10 = SPDK_NVME_LOG_ERROR | (req.length / 4 - 1) << 16; + CU_ASSERT(spdk_nvmf_ctrlr_get_log_page(&req) == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE); + CU_ASSERT(req.rsp->nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC); + CU_ASSERT(req.rsp->nvme_cpl.status.sc == SPDK_NVME_SC_SUCCESS); + + /* Get Log Page with invalid log ID */ + memset(&cmd, 0, sizeof(cmd)); + memset(&rsp, 0, sizeof(rsp)); + cmd.nvme_cmd.opc = SPDK_NVME_OPC_GET_LOG_PAGE; + cmd.nvme_cmd.cdw10 = 0; + CU_ASSERT(spdk_nvmf_ctrlr_get_log_page(&req) == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE); + CU_ASSERT(req.rsp->nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC); + CU_ASSERT(req.rsp->nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_FIELD); + + /* Get Log Page with invalid offset (not dword aligned) */ + memset(&cmd, 0, sizeof(cmd)); + memset(&rsp, 0, sizeof(rsp)); + cmd.nvme_cmd.opc = SPDK_NVME_OPC_GET_LOG_PAGE; + cmd.nvme_cmd.cdw10 = SPDK_NVME_LOG_ERROR | (req.length / 4 - 1) << 16; + cmd.nvme_cmd.cdw12 = 2; + CU_ASSERT(spdk_nvmf_ctrlr_get_log_page(&req) == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE); + CU_ASSERT(req.rsp->nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC); + CU_ASSERT(req.rsp->nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_FIELD); + + /* Get Log Page without data buffer */ + memset(&cmd, 0, sizeof(cmd)); + memset(&rsp, 0, sizeof(rsp)); + req.data = NULL; + cmd.nvme_cmd.opc = SPDK_NVME_OPC_GET_LOG_PAGE; + cmd.nvme_cmd.cdw10 = SPDK_NVME_LOG_ERROR | (req.length / 4 - 1) << 16; + CU_ASSERT(spdk_nvmf_ctrlr_get_log_page(&req) == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE); + CU_ASSERT(req.rsp->nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC); + CU_ASSERT(req.rsp->nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_FIELD); + req.data = data; +} + int main(int argc, char **argv) { CU_pSuite suite = NULL; @@ -135,7 +199,7 @@ int main(int argc, char **argv) } if ( - CU_add_test(suite, "foobar", test_foobar) == NULL) { + CU_add_test(suite, "get_log_page", test_get_log_page) == NULL) { CU_cleanup_registry(); return CU_get_error(); } diff --git a/test/unit/lib/nvmf/ctrlr_bdev.c/ctrlr_bdev_ut.c b/test/unit/lib/nvmf/ctrlr_bdev.c/ctrlr_bdev_ut.c index 102e9d278c..45430f0891 100644 --- a/test/unit/lib/nvmf/ctrlr_bdev.c/ctrlr_bdev_ut.c +++ b/test/unit/lib/nvmf/ctrlr_bdev.c/ctrlr_bdev_ut.c @@ -104,6 +104,12 @@ spdk_nvmf_ctrlr_async_event_request(struct spdk_nvmf_request *req) return -1; } +int +spdk_nvmf_ctrlr_get_log_page(struct spdk_nvmf_request *req) +{ + return -1; +} + int spdk_nvmf_request_complete(struct spdk_nvmf_request *req) { diff --git a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c index 9d594a36ef..854401f67d 100644 --- a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c +++ b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c @@ -155,17 +155,21 @@ spdk_nvmf_ctrlr_poll(struct spdk_nvmf_ctrlr *ctrlr) return -1; } +int +spdk_nvmf_ctrlr_get_log_page(struct spdk_nvmf_request *req) +{ + abort(); + return -1; +} + static void test_process_discovery_cmd(void) { struct spdk_nvmf_request req = {}; int ret; - /* random request length value for testing */ - int req_length = 122; struct spdk_nvmf_qpair req_qpair = {}; struct spdk_nvmf_ctrlr req_ctrlr = {}; struct spdk_nvme_ctrlr_data req_data = {}; - struct spdk_nvmf_discovery_log_page req_page = {}; union nvmf_h2c_msg req_cmd = {}; union nvmf_c2h_msg req_rsp = {}; @@ -187,19 +191,6 @@ test_process_discovery_cmd(void) CU_ASSERT_EQUAL(req.rsp->nvme_cpl.status.sc, SPDK_NVME_SC_SUCCESS); CU_ASSERT_EQUAL(ret, SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE); - /* GET_LOG_PAGE opcode return value check */ - req.cmd->nvme_cmd.opc = SPDK_NVME_OPC_GET_LOG_PAGE; - req.cmd->nvme_cmd.cdw10 = SPDK_NVME_LOG_DISCOVERY; - req.data = &req_page; - req.length = req_length; - ret = nvmf_discovery_ctrlr_process_admin_cmd(&req); - CU_ASSERT_EQUAL(req.rsp->nvme_cpl.status.sc, SPDK_NVME_SC_SUCCESS); - CU_ASSERT_EQUAL(ret, SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE); - req.cmd->nvme_cmd.cdw10 = 15; - ret = nvmf_discovery_ctrlr_process_admin_cmd(&req); - CU_ASSERT_EQUAL(req.rsp->nvme_cpl.status.sc, SPDK_NVME_SC_INVALID_FIELD); - CU_ASSERT_EQUAL(ret, SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE); - /* Invalid opcode return value check */ req.cmd->nvme_cmd.opc = 100; ret = nvmf_discovery_ctrlr_process_admin_cmd(&req);