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

[Jeremy Allison]

On Thu, Jan 04, 2024 at 09:43:30AM -0800, Jeremy Allison wrote:
>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.

Yeah, the tricky part is that call chain for nvme_shutdown()
looks like (inside drivers/nvme/host/pci.c):

nvme_shutdown() ->
	nvme_disable_prepare_reset() ->
		nvme_dev_disable() ->
			nvme_disable_ctrl()

Adding a new function nvme_shutdown_ctrl_nowait()
that sets the bit without waiting is trivial, but
the problem is nvme_dev_disable() does a lot of
things before calling nvme_disable_ctrl() which
obviously should not be duplicated in a separate
"twopass" code path.

There needs to be some way to pass a use
"two-pass" flag down from nvme_shutdown()
into nvme_dev_disable().

I guess I could add a second bool flag passed
down from nvme_shutdown() all the way to
nvme_dev_disable(), which then calls the
nvme_shutdown_ctrl_nowait() varient instead
of nvme_disable_ctrl() if both "bool shutdown"
and "bool twopass" are set but that seems kind
of ugly and confusing to me.

How about keeping the enum internal to
drivers/nvme/host/pci.c
and making it:

enum shutdown_type {
	NVME_DISABLE_RESET,
	NVME_DISABLE_SHUTDOWN,
	NVME_DISABLE_SHUTDOWN_TWOPASS
};

and changing 'bool shutdown' to enum shutdown_type
inside the static functions nvme_disable_prepare_reset()
and nvme_dev_disable() only ?

That way we don't have to have the confusing
double bool:

static int nvme_disable_prepare_reset(struct nvme_dev *dev, bool shutdown, bool twopass);
static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown, bool twopass);

instead we have:

static int nvme_disable_prepare_reset(struct nvme_dev *dev, enum shutdown_type shutdown_type);
static void nvme_dev_disable(struct nvme_dev *dev, enum shutdown_type shutdown_type);

function signatures and the use of the enum is
contained solely inside drivers/nvme/host/pci.c.

IMHO this is easier to understand, but again I'm
a newbie at this code so I'm happy to take your
preferences here.