remove path cleanups
Subsystems (5)
Christoph Hellwig
maintainerHi 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(-)
Christoph Hellwig
maintainerPATCH 1/9 nvme-apple: fix controller shutdown in apple_nvme_disable
nvme_shutdown_ctrl already shuts the controller down, there is no need to also call nvme_disable_ctrl for the shutdown case.
--- drivers/nvme/host/apple.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/nvme/host/apple.c +2 -1
@@ -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);
Signature · 1 line
2.30.2
Re: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable
Quoted 13 lines · Christoph Hellwig
nvme_shutdown_ctrlalready shuts the controller down, there is no need to also callnvme_disable_ctrlfor the shutdown case.Signed-off-by Christoph Hellwig <[email protected]>--- drivers/nvme/host/apple.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/nvme/host/apple.c +0
-0
drivers/nvme/host/apple.c static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown) · line 838
Quoted 9 lines
drivers/nvme/host/apple.c +2
-1@@ -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);
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
Janne Grunau
maintainerRe: [PATCH 1/9] nvme-apple: fix controller shutdown in apple_nvme_disable
Quoted 22 lines · Hector Martin
Quoted 21 lines · Christoph Hellwig
nvme_shutdown_ctrlalready shuts the controller down, there is no need to also callnvme_disable_ctrlfor the shutdown case.Signed-off-by Christoph Hellwig <[email protected]>--- drivers/nvme/host/apple.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/nvme/host/apple.c +2
-1@@ -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); }
Quoted 7 lines
drivers/nvme/host/apple.c static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown) · line 838
Quoted 1 line
drivers/nvme/host/apple.c +0
-0WRITE_ONCE (anv->ioq.enabled, false);Reviewed-by Hector Martin <[email protected]>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
---
drivers/nvme/host/apple.c +0 -0
@@ -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
Quoted 41 lines · Janne Grunau
Quoted 22 lines · Hector Martin
Quoted 21 lines · Christoph Hellwig
nvme_shutdown_ctrlalready shuts the controller down, there is no need to also callnvme_disable_ctrlfor the shutdown case.Signed-off-by Christoph Hellwig <[email protected]>--- drivers/nvme/host/apple.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/nvme/host/apple.c +2
-1@@ -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); }Quoted 7 lines
drivers/nvme/host/apple.c static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown) · line 838
Quoted 1 line
drivers/nvme/host/apple.c +0
-0WRITE_ONCE (anv->ioq.enabled, false);Reviewed-by Hector Martin <[email protected]>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()usesNVME_CC_ENABLEto decide whether to callapple_nvme_disable(). Onlynvme_disable_ctrl(ctrl, false)clearsNVME_CC_ENABLE.Even after extending the check to consider
NVME_CC_SHN_NORMALresume 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:
Quoted 6 lines
| 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_PRI0WThis 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
In reply to Janne Grunau[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:
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.]
Quoted 48 lines · Janne Grunau
Quoted 22 lines · Hector Martin
Quoted 21 lines · Christoph Hellwig
nvme_shutdown_ctrlalready shuts the controller down, there is no need to also callnvme_disable_ctrlfor the shutdown case.Signed-off-by Christoph Hellwig <[email protected]>--- drivers/nvme/host/apple.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/nvme/host/apple.c +2
-1@@ -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); }Quoted 7 lines
drivers/nvme/host/apple.c static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown) · line 838
Quoted 1 line
drivers/nvme/host/apple.c +0
-0WRITE_ONCE (anv->ioq.enabled, false);Reviewed-by Hector Martin <[email protected]>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 thatapple_nvme_reset_work()usesNVME_CC_ENABLEto decide whether to callapple_nvme_disable(). Onlynvme_disable_ctrl(ctrl, false)clearsNVME_CC_ENABLE.Even after extending the check to consider
NVME_CC_SHN_NORMALresume 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
Quoted 6 lines
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/[email protected]/
#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.
Quoted 16 lines
---
drivers/nvme/host/apple.c +3
-1@@ -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
In reply to Christoph HellwigQuoted 5 lines · Christoph Hellwig
nvme_shutdown_ctrlalready shuts the controller down, there is no need to also callnvme_disable_ctrlfor the shutdown case.Signed-off-by Christoph Hellwig <[email protected]>
Quoted 22 lines
--- drivers/nvme/host/apple.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/nvme/host/apple.c +2
-2@@ -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
In reply to Christoph HellwigChristoph Hellwig
maintainerPATCH 2/9 nvme: use nvme_wait_ready in nvme_shutdown_ctrl
In reply to Christoph HellwigRefactor the code to wait for CSTS state changes so that it can be reused by nvme_shutdown_ctrl.
--- drivers/nvme/host/core.c | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-)
drivers/nvme/host/core.c +11 -25
@@ -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);
Signature · 1 line
2.30.2
Re: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl
Quoted 13 lines · 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 <[email protected]>--- drivers/nvme/host/core.c | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-)drivers/nvme/host/core.c +0
-0
drivers/nvme/host/core.c static const struct block_device_operations nvme_bdev_ops = { · line 2266
Quoted 8 lines
drivers/nvme/host/core.c +2
-1@@ -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?
Quoted 21 lines
drivers/nvme/host/core.c +3
-3-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 ; } }
Christoph Hellwig
maintainerRe: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl
Quoted 5 lines · Pankaj Raghav
The
timeout_jiffiescalculation innvme_shutdown_ctrlis:
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 <[email protected]>
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.
--- drivers/nvme/host/core.c | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-)
drivers/nvme/host/core.c +12 -26
@@ -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);
Signature · 1 line
2.30.2
Pankaj Raghav
Reviewed-byRe: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl
Quoted 16 lines · Christoph Hellwig
Quoted 5 lines · Pankaj Raghav
The
timeout_jiffiescalculation innvme_shutdown_ctrlis:
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_timeoutfield. This version should fix that:---
From2f862232c4428d57dcfdc3b332866a9b2743711dMon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Wed, 16 Nov 2022 08:54:26 +0100
Subject: nvme: usenvme_wait_readyinnvme_shutdown_ctrl
Quoted 4 lines
Refactor the code to wait for CSTS state changes so that it can be reused by
nvme_shutdown_ctrl.Signed-off-by Christoph Hellwig <[email protected]>
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:
Quoted 30 lines
--- drivers/nvme/host/core.c | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-)drivers/nvme/host/core.c +5
-4@@ -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>
Quoted 3 lines
drivers/nvme/host/core.c +0
-3-msleep (100 ); -if (fatal_signal_pending (current)) -return -EINTR ;
Keith Busch
maintainerRe: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl
Quoted 9 lines · Pankaj Raghav
Quoted 8 lines · Christoph Hellwig
From
2f862232c4428d57dcfdc3b332866a9b2743711dMon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Wed, 16 Nov 2022 08:54:26 +0100
Subject: nvme: usenvme_wait_readyinnvme_shutdown_ctrlRefactor the code to wait for CSTS state changes so that it can be reused by
nvme_shutdown_ctrl.
Quoted 7 lines
Quoted 1 line
Signed-off-by Christoph Hellwig <[email protected]>One minor nit: I also notice that
nvme_wait_readybusy 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.
Christoph Hellwig
maintainerRe: [PATCH 2/9] nvme: use nvme_wait_ready in nvme_shutdown_ctrl
In reply to Pankaj RaghavQuoted 3 lines
One minor nit: I also notice that
nvme_wait_readybusy 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.
Christoph Hellwig
maintainerPATCH 3/9 nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl
In reply to Christoph HellwigMany 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.
--- 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(-)
drivers/nvme/host/apple.c +1 -4
@@ -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);
drivers/nvme/host/core.c +10 -23
@@ -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;
drivers/nvme/host/fc.c +1 -1
@@ -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
drivers/nvme/host/nvme.h +1 -2
@@ -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);
drivers/nvme/host/pci.c +9 -6
@@ -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;
drivers/nvme/host/rdma.c +1 -4
@@ -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); }
drivers/nvme/host/tcp.c +1 -4
@@ -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); }
drivers/nvme/target/loop.c +1 -1
@@ -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);
Signature · 1 line
2.30.2
Re: [PATCH 3/9] nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl
Quoted 5 lines · 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 <[email protected]>
Quoted 11 lines
--- 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:
- Hector
Re: [PATCH 3/9] nvme: merge nvme_shutdown_ctrl into nvme_disable_ctrl
In reply to Christoph HellwigChristoph Hellwig
maintainerPATCH 4/9 nvme-pci: remove nvme_disable_admin_queue
In reply to Christoph Hellwignvme_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.
--- drivers/nvme/host/pci.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/nvme/host/pci.c +2 -9
@@ -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 ]);
Signature · 1 line
2.30.2
Re: [PATCH 4/9] nvme-pci: remove nvme_disable_admin_queue
Quoted 5 lines · Christoph Hellwig
nvme_disable_admin_queuehas 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 <[email protected]>
Quoted 37 lines
--- drivers/nvme/host/pci.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)drivers/nvme/host/pci.c +2
-10@@ -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
In reply to Christoph HellwigChristoph Hellwig
maintainerPATCH 5/9 nvme-pci: remove nvme_pci_disable
In reply to Christoph Hellwignvme_pci_disable has a single caller, fold it into that.
--- drivers/nvme/host/pci.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)
drivers/nvme/host/pci.c +5 -13
@@ -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);
Signature · 1 line
2.30.2
Re: [PATCH 5/9] nvme-pci: remove nvme_pci_disable
Quoted 4 lines · Christoph Hellwig
nvme_pci_disablehas a single caller, fold it into that.Signed-off-by Christoph Hellwig <[email protected]>
Quoted 44 lines
--- drivers/nvme/host/pci.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)drivers/nvme/host/pci.c +5
-14@@ -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
In reply to Christoph HellwigChristoph Hellwig
maintainerPATCH 6/9 nvme-pci: cleanup nvme_suspend_queue
In reply to Christoph HellwigRemove 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.
--- drivers/nvme/host/pci.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
drivers/nvme/host/pci.c +7 -10
@@ -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);
Signature · 1 line
2.30.2
Re: [PATCH 6/9] nvme-pci: cleanup nvme_suspend_queue
Christoph Hellwig
maintainerPATCH 7/9 nvme-pci: rename nvme_disable_io_queues
In reply to Christoph HellwigThis 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.
--- drivers/nvme/host/pci.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/nvme/host/pci.c +10 -10
@@ -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 ]); }
Signature · 1 line
2.30.2
Re: [PATCH 7/9] nvme-pci: rename nvme_disable_io_queues
Christoph Hellwig
maintainerPATCH 8/9 nvme-pci: return early on ctrl state mismatch in nvme_reset_work
In reply to Christoph HellwigWhen 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.
--- drivers/nvme/host/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/nvme/host/pci.c +1 -2
@@ -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 ; }/*
Signature · 1 line
2.30.2
Re: [PATCH 8/9] nvme-pci: return early on ctrl state mismatch in nvme_reset_work
Quoted 3 lines · Christoph Hellwig
When
nvme_reset_workis 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.
Christoph Hellwig
maintainerRe: [PATCH 8/9] nvme-pci: return early on ctrl state mismatch in nvme_reset_work
Quoted 3 lines · Keith Busch
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
In reply to Christoph HellwigChristoph Hellwig
maintainerPATCH 9/9 nvme-pci: split out a nvme_pci_ctrl_is_dead helper
In reply to Christoph HellwigClean up nvme_dev_disable by splitting the logic to detect if a controller is dead into a separate helper.
--- drivers/nvme/host/pci.c | 47 ++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 22 deletions(-)
drivers/nvme/host/pci.c +25 -22
@@ -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 ) {
Signature · 1 line
2.30.2
Re: [PATCH 9/9] nvme-pci: split out a nvme_pci_ctrl_is_dead helper
Christoph Hellwig
maintainerAny comments on this series?
Quoted 4 lines · 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.
Series looks good.