remove path cleanups

remove path cleanups

[Christoph Hellwig]

Hi all,

this series cleans up the remove path in the PCIe driver and a bit in the
common code after fixing a harmless bug in the Apple driver.

Diffstat:
 host/apple.c  |    4 -
 host/core.c   |   65 +++++++++-----------------------
 host/fc.c     |    2 
 host/nvme.h   |    3 -
 host/pci.c    |  117 +++++++++++++++++++++++++---------------------------------
 host/rdma.c   |    5 --
 host/tcp.c    |    5 --
 target/loop.c |    2 
 8 files changed, 77 insertions(+), 126 deletions(-)

[PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable

[Christoph Hellwig]

nvme_shutdown_ctrl already shuts the controller down, there is no
need to also call nvme_disable_ctrl for the shutdown case.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/apple.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index 94ef797e8b4a5f..56d9e9be945b76 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -831,7 +831,8 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
 
 		if (shutdown)
 			nvme_shutdown_ctrl(&anv->ctrl);
-		nvme_disable_ctrl(&anv->ctrl);
+		else
+			nvme_disable_ctrl(&anv->ctrl);
 	}
 
 	WRITE_ONCE(anv->ioq.enabled, false);
-- 
2.30.2

Re: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable

[Hector Martin]

On 29/11/2022 22.22, Christoph Hellwig wrote:
> nvme_shutdown_ctrl already shuts the controller down, there is no
> need to also call nvme_disable_ctrl for the shutdown case.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/apple.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
> index 94ef797e8b4a5f..56d9e9be945b76 100644
> --- a/drivers/nvme/host/apple.c
> +++ b/drivers/nvme/host/apple.c
> @@ -831,7 +831,8 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
>  
>  		if (shutdown)
>  			nvme_shutdown_ctrl(&anv->ctrl);
> -		nvme_disable_ctrl(&anv->ctrl);
> +		else
> +			nvme_disable_ctrl(&anv->ctrl);
>  	}
>  
>  	WRITE_ONCE(anv->ioq.enabled, false);

Reviewed-by: Hector Martin <marcan@marcan.st>

I looked at some of our other implementations and we always seem to do
both, but this makes sense. If it breaks something we'll notice and
shout loudly when it makes it into an -rc at the latest :)

- Hector

Re: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable

[Janne Grunau]

On 2022-11-30 00:41:45 +0900, Hector Martin wrote:
> On 29/11/2022 22.22, Christoph Hellwig wrote:
> > nvme_shutdown_ctrl already shuts the controller down, there is no
> > need to also call nvme_disable_ctrl for the shutdown case.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  drivers/nvme/host/apple.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
> > index 94ef797e8b4a5f..56d9e9be945b76 100644
> > --- a/drivers/nvme/host/apple.c
> > +++ b/drivers/nvme/host/apple.c
> > @@ -831,7 +831,8 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
> >  
> >  		if (shutdown)
> >  			nvme_shutdown_ctrl(&anv->ctrl);
> > -		nvme_disable_ctrl(&anv->ctrl);
> > +		else
> > +			nvme_disable_ctrl(&anv->ctrl);
> >  	}
> >  
> >  	WRITE_ONCE(anv->ioq.enabled, false);
> 
> Reviewed-by: Hector Martin <marcan@marcan.st>
> 
> I looked at some of our other implementations and we always seem to do
> both, but this makes sense. If it breaks something we'll notice and
> shout loudly when it makes it into an -rc at the latest :)

This breaks resume for the apple nvme controller. The immediate problem 
is that apple_nvme_reset_work() uses NVME_CC_ENABLE to decide whether to 
call apple_nvme_disable(). Only nvme_disable_ctrl(ctrl, false) clears 
NVME_CC_ENABLE.

Even after extending the check to consider NVME_CC_SHN_NORMAL resume is 
still broken. The host specific reset to re-enable the controller works 
but IO is blocked. The controller logs following messages into its 
syslog:

| RTKit: syslog message: cmd.c:728: Oldest tag 12 is 19 seconds VERY OLD with cmdQe:2
| RTKit: syslog message: cmd.c:732: Oldest tag is about to crash!!
| RTKit: syslog message: cmd.c:720: Oldest high water mark moved up to 21,tag 12, cmdQ CQ_PRI0W

This looks looks like the controller reset after/during shutdown is required
on this controller.

Below patch fixes resume for me. I can send this with comment to
prevent repeating this regression or if preferred handle this in
nvme_disable_ctrl() either via a quirk or an additional parameter.

Janne

--- 
diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index e13a992b6096..82396f9486e1 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -867,7 +867,9 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
			apple_nvme_remove_cq(anv);
		}
 
-               nvme_disable_ctrl(&anv->ctrl, shutdown);
+               if (shutdown)
+                       nvme_disable_ctrl(&anv->ctrl, true);
+               nvme_disable_ctrl(&anv->ctrl, false);
	}
 
	WRITE_ONCE(anv->ioq.enabled, false);

Re: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable

[Hector Martin]

On 11/01/2023 02.47, Janne Grunau wrote:
> On 2022-11-30 00:41:45 +0900, Hector Martin wrote:
>> On 29/11/2022 22.22, Christoph Hellwig wrote:
>>> nvme_shutdown_ctrl already shuts the controller down, there is no
>>> need to also call nvme_disable_ctrl for the shutdown case.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>  drivers/nvme/host/apple.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
>>> index 94ef797e8b4a5f..56d9e9be945b76 100644
>>> --- a/drivers/nvme/host/apple.c
>>> +++ b/drivers/nvme/host/apple.c
>>> @@ -831,7 +831,8 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
>>>  
>>>  		if (shutdown)
>>>  			nvme_shutdown_ctrl(&anv->ctrl);
>>> -		nvme_disable_ctrl(&anv->ctrl);
>>> +		else
>>> +			nvme_disable_ctrl(&anv->ctrl);
>>>  	}
>>>  
>>>  	WRITE_ONCE(anv->ioq.enabled, false);
>>
>> Reviewed-by: Hector Martin <marcan@marcan.st>
>>
>> I looked at some of our other implementations and we always seem to do
>> both, but this makes sense. If it breaks something we'll notice and
>> shout loudly when it makes it into an -rc at the latest :)
> 
> This breaks resume for the apple nvme controller. The immediate problem 
> is that apple_nvme_reset_work() uses NVME_CC_ENABLE to decide whether to 
> call apple_nvme_disable(). Only nvme_disable_ctrl(ctrl, false) clears 
> NVME_CC_ENABLE.
> 
> Even after extending the check to consider NVME_CC_SHN_NORMAL resume is 
> still broken. The host specific reset to re-enable the controller works 
> but IO is blocked. The controller logs following messages into its 
> syslog:
> 
> | RTKit: syslog message: cmd.c:728: Oldest tag 12 is 19 seconds VERY OLD with cmdQe:2
> | RTKit: syslog message: cmd.c:732: Oldest tag is about to crash!!
> | RTKit: syslog message: cmd.c:720: Oldest high water mark moved up to 21,tag 12, cmdQ CQ_PRI0W
> 
> This looks looks like the controller reset after/during shutdown is required
> on this controller.

It's actually required by the NVMe spec. See the SHST description in
Figure 47 (NVMe Base Spec 2.0c). I'll send a patch.

- Hector

Re: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable

[Linux kernel regression tracking (#adding)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

Top-posting one thing, to make this msg easier to consume.

Hector, thx for already working on this. If you respin your patches,
could you please include these tags in your patches:

Reported-by: Janne Grunau <j@jannau.net>
Link: https://lore.kernel.org/all/20230110174745.GA3576@jannau.net/

For details, see Documentation/process/submitting-patches.rst
(http://docs.kernel.org/process/submitting-patches.html) and
Documentation/process/5.Posting.rst
(https://docs.kernel.org/process/5.Posting.html)

Or these posts from Linus:

https://lore.kernel.org/all/CAHk-=wjMmSZzMJ3Xnskdg4+GGz=5p5p+GSYyFBTh0f-DgvdBWg@mail.gmail.com/

https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/

https://lore.kernel.org/all/CAHk-=wjxzafG-=J8oT30s7upn4RhBs6TX-uVFZ5rME+L5_DoJA@mail.gmail.com/


For the rest of this mail:

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 10.01.23 18:47, Janne Grunau wrote:
> On 2022-11-30 00:41:45 +0900, Hector Martin wrote:
>> On 29/11/2022 22.22, Christoph Hellwig wrote:
>>> nvme_shutdown_ctrl already shuts the controller down, there is no
>>> need to also call nvme_disable_ctrl for the shutdown case.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>  drivers/nvme/host/apple.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
>>> index 94ef797e8b4a5f..56d9e9be945b76 100644
>>> --- a/drivers/nvme/host/apple.c
>>> +++ b/drivers/nvme/host/apple.c
>>> @@ -831,7 +831,8 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
>>>  
>>>  		if (shutdown)
>>>  			nvme_shutdown_ctrl(&anv->ctrl);
>>> -		nvme_disable_ctrl(&anv->ctrl);
>>> +		else
>>> +			nvme_disable_ctrl(&anv->ctrl);
>>>  	}
>>>  
>>>  	WRITE_ONCE(anv->ioq.enabled, false);
>>
>> Reviewed-by: Hector Martin <marcan@marcan.st>
>>
>> I looked at some of our other implementations and we always seem to do
>> both, but this makes sense. If it breaks something we'll notice and
>> shout loudly when it makes it into an -rc at the latest :)
> 
> This breaks resume for the apple nvme controller.


> The immediate problem 
> is that apple_nvme_reset_work() uses NVME_CC_ENABLE to decide whether to 
> call apple_nvme_disable(). Only nvme_disable_ctrl(ctrl, false) clears 
> NVME_CC_ENABLE.
> 
> Even after extending the check to consider NVME_CC_SHN_NORMAL resume is 
> still broken. The host specific reset to re-enable the controller works 
> but IO is blocked. The controller logs following messages into its 
> syslog:
> 
> | RTKit: syslog message: cmd.c:728: Oldest tag 12 is 19 seconds VERY OLD with cmdQe:2
> | RTKit: syslog message: cmd.c:732: Oldest tag is about to crash!!
> | RTKit: syslog message: cmd.c:720: Oldest high water mark moved up to 21,tag 12, cmdQ CQ_PRI0W
> 
> This looks looks like the controller reset after/during shutdown is required
> on this controller.
> 
> Below patch fixes resume for me. I can send this with comment to
> prevent repeating this regression or if preferred handle this in
> nvme_disable_ctrl() either via a quirk or an additional parameter.

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced c76b8308e4c9
#regzbot title nvme: resume broken on apple nvme controller
#regzbot monitor:
https://lore.kernel.org/all/20230111043614.27087-1-marcan@marcan.st/
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

> --- 
> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
> index e13a992b6096..82396f9486e1 100644
> --- a/drivers/nvme/host/apple.c
> +++ b/drivers/nvme/host/apple.c
> @@ -867,7 +867,9 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
> 			apple_nvme_remove_cq(anv);
> 		}
>  
> -               nvme_disable_ctrl(&anv->ctrl, shutdown);
> +               if (shutdown)
> +                       nvme_disable_ctrl(&anv->ctrl, true);
> +               nvme_disable_ctrl(&anv->ctrl, false);
> 	}
>  
> 	WRITE_ONCE(anv->ioq.enabled, false);

Re: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable

[Eric Curtin]

On Tue, 29 Nov 2022 at 13:29, Christoph Hellwig <hch@lst.de> wrote:
>
> nvme_shutdown_ctrl already shuts the controller down, there is no
> need to also call nvme_disable_ctrl for the shutdown case.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

> ---
>  drivers/nvme/host/apple.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
> index 94ef797e8b4a5f..56d9e9be945b76 100644
> --- a/drivers/nvme/host/apple.c
> +++ b/drivers/nvme/host/apple.c
> @@ -831,7 +831,8 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
>
>                 if (shutdown)
>                         nvme_shutdown_ctrl(&anv->ctrl);
> -               nvme_disable_ctrl(&anv->ctrl);
> +               else
> +                       nvme_disable_ctrl(&anv->ctrl);
>         }
>
>         WRITE_ONCE(anv->ioq.enabled, false);
> --
> 2.30.2
>
>

Re: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl

[Christoph Hellwig]

Refactor the code to wait for CSTS state changes so that it can be reused
by nvme_shutdown_ctrl.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 02cc8dfc9f0b59..9f20af2406dcc8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2260,16 +2260,17 @@ static const struct block_device_operations nvme_bdev_ops = {
 	.pr_ops		= &nvme_pr_ops,
 };
 
-static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 timeout, bool enabled)
+static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
+		u32 timeout, const char *op)
 {
 	unsigned long timeout_jiffies = ((timeout + 1) * HZ / 2) + jiffies;
-	u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
+	u32 csts;
 	int ret;
 
 	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
 		if (csts == ~0)
 			return -ENODEV;
-		if ((csts & NVME_CSTS_RDY) == bit)
+		if ((csts & mask) == val)
 			break;
 
 		usleep_range(1000, 2000);
@@ -2278,7 +2279,7 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 timeout, bool enabled)
 		if (time_after(jiffies, timeout_jiffies)) {
 			dev_err(ctrl->device,
 				"Device not ready; aborting %s, CSTS=0x%x\n",
-				enabled ? "initialisation" : "reset", csts);
+				op, csts);
 			return -ENODEV;
 		}
 	}
@@ -2305,8 +2306,8 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
 
 	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY)
 		msleep(NVME_QUIRK_DELAY_AMOUNT);
-
-	return nvme_wait_ready(ctrl, NVME_CAP_TIMEOUT(ctrl->cap), false);
+	return nvme_wait_ready(ctrl, NVME_CSTS_RDY, 0,
+			       NVME_CAP_TIMEOUT(ctrl->cap), "reset");
 }
 EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
 
@@ -2371,14 +2372,13 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
-	return nvme_wait_ready(ctrl, timeout, true);
+	return nvme_wait_ready(ctrl, NVME_CSTS_RDY, NVME_CSTS_RDY, timeout,
+			       "initialisation");
 }
 EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
-	u32 csts;
 	int ret;
 
 	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
@@ -2387,22 +2387,8 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
-
-	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
-		if ((csts & NVME_CSTS_SHST_MASK) == NVME_CSTS_SHST_CMPLT)
-			break;
-
-		msleep(100);
-		if (fatal_signal_pending(current))
-			return -EINTR;
-		if (time_after(jiffies, timeout)) {
-			dev_err(ctrl->device,
-				"Device shutdown incomplete; abort shutdown\n");
-			return -ENODEV;
-		}
-	}
-
-	return ret;
+	return nvme_wait_ready(ctrl, NVME_CSTS_SHST_MASK, NVME_CSTS_SHST_CMPLT,
+			       ctrl->shutdown_timeout, "shutdown");
 }
 EXPORT_SYMBOL_GPL(nvme_shutdown_ctrl);
 
-- 
2.30.2

Re: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl

[Pankaj Raghav]

On Tue, Nov 29, 2022 at 02:22:01PM +0100, Christoph Hellwig wrote:
> Refactor the code to wait for CSTS state changes so that it can be reused
> by nvme_shutdown_ctrl.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 36 +++++++++++-------------------------
>  1 file changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 02cc8dfc9f0b59..9f20af2406dcc8 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2260,16 +2260,17 @@ static const struct block_device_operations nvme_bdev_ops = {
>  	.pr_ops		= &nvme_pr_ops,
>  };
>  
> -static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 timeout, bool enabled)
> +static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
> +		u32 timeout, const char *op)
>  {
>  	unsigned long timeout_jiffies = ((timeout + 1) * HZ / 2) + jiffies;
The timeout_jiffies calculation in nvme_shutdown_ctrl is:
unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);

Aren't we changing the timeout with this change to something
different compared to what it was before for shutdown?

> -	u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
> +	u32 csts;
>  	int ret;
>  
>  	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
>  		if (csts == ~0)
>  			return -ENODEV;
> -		if ((csts & NVME_CSTS_RDY) == bit)
> +		if ((csts & mask) == val)
>  			break;
>  
>  		usleep_range(1000, 2000);
> @@ -2278,7 +2279,7 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 timeout, bool enabled)
>  		if (time_after(jiffies, timeout_jiffies)) {
>  			dev_err(ctrl->device,
>  				"Device not ready; aborting %s, CSTS=0x%x\n",
> -				enabled ? "initialisation" : "reset", csts);
> +				op, csts);
>  			return -ENODEV;
>  		}
>  	}

Re: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl

[Christoph Hellwig]

On Tue, Nov 29, 2022 at 04:57:18PM +0100, Pankaj Raghav wrote:
> The timeout_jiffies calculation in nvme_shutdown_ctrl is:
> unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
> 
> Aren't we changing the timeout with this change to something
> different compared to what it was before for shutdown?

Indeed.  the timeout fields in the spec are in a different granularity
than the shutdown_timeout field.  This version should fix that:

---
From 2f862232c4428d57dcfdc3b332866a9b2743711d Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 16 Nov 2022 08:54:26 +0100
Subject: nvme: use nvme_wait_ready in nvme_shutdown_ctrl

Refactor the code to wait for CSTS state changes so that it can be reused
by nvme_shutdown_ctrl.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 38 ++++++++++++--------------------------
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7961e146bbb163..03b2e34dcf7249 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2252,16 +2252,17 @@ static const struct block_device_operations nvme_bdev_ops = {
 	.pr_ops		= &nvme_pr_ops,
 };
 
-static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 timeout, bool enabled)
+static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
+		u32 timeout, const char *op)
 {
-	unsigned long timeout_jiffies = ((timeout + 1) * HZ / 2) + jiffies;
-	u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
+	unsigned long timeout_jiffies = jiffies + timeout * HZ;
+	u32 csts;
 	int ret;
 
 	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
 		if (csts == ~0)
 			return -ENODEV;
-		if ((csts & NVME_CSTS_RDY) == bit)
+		if ((csts & mask) == val)
 			break;
 
 		usleep_range(1000, 2000);
@@ -2270,7 +2271,7 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 timeout, bool enabled)
 		if (time_after(jiffies, timeout_jiffies)) {
 			dev_err(ctrl->device,
 				"Device not ready; aborting %s, CSTS=0x%x\n",
-				enabled ? "initialisation" : "reset", csts);
+				op, csts);
 			return -ENODEV;
 		}
 	}
@@ -2297,8 +2298,8 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
 
 	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY)
 		msleep(NVME_QUIRK_DELAY_AMOUNT);
-
-	return nvme_wait_ready(ctrl, NVME_CAP_TIMEOUT(ctrl->cap), false);
+	return nvme_wait_ready(ctrl, NVME_CSTS_RDY, 0,
+			       (NVME_CAP_TIMEOUT(ctrl->cap) + 1) / 2, "reset");
 }
 EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
 
@@ -2363,14 +2364,13 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
-	return nvme_wait_ready(ctrl, timeout, true);
+	return nvme_wait_ready(ctrl, NVME_CSTS_RDY, NVME_CSTS_RDY,
+			       (timeout + 1) / 2, "initialisation");
 }
 EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
-	u32 csts;
 	int ret;
 
 	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
@@ -2379,22 +2379,8 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
-
-	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
-		if ((csts & NVME_CSTS_SHST_MASK) == NVME_CSTS_SHST_CMPLT)
-			break;
-
-		msleep(100);
-		if (fatal_signal_pending(current))
-			return -EINTR;
-		if (time_after(jiffies, timeout)) {
-			dev_err(ctrl->device,
-				"Device shutdown incomplete; abort shutdown\n");
-			return -ENODEV;
-		}
-	}
-
-	return ret;
+	return nvme_wait_ready(ctrl, NVME_CSTS_SHST_MASK, NVME_CSTS_SHST_CMPLT,
+			       ctrl->shutdown_timeout, "shutdown");
 }
 EXPORT_SYMBOL_GPL(nvme_shutdown_ctrl);
 
-- 
2.30.2

Re: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl

[Pankaj Raghav]

On 2022-11-30 14:57, Christoph Hellwig wrote:
> On Tue, Nov 29, 2022 at 04:57:18PM +0100, Pankaj Raghav wrote:
>> The timeout_jiffies calculation in nvme_shutdown_ctrl is:
>> unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
>>
>> Aren't we changing the timeout with this change to something
>> different compared to what it was before for shutdown?
> 
> Indeed.  the timeout fields in the spec are in a different granularity
> than the shutdown_timeout field.  This version should fix that:
> 
> ---
> From 2f862232c4428d57dcfdc3b332866a9b2743711d Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Wed, 16 Nov 2022 08:54:26 +0100
> Subject: nvme: use nvme_wait_ready in nvme_shutdown_ctrl
> 
> Refactor the code to wait for CSTS state changes so that it can be reused
> by nvme_shutdown_ctrl.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

One minor nit: I also notice that nvme_wait_ready busy loop sleeps for
1~2ms, while the existing shutdown code busy loop sleeps for 100ms. Though,
I don't think it is an issue.

Apart from that, the new version looks good to me. Feel free to add to it:

Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
>  drivers/nvme/host/core.c | 38 ++++++++++++--------------------------
>  1 file changed, 12 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 7961e146bbb163..03b2e34dcf7249 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2252,16 +2252,17 @@ static const struct block_device_operations nvme_bdev_ops = {
>  	.pr_ops		= &nvme_pr_ops,
>  };
>  
> -static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 timeout, bool enabled)
> +static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
> +		u32 timeout, const char *op)
>  {
> -	unsigned long timeout_jiffies = ((timeout + 1) * HZ / 2) + jiffies;
> -	u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
> +	unsigned long timeout_jiffies = jiffies + timeout * HZ;
> +	u32 csts;
>  	int ret;
>  
>  	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
>  		if (csts == ~0)
>  			return -ENODEV;
> -		if ((csts & NVME_CSTS_RDY) == bit)
> +		if ((csts & mask) == val)
>  			break;
>  
>  		usleep_range(1000, 2000);
<snip>

> -		msleep(100);
> -		if (fatal_signal_pending(current))
> -			return -EINTR;

Re: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl

[Keith Busch]

On Wed, Nov 30, 2022 at 03:27:29PM +0100, Pankaj Raghav wrote:
> On 2022-11-30 14:57, Christoph Hellwig wrote:
> > From 2f862232c4428d57dcfdc3b332866a9b2743711d Mon Sep 17 00:00:00 2001
> > From: Christoph Hellwig <hch@lst.de>
> > Date: Wed, 16 Nov 2022 08:54:26 +0100
> > Subject: nvme: use nvme_wait_ready in nvme_shutdown_ctrl
> > 
> > Refactor the code to wait for CSTS state changes so that it can be reused
> > by nvme_shutdown_ctrl.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> One minor nit: I also notice that nvme_wait_ready busy loop sleeps for
> 1~2ms, while the existing shutdown code busy loop sleeps for 100ms. Though,
> I don't think it is an issue.
> 
> Apart from that, the new version looks good to me. Feel free to add to it:

That is indeed a difference, but it's probably a good one. It should
tighten up the time to shutdown more closely to the hardware's
completion, and there's probably some environment where the 10's of
millisecs saved might be meaningful.

Re: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl

[Christoph Hellwig]

> One minor nit: I also notice that nvme_wait_ready busy loop sleeps for
> 1~2ms, while the existing shutdown code busy loop sleeps for 100ms. Though,
> I don't think it is an issue.

Thanks, I've added a note about that, and a reference to the commit
that changed from the msleep(100) to the usleep_range for the
enable/disable path.

[PATCH 3/9] nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl

[Christoph Hellwig]

Many of the callers decide which one to use based on a bool argument and
there is at least some code to be shared, so merge these two.  Also
move a comment specific to a single callsite to that callsite.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/apple.c  |  5 +----
 drivers/nvme/host/core.c   | 33 ++++++++++-----------------------
 drivers/nvme/host/fc.c     |  2 +-
 drivers/nvme/host/nvme.h   |  3 +--
 drivers/nvme/host/pci.c    | 15 +++++++++------
 drivers/nvme/host/rdma.c   |  5 +----
 drivers/nvme/host/tcp.c    |  5 +----
 drivers/nvme/target/loop.c |  2 +-
 8 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index 56d9e9be945b76..e36aeb50b4edc1 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -829,10 +829,7 @@ static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
 			apple_nvme_remove_cq(anv);
 		}
 
-		if (shutdown)
-			nvme_shutdown_ctrl(&anv->ctrl);
-		else
-			nvme_disable_ctrl(&anv->ctrl);
+		nvme_disable_ctrl(&anv->ctrl, shutdown);
 	}
 
 	WRITE_ONCE(anv->ioq.enabled, false);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9f20af2406dcc8..3753f9b5b54110 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2287,23 +2287,25 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
 	return ret;
 }
 
-/*
- * If the device has been passed off to us in an enabled state, just clear
- * the enabled bit.  The spec says we should set the 'shutdown notification
- * bits', but doing so may cause the device to complete commands to the
- * admin queue ... and we don't know what memory that might be pointing at!
- */
-int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
+int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
 	int ret;
 
 	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
-	ctrl->ctrl_config &= ~NVME_CC_ENABLE;
+	if (shutdown)
+		ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
+	else
+		ctrl->ctrl_config &= ~NVME_CC_ENABLE;
 
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
 
+	if (shutdown) {
+		return nvme_wait_ready(ctrl, NVME_CSTS_SHST_MASK,
+				       NVME_CSTS_SHST_CMPLT,
+				       ctrl->shutdown_timeout, "shutdown");
+	}
 	if (ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY)
 		msleep(NVME_QUIRK_DELAY_AMOUNT);
 	return nvme_wait_ready(ctrl, NVME_CSTS_RDY, 0,
@@ -2377,21 +2379,6 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
-int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
-{
-	int ret;
-
-	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
-	ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
-
-	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
-	if (ret)
-		return ret;
-	return nvme_wait_ready(ctrl, NVME_CSTS_SHST_MASK, NVME_CSTS_SHST_CMPLT,
-			       ctrl->shutdown_timeout, "shutdown");
-}
-EXPORT_SYMBOL_GPL(nvme_shutdown_ctrl);
-
 static int nvme_configure_timestamp(struct nvme_ctrl *ctrl)
 {
 	__le64 ts;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 489f5e79720469..9cb9e067969c7a 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2506,7 +2506,7 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
 	 * Other transports, which don't have link-level contexts bound
 	 * to sqe's, would try to gracefully shutdown the controller by
 	 * writing the registers for shutdown and polling (call
-	 * nvme_shutdown_ctrl()). Given a bunch of i/o was potentially
+	 * nvme_disable_ctrl()). Given a bunch of i/o was potentially
 	 * just aborted and we will wait on those contexts, and given
 	 * there was no indication of how live the controlelr is on the
 	 * link, don't send more io to create more contexts for the
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index b5c0e4312d9b60..c2a0c1593fc779 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -733,9 +733,8 @@ void nvme_cancel_tagset(struct nvme_ctrl *ctrl);
 void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
-int nvme_disable_ctrl(struct nvme_ctrl *ctrl);
+int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown);
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl);
-int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
 int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 		const struct nvme_ctrl_ops *ops, unsigned long quirks);
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ac734c8f6640e5..84226dce9b3b92 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1500,11 +1500,7 @@ static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
 {
 	struct nvme_queue *nvmeq = &dev->queues[0];
 
-	if (shutdown)
-		nvme_shutdown_ctrl(&dev->ctrl);
-	else
-		nvme_disable_ctrl(&dev->ctrl);
-
+	nvme_disable_ctrl(&dev->ctrl, shutdown);
 	nvme_poll_irqdisable(nvmeq);
 }
 
@@ -1819,7 +1815,14 @@ static int nvme_pci_configure_admin_queue(struct nvme_dev *dev)
 	    (readl(dev->bar + NVME_REG_CSTS) & NVME_CSTS_NSSRO))
 		writel(NVME_CSTS_NSSRO, dev->bar + NVME_REG_CSTS);
 
-	result = nvme_disable_ctrl(&dev->ctrl);
+	/*
+	 * If the device has been passed off to us in an enabled state, just
+	 * clear the enabled bit.  The spec says we should set the 'shutdown
+	 * notification bits', but doing so may cause the device to complete
+	 * commands to the admin queue ... and we don't know what memory that
+	 * might be pointing at!
+	 */
+	result = nvme_disable_ctrl(&dev->ctrl, false);
 	if (result < 0)
 		return result;
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 448abf8cdf1f6e..cc61a1b8311b16 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2208,10 +2208,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 {
 	nvme_rdma_teardown_io_queues(ctrl, shutdown);
 	nvme_quiesce_admin_queue(&ctrl->ctrl);
-	if (shutdown)
-		nvme_shutdown_ctrl(&ctrl->ctrl);
-	else
-		nvme_disable_ctrl(&ctrl->ctrl);
+	nvme_disable_ctrl(&ctrl->ctrl, shutdown);
 	nvme_rdma_teardown_admin_queue(ctrl, shutdown);
 }
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 79789daddeac52..95e54e9c1bb1fc 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2142,10 +2142,7 @@ static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
 	nvme_tcp_teardown_io_queues(ctrl, shutdown);
 	nvme_quiesce_admin_queue(ctrl);
-	if (shutdown)
-		nvme_shutdown_ctrl(ctrl);
-	else
-		nvme_disable_ctrl(ctrl);
+	nvme_disable_ctrl(ctrl, shutdown);
 	nvme_tcp_teardown_admin_queue(ctrl, shutdown);
 }
 
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 6d176621f46dc6..0015aed5c16996 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -401,7 +401,7 @@ static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 
 	nvme_quiesce_admin_queue(&ctrl->ctrl);
 	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
-		nvme_shutdown_ctrl(&ctrl->ctrl);
+		nvme_disable_ctrl(&ctrl->ctrl, true);
 
 	nvme_cancel_admin_tagset(&ctrl->ctrl);
 	nvme_loop_destroy_admin_queue(ctrl);
-- 
2.30.2

Re: [PATCH 3/9] nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl

[Hector Martin]

On 29/11/2022 22.22, Christoph Hellwig wrote:
> Many of the callers decide which one to use based on a bool argument and
> there is at least some code to be shared, so merge these two.  Also
> move a comment specific to a single callsite to that callsite.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/apple.c  |  5 +----
>  drivers/nvme/host/core.c   | 33 ++++++++++-----------------------
>  drivers/nvme/host/fc.c     |  2 +-
>  drivers/nvme/host/nvme.h   |  3 +--
>  drivers/nvme/host/pci.c    | 15 +++++++++------
>  drivers/nvme/host/rdma.c   |  5 +----
>  drivers/nvme/host/tcp.c    |  5 +----
>  drivers/nvme/target/loop.c |  2 +-
>  8 files changed, 25 insertions(+), 45 deletions(-)
> 

For the Apple and core bits:

Reviewed-by: Hector Martin <marcan@marcan.st>

- Hector

Re: [PATCH 3/9] nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 4/9] nvme-pci: remove nvme_disable_admin_queue

[Christoph Hellwig]

nvme_disable_admin_queue has only a single caller, and just calls two
other funtions, so remove it to clean up the remove path a little more.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 84226dce9b3b92..c6a02210d22b46 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1496,14 +1496,6 @@ static void nvme_suspend_io_queues(struct nvme_dev *dev)
 		nvme_suspend_queue(&dev->queues[i]);
 }
 
-static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
-{
-	struct nvme_queue *nvmeq = &dev->queues[0];
-
-	nvme_disable_ctrl(&dev->ctrl, shutdown);
-	nvme_poll_irqdisable(nvmeq);
-}
-
 /*
  * Called only on a device that has been disabled and after all other threads
  * that can check this device's completion queues have synced, except
@@ -2711,7 +2703,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 
 	if (!dead && dev->ctrl.queue_count > 0) {
 		nvme_disable_io_queues(dev);
-		nvme_disable_admin_queue(dev, shutdown);
+		nvme_disable_ctrl(&dev->ctrl, shutdown);
+		nvme_poll_irqdisable(&dev->queues[0]);
 	}
 	nvme_suspend_io_queues(dev);
 	nvme_suspend_queue(&dev->queues[0]);
-- 
2.30.2

Re: [PATCH 4/9] nvme-pci: remove nvme_disable_admin_queue

[Eric Curtin]

On Tue, 29 Nov 2022 at 13:29, Christoph Hellwig <hch@lst.de> wrote:
>
> nvme_disable_admin_queue has only a single caller, and just calls two
> other funtions, so remove it to clean up the remove path a little more.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

> ---
>  drivers/nvme/host/pci.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 84226dce9b3b92..c6a02210d22b46 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1496,14 +1496,6 @@ static void nvme_suspend_io_queues(struct nvme_dev *dev)
>                 nvme_suspend_queue(&dev->queues[i]);
>  }
>
> -static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
> -{
> -       struct nvme_queue *nvmeq = &dev->queues[0];
> -
> -       nvme_disable_ctrl(&dev->ctrl, shutdown);
> -       nvme_poll_irqdisable(nvmeq);
> -}
> -
>  /*
>   * Called only on a device that has been disabled and after all other threads
>   * that can check this device's completion queues have synced, except
> @@ -2711,7 +2703,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>
>         if (!dead && dev->ctrl.queue_count > 0) {
>                 nvme_disable_io_queues(dev);
> -               nvme_disable_admin_queue(dev, shutdown);
> +               nvme_disable_ctrl(&dev->ctrl, shutdown);
> +               nvme_poll_irqdisable(&dev->queues[0]);
>         }
>         nvme_suspend_io_queues(dev);
>         nvme_suspend_queue(&dev->queues[0]);
> --
> 2.30.2
>
>

Re: [PATCH 4/9] nvme-pci: remove nvme_disable_admin_queue

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 5/9] nvme-pci: remove nvme_pci_disable

[Christoph Hellwig]

nvme_pci_disable has a single caller, fold it into that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c6a02210d22b46..c3d9b23699d3f3 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2657,18 +2657,6 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 	pci_release_mem_regions(to_pci_dev(dev->dev));
 }
 
-static void nvme_pci_disable(struct nvme_dev *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
-
-	pci_free_irq_vectors(pdev);
-
-	if (pci_is_enabled(pdev)) {
-		pci_disable_pcie_error_reporting(pdev);
-		pci_disable_device(pdev);
-	}
-}
-
 static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 {
 	bool dead = true, freeze = false;
@@ -2708,7 +2696,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	}
 	nvme_suspend_io_queues(dev);
 	nvme_suspend_queue(&dev->queues[0]);
-	nvme_pci_disable(dev);
+	pci_free_irq_vectors(pdev);
+	if (pci_is_enabled(pdev)) {
+		pci_disable_pcie_error_reporting(pdev);
+		pci_disable_device(pdev);
+	}
 	nvme_reap_pending_cqes(dev);
 
 	nvme_cancel_tagset(&dev->ctrl);
-- 
2.30.2

Re: [PATCH 5/9] nvme-pci: remove nvme_pci_disable

[Eric Curtin]

On Tue, 29 Nov 2022 at 13:29, Christoph Hellwig <hch@lst.de> wrote:
>
> nvme_pci_disable has a single caller, fold it into that.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

> ---
>  drivers/nvme/host/pci.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index c6a02210d22b46..c3d9b23699d3f3 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2657,18 +2657,6 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
>         pci_release_mem_regions(to_pci_dev(dev->dev));
>  }
>
> -static void nvme_pci_disable(struct nvme_dev *dev)
> -{
> -       struct pci_dev *pdev = to_pci_dev(dev->dev);
> -
> -       pci_free_irq_vectors(pdev);
> -
> -       if (pci_is_enabled(pdev)) {
> -               pci_disable_pcie_error_reporting(pdev);
> -               pci_disable_device(pdev);
> -       }
> -}
> -
>  static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  {
>         bool dead = true, freeze = false;
> @@ -2708,7 +2696,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>         }
>         nvme_suspend_io_queues(dev);
>         nvme_suspend_queue(&dev->queues[0]);
> -       nvme_pci_disable(dev);
> +       pci_free_irq_vectors(pdev);
> +       if (pci_is_enabled(pdev)) {
> +               pci_disable_pcie_error_reporting(pdev);
> +               pci_disable_device(pdev);
> +       }
>         nvme_reap_pending_cqes(dev);
>
>         nvme_cancel_tagset(&dev->ctrl);
> --
> 2.30.2
>
>

Re: [PATCH 5/9] nvme-pci: remove nvme_pci_disable

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 6/9] nvme-pci: cleanup nvme_suspend_queue

[Christoph Hellwig]

Remove the unused returne value, pass a dev + qid instead of the queue
as that is better for the callers as well as the function itself, and
remove the entirely pointless kerneldoc comment.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c3d9b23699d3f3..2c8dd0f7ef463b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1468,14 +1468,12 @@ static void nvme_free_queues(struct nvme_dev *dev, int lowest)
 	}
 }
 
-/**
- * nvme_suspend_queue - put queue into suspended state
- * @nvmeq: queue to suspend
- */
-static int nvme_suspend_queue(struct nvme_queue *nvmeq)
+static void nvme_suspend_queue(struct nvme_dev *dev, unsigned int qid)
 {
+	struct nvme_queue *nvmeq = &dev->queues[qid];
+
 	if (!test_and_clear_bit(NVMEQ_ENABLED, &nvmeq->flags))
-		return 1;
+		return;
 
 	/* ensure that nvme_queue_rq() sees NVMEQ_ENABLED cleared */
 	mb();
@@ -1484,8 +1482,7 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
 		nvme_quiesce_admin_queue(&nvmeq->dev->ctrl);
 	if (!test_and_clear_bit(NVMEQ_POLLED, &nvmeq->flags))
-		pci_free_irq(to_pci_dev(nvmeq->dev->dev), nvmeq->cq_vector, nvmeq);
-	return 0;
+		pci_free_irq(to_pci_dev(dev->dev), nvmeq->cq_vector, nvmeq);
 }
 
 static void nvme_suspend_io_queues(struct nvme_dev *dev)
@@ -1493,7 +1490,7 @@ static void nvme_suspend_io_queues(struct nvme_dev *dev)
 	int i;
 
 	for (i = dev->ctrl.queue_count - 1; i > 0; i--)
-		nvme_suspend_queue(&dev->queues[i]);
+		nvme_suspend_queue(dev, i);
 }
 
 /*
@@ -2695,7 +2692,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 		nvme_poll_irqdisable(&dev->queues[0]);
 	}
 	nvme_suspend_io_queues(dev);
-	nvme_suspend_queue(&dev->queues[0]);
+	nvme_suspend_queue(dev, 0);
 	pci_free_irq_vectors(pdev);
 	if (pci_is_enabled(pdev)) {
 		pci_disable_pcie_error_reporting(pdev);
-- 
2.30.2

Re: [PATCH 6/9] nvme-pci: cleanup nvme_suspend_queue

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 7/9] nvme-pci: rename nvme_disable_io_queues

[Christoph Hellwig]

This function really deletes the I/O queues, so rename it to match
the functionality.  Also move the main wrapper right next to the
actual underlying implementation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 2c8dd0f7ef463b..1b527377e35197 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -109,7 +109,7 @@ struct nvme_dev;
 struct nvme_queue;
 
 static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
-static bool __nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode);
+static void nvme_delete_io_queues(struct nvme_dev *dev);
 
 /*
  * Represents an NVM Express device.  Each nvme_dev is a PCI function.
@@ -2310,12 +2310,6 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
 			      PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY, &affd);
 }
 
-static void nvme_disable_io_queues(struct nvme_dev *dev)
-{
-	if (__nvme_disable_io_queues(dev, nvme_admin_delete_sq))
-		__nvme_disable_io_queues(dev, nvme_admin_delete_cq);
-}
-
 static unsigned int nvme_max_io_queues(struct nvme_dev *dev)
 {
 	/*
@@ -2423,7 +2417,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 
 	if (dev->online_queues - 1 < dev->max_qid) {
 		nr_io_queues = dev->online_queues - 1;
-		nvme_disable_io_queues(dev);
+		nvme_delete_io_queues(dev);
 		result = nvme_setup_io_queues_trylock(dev);
 		if (result)
 			return result;
@@ -2487,7 +2481,7 @@ static int nvme_delete_queue(struct nvme_queue *nvmeq, u8 opcode)
 	return 0;
 }
 
-static bool __nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode)
+static bool __nvme_delete_io_queues(struct nvme_dev *dev, u8 opcode)
 {
 	int nr_queues = dev->online_queues - 1, sent = 0;
 	unsigned long timeout;
@@ -2515,6 +2509,12 @@ static bool __nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode)
 	return true;
 }
 
+static void nvme_delete_io_queues(struct nvme_dev *dev)
+{
+	if (__nvme_delete_io_queues(dev, nvme_admin_delete_sq))
+		__nvme_delete_io_queues(dev, nvme_admin_delete_cq);
+}
+
 static void nvme_pci_alloc_tag_set(struct nvme_dev *dev)
 {
 	struct blk_mq_tag_set * set = &dev->tagset;
@@ -2687,7 +2687,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	nvme_quiesce_io_queues(&dev->ctrl);
 
 	if (!dead && dev->ctrl.queue_count > 0) {
-		nvme_disable_io_queues(dev);
+		nvme_delete_io_queues(dev);
 		nvme_disable_ctrl(&dev->ctrl, shutdown);
 		nvme_poll_irqdisable(&dev->queues[0]);
 	}
-- 
2.30.2

Re: [PATCH 7/9] nvme-pci: rename nvme_disable_io_queues

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 8/9] nvme-pci: return early on ctrl state mismatch in nvme_reset_work

[Christoph Hellwig]

When nvme_reset_work is called not in the resetting state something went
horribly wrong and we should not try to touch the controller registers,
so just leave the controller alone.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1b527377e35197..02940b4f42b104 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2792,8 +2792,7 @@ static void nvme_reset_work(struct work_struct *work)
 	if (dev->ctrl.state != NVME_CTRL_RESETTING) {
 		dev_warn(dev->ctrl.device, "ctrl state %d is not RESETTING\n",
 			 dev->ctrl.state);
-		result = -ENODEV;
-		goto out;
+		return;
 	}
 
 	/*
-- 
2.30.2

Re: [PATCH 8/9] nvme-pci: return early on ctrl state mismatch in nvme_reset_work

[Keith Busch]

On Tue, Nov 29, 2022 at 02:22:07PM +0100, Christoph Hellwig wrote:
> When nvme_reset_work is called not in the resetting state something went
> horribly wrong and we should not try to touch the controller registers,
> so just leave the controller alone.

Yeah, the only way something like that should happen is if a reset and
remove happen near the same time. We don't want the reset work to
disable the controller because the remove is already doing that.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 8/9] nvme-pci: return early on ctrl state mismatch in nvme_reset_work

[Christoph Hellwig]

On Tue, Dec 06, 2022 at 11:26:19AM +0000, Keith Busch wrote:
> Yeah, the only way something like that should happen is if a reset and
> remove happen near the same time. We don't want the reset work to
> disable the controller because the remove is already doing that.

Thanks, I've stole some of this for a better commit log.

Re: [PATCH 8/9] nvme-pci: return early on ctrl state mismatch in nvme_reset_work

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 9/9] nvme-pci: split out a nvme_pci_ctrl_is_dead helper

[Christoph Hellwig]

Clean up nvme_dev_disable by splitting the logic to detect if a
controller is dead into a separate helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 47 ++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 02940b4f42b104..d613b4292c0f95 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2654,36 +2654,39 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 	pci_release_mem_regions(to_pci_dev(dev->dev));
 }
 
-static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
+static bool nvme_pci_ctrl_is_dead(struct nvme_dev *dev)
 {
-	bool dead = true, freeze = false;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	u32 csts;
 
-	mutex_lock(&dev->shutdown_lock);
-	if (pci_is_enabled(pdev)) {
-		u32 csts;
+	if (!pci_is_enabled(pdev) || !pci_device_is_present(pdev))
+		return true;
+	if (pdev->error_state != pci_channel_io_normal)
+		return true;
 
-		if (pci_device_is_present(pdev))
-			csts = readl(dev->bar + NVME_REG_CSTS);
-		else
-			csts = ~0;
+	csts = readl(dev->bar + NVME_REG_CSTS);
+	return (csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY);
+}
+
+static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
+{
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	bool dead;
 
-		if (dev->ctrl.state == NVME_CTRL_LIVE ||
-		    dev->ctrl.state == NVME_CTRL_RESETTING) {
-			freeze = true;
+	mutex_lock(&dev->shutdown_lock);
+	dead = nvme_pci_ctrl_is_dead(dev);
+	if (dev->ctrl.state == NVME_CTRL_LIVE ||
+	    dev->ctrl.state == NVME_CTRL_RESETTING) {
+		if (pci_is_enabled(pdev))
 			nvme_start_freeze(&dev->ctrl);
-		}
-		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
-			pdev->error_state  != pci_channel_io_normal);
+		/*
+		 * Give the controller a chance to complete all entered requests
+		 * if doing a safe shutdown.
+		 */
+		if (!dead && shutdown)
+			nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
 	}
 
-	/*
-	 * Give the controller a chance to complete all entered requests if
-	 * doing a safe shutdown.
-	 */
-	if (!dead && shutdown && freeze)
-		nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
-
 	nvme_quiesce_io_queues(&dev->ctrl);
 
 	if (!dead && dev->ctrl.queue_count > 0) {
-- 
2.30.2

Re: [PATCH 9/9] nvme-pci: split out a nvme_pci_ctrl_is_dead helper

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: remove path cleanups

[Christoph Hellwig]

Any comments on this series?

Re: remove path cleanups

[Keith Busch]

On Tue, Nov 29, 2022 at 02:21:59PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up the remove path in the PCIe driver and a bit in the
> common code after fixing a harmless bug in the Apple driver.

Series looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>