From 4246e79c047564c96e578de3e408d58132647972 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Mon, 5 Jul 2021 17:47:19 +0000 Subject: [PATCH] nvme: change nvme_transport_ctrlr_delete_io_qpair to void Returning an error from this function is not useful - there is nothing the caller can do with that information. So change the return value to void. Also add ERRLOG and assert if a transport actually returns a non-zero status, to force the transport implementer (which must be an out-of-tree transport) to make changes as necessary. Signed-off-by: Jim Harris Change-Id: I402afec045265db178af821d25b99a6dbe066eab Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8659 Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins --- include/spdk/nvme.h | 5 +++-- lib/nvme/nvme_ctrlr.c | 6 +----- lib/nvme/nvme_internal.h | 2 +- lib/nvme/nvme_transport.c | 10 ++++++++-- test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 3 +-- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/include/spdk/nvme.h b/include/spdk/nvme.h index 8366e7ab03..1f4e0c6fe2 100644 --- a/include/spdk/nvme.h +++ b/include/spdk/nvme.h @@ -1501,10 +1501,11 @@ spdk_nvme_qp_failure_reason spdk_nvme_ctrlr_get_admin_qp_failure_reason( /** * Free an I/O queue pair that was allocated by spdk_nvme_ctrlr_alloc_io_qpair(). * + * The qpair must not be accessed after calling this function. + * * \param qpair I/O queue pair to free. * - * \return 0 on success, -1 on failure. On failure, the caller should reset - * the controller and try to free the io qpair again after the reset. + * \return 0 on success. This function will never return any value other than 0. */ int spdk_nvme_ctrlr_free_io_qpair(struct spdk_nvme_qpair *qpair); diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index f9534175c1..aab3395073 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -583,11 +583,7 @@ spdk_nvme_ctrlr_free_io_qpair(struct spdk_nvme_qpair *qpair) TAILQ_REMOVE(&ctrlr->active_io_qpairs, qpair, tailq); spdk_nvme_ctrlr_free_qid(ctrlr, qpair->id); - if (nvme_transport_ctrlr_delete_io_qpair(ctrlr, qpair)) { - nvme_robust_mutex_unlock(&ctrlr->ctrlr_lock); - return -1; - } - + nvme_transport_ctrlr_delete_io_qpair(ctrlr, qpair); nvme_robust_mutex_unlock(&ctrlr->ctrlr_lock); return 0; } diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index 49fd789dbd..eb32b7907d 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -1342,7 +1342,7 @@ int nvme_transport_ctrlr_enable_pmr(struct spdk_nvme_ctrlr *ctrlr); int nvme_transport_ctrlr_disable_pmr(struct spdk_nvme_ctrlr *ctrlr); void *nvme_transport_ctrlr_map_pmr(struct spdk_nvme_ctrlr *ctrlr, size_t *size); int nvme_transport_ctrlr_unmap_pmr(struct spdk_nvme_ctrlr *ctrlr); -int nvme_transport_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, +void nvme_transport_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpair *qpair); int nvme_transport_ctrlr_connect_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpair *qpair); diff --git a/lib/nvme/nvme_transport.c b/lib/nvme/nvme_transport.c index f2bb19d9da..d59185cdd7 100644 --- a/lib/nvme/nvme_transport.c +++ b/lib/nvme/nvme_transport.c @@ -328,10 +328,11 @@ nvme_transport_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, uint16_t qid return qpair; } -int +void nvme_transport_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpair *qpair) { const struct spdk_nvme_transport *transport = nvme_get_transport(ctrlr->trid.trstring); + int rc; assert(transport != NULL); @@ -340,7 +341,12 @@ nvme_transport_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_ * own unique transport objects since they contain function pointers). So we look up the * transport object in the delete_io_qpair case. */ - return transport->ops.ctrlr_delete_io_qpair(ctrlr, qpair); + rc = transport->ops.ctrlr_delete_io_qpair(ctrlr, qpair); + if (rc != 0) { + SPDK_ERRLOG("transport %s returned non-zero for ctrlr_delete_io_qpair op\n", + transport->ops.name); + assert(false); + } } int 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 75a69a2cb4..8de8b739b7 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 @@ -204,11 +204,10 @@ nvme_transport_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, uint16_t qid return qpair; } -int +void nvme_transport_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpair *qpair) { free(qpair); - return 0; } void