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

[Sagi Grimberg]

On 1/4/24 20:44, Jeremy Allison wrote:
> 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.

Ugh.

> 
> There needs to be some way to pass a use
> "two-pass" flag down from nvme_shutdown()
> into nvme_dev_disable()
Ideally nvme_dev_disable can be broken in a way that
nvme_shutdown can have 2-3 helpers and the rest of the
code can remain intact calling nvme_dev_disable (internally
calling these helpers).

> 
> 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.

Probably.. Maybe call it dontwait (bool).
And yes, it is ugly, but better than moving the
ugliness to the core.

> 
> 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
> };

NVME_PCI_

> 
> 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:

Neither are great. I'll defer to Keith and Christoph on this one.