[PATCH 3/5] nvme: Change 'bool shutdown' into an enum shutdown_type.

[Jeremy Allison]

On Thu, Jan 04, 2024 at 03:26:11PM +0200, Sagi Grimberg wrote:
>
>
>On 1/3/24 23:04, Jeremy Allison wrote:
>>Convert nvme_disable_ctrl() and nvme_dev_disable()
>>inside drivers/nvme/host/pci.c to use this:
>>
>>bool shutdown = false == NVME_DISABLE_RESET
>>bool shutdown = true == NVME_DISABLE_SHUTDOWN_SYNC.
>>
>>This will make it easier to add a third request later:
>>NVME_DISABLE_SHUTDOWN_ASNYC
>>
>>As nvme_disable_ctrl() is used outside of drivers/nvme/host/pci.c,
>>convert the callers of nvme_disable_ctrl() to this convention too.
>>
>>Signed-off-by: Jeremy Allison <jallison at ciq.com>
>>---
>>  drivers/nvme/host/apple.c  |  4 ++--
>>  drivers/nvme/host/core.c   |  6 +++---
>>  drivers/nvme/host/nvme.h   |  7 +++++-
>>  drivers/nvme/host/pci.c    | 44 +++++++++++++++++++-------------------
>>  drivers/nvme/host/rdma.c   |  3 ++-
>>  drivers/nvme/host/tcp.c    |  3 ++-
>>  drivers/nvme/target/loop.c |  2 +-
>>  7 files changed, 38 insertions(+), 31 deletions(-)
>>
>>diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
>>index 596bb11eeba5..764639ede41d 100644
>>--- a/drivers/nvme/host/apple.c
>>+++ b/drivers/nvme/host/apple.c
>>@@ -844,8 +844,8 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
>>  		 * NVMe disabled state, after a clean shutdown).
>>  		 */
>>  		if (shutdown)
>>-			nvme_disable_ctrl(&anv->ctrl, shutdown);
>>-		nvme_disable_ctrl(&anv->ctrl, false);
>>+			nvme_disable_ctrl(&anv->ctrl, NVME_DISABLE_SHUTDOWN_SYNC);
>>+		nvme_disable_ctrl(&anv->ctrl, NVME_DISABLE_RESET);
>>  	}
>>  	WRITE_ONCE(anv->ioq.enabled, false);
>>diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>index 50818dbcfa1a..e1b2facb7d6a 100644
>>--- a/drivers/nvme/host/core.c
>>+++ b/drivers/nvme/host/core.c
>>@@ -2219,12 +2219,12 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
>>  	return ret;
>>  }
>>-int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
>>+int nvme_disable_ctrl(struct nvme_ctrl *ctrl, enum shutdown_type shutdown_type)
>>  {
>>  	int ret;
>>  	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
>>-	if (shutdown)
>>+	if (shutdown_type == NVME_DISABLE_SHUTDOWN_SYNC)
>>  		ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
>>  	else
>>  		ctrl->ctrl_config &= ~NVME_CC_ENABLE;
>>@@ -2233,7 +2233,7 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
>>  	if (ret)
>>  		return ret;
>>-	if (shutdown) {
>>+	if (shutdown_type == NVME_DISABLE_SHUTDOWN_SYNC) {
>>  		return nvme_wait_ready(ctrl, NVME_CSTS_SHST_MASK,
>>  				       NVME_CSTS_SHST_CMPLT,
>>  				       ctrl->shutdown_timeout, "shutdown");
>>diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>>index 6092cc361837..1a748640f2fb 100644
>>--- a/drivers/nvme/host/nvme.h
>>+++ b/drivers/nvme/host/nvme.h
>>@@ -187,6 +187,11 @@ enum {
>>  	NVME_MPATH_IO_STATS		= (1 << 2),
>>  };
>>+enum shutdown_type {
>>+	NVME_DISABLE_RESET = 0,
>>+	NVME_DISABLE_SHUTDOWN_SYNC = 1
>>+};
>
>I also don't find this a bit awkward especially when another
>enumeration value is added later on.
>
>An alternative approach would be to stick with nvme_disable_ctrl that
>accepts shutdown bool, and add nvme_disable_ctrl_nowait() wrapper for it
>that call-sites can use. Or even better, because we only have a nowait
>variant for shutdown, perhaps we can add nvme_shutdown_ctrl_nowait()
>wrapper that accepts only the ctrl instead (this will require splitting
>nvme_disable_ctrl to the disabling phase and the waiting phase, but that
>is hidden in the core as static functions).

OK. Let me take a look at this. It would certainly remove the
changes in unrelated files such as drivers/nvme/host/apple.c,
drivers/nvme/host/tcp.c and drivers/nvme/target/loop.c.

This change might take me a little longer to get right, so
please bear with me.