* [PATCH 0/2] nvme-apple: Fix suspend-resume regression @ 2023-01-11 4:36 Hector Martin 2023-01-11 4:36 ` [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice Hector Martin 2023-01-11 4:36 ` [PATCH 2/2] nvme: Handle shut down controllers during initialization Hector Martin 0 siblings, 2 replies; 9+ messages in thread From: Hector Martin @ 2023-01-11 4:36 UTC (permalink / raw) To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg Cc: Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel, Hector Martin Commit c76b8308e4c9 introduced a behavior change in the way nvme-apple disables controllers, avoiding a disable in the shutdown path. Unfortunately, the NVMe core does not know how to actually start up controllers in the shutdown state, and this broke suspend/resume since we use the shutdown command for device suspend in nvme-apple. Additionally, nvme-apple was also checking only for the disable state in the reset path to decide whether to disable the controller again, and just having the controller shut down was triggering an unnecessary code path that broke things further. This short series fixes those issues and makes suspend/resume work on nvme-apple again. nvme-pci is, to my knowledge, not affected since it only issues a shutdown when the whole system is actually shutting down, never to come back. Hector Martin (2): nvme-apple: Do not try to shut down the controller twice nvme: Handle shut down controllers during initialization drivers/nvme/host/apple.c | 3 ++- drivers/nvme/host/core.c | 13 +++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) -- 2.35.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice 2023-01-11 4:36 [PATCH 0/2] nvme-apple: Fix suspend-resume regression Hector Martin @ 2023-01-11 4:36 ` Hector Martin 2023-01-11 4:54 ` Christoph Hellwig 2023-01-11 4:36 ` [PATCH 2/2] nvme: Handle shut down controllers during initialization Hector Martin 1 sibling, 1 reply; 9+ messages in thread From: Hector Martin @ 2023-01-11 4:36 UTC (permalink / raw) To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg Cc: Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel, Hector Martin The blamed commit stopped explicitly disabling the controller when we do a controlled shutdown, but apple_nvme_reset_work was only checking for the disable bit before deciding to issue another disable. Check for the shutdown state too, to avoid breakage. This issue does not affect nvme-pci, since it only issues controller shutdowns when the system is actually shutting down anyway. Fixes: c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable") Signed-off-by: Hector Martin <marcan@marcan.st> --- 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 e13a992b6096..1961376447dc 100644 --- a/drivers/nvme/host/apple.c +++ b/drivers/nvme/host/apple.c @@ -1023,7 +1023,8 @@ static void apple_nvme_reset_work(struct work_struct *work) goto out; } - if (anv->ctrl.ctrl_config & NVME_CC_ENABLE) + if (anv->ctrl.ctrl_config & NVME_CC_ENABLE && + !(anv->ctrl.ctrl_config & NVME_CC_SHN_MASK)) apple_nvme_disable(anv, false); /* RTKit must be shut down cleanly for the (soft)-reset to work */ -- 2.35.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice 2023-01-11 4:36 ` [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice Hector Martin @ 2023-01-11 4:54 ` Christoph Hellwig 2023-01-11 5:10 ` Hector Martin 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2023-01-11 4:54 UTC (permalink / raw) To: Hector Martin Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel On Wed, Jan 11, 2023 at 01:36:13PM +0900, Hector Martin wrote: > The blamed commit stopped explicitly disabling the controller when we do > a controlled shutdown, but apple_nvme_reset_work was only checking for > the disable bit before deciding to issue another disable. Check for the > shutdown state too, to avoid breakage. > > This issue does not affect nvme-pci, since it only issues controller > shutdowns when the system is actually shutting down anyway. There's a few other places where nvme-pci does a shutdown like probe/reset failure and most notably and mostly notably various power management scenarios. What path is causing a problem here for nvme-apple? I fear we're missing some highler level check here and getting further out of sync. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice 2023-01-11 4:54 ` Christoph Hellwig @ 2023-01-11 5:10 ` Hector Martin 2023-01-11 5:18 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Hector Martin @ 2023-01-11 5:10 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, Jens Axboe, Sagi Grimberg, Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel On 11/01/2023 13.54, Christoph Hellwig wrote: > On Wed, Jan 11, 2023 at 01:36:13PM +0900, Hector Martin wrote: >> The blamed commit stopped explicitly disabling the controller when we do >> a controlled shutdown, but apple_nvme_reset_work was only checking for >> the disable bit before deciding to issue another disable. Check for the >> shutdown state too, to avoid breakage. >> >> This issue does not affect nvme-pci, since it only issues controller >> shutdowns when the system is actually shutting down anyway. > > There's a few other places where nvme-pci does a shutdown like > probe/reset failure and most notably and mostly notably various > power management scenarios. > > What path is causing a problem here for nvme-apple? I fear we're > missing some highler level check here and getting further out of > sync. > OK, so the first question is who is responsible for resetting the controller after a shutdown? The spec requires a reset in order to bring it back up from that state. Indeed the PCIe code does an explicit disable right now (though, judging by the comment, it probably wasn't done with the intent of resetting after a shutdown, it just happens to work for that too :)) Right now, apple_nvme_reset_work() tries to check for the condition of an enabled controller (under the assumption that it's coming from a live controller, not considering shutdown/sleep) and issue an apple_nvme_disable(). However, this doesn't work when resuming because at that point the firmware coprocessor is shut down, so the device isn't usable (can't even get a disable command to complete properly). Perhaps a better conditional here would be to check for apple_rtkit_is_running(), since apple_nvme_disable() can't work otherwise. But then, even if we get past that, once apple_nvme_reset_work actually resets the firmware CPU and kicks things back online, the controller is still logically in the shutdown state. So something has to disable it in order for nvme_enable_ctrl() to work. An alternate patch would be to change the gate for apple_nvme_disable() in apple_nvme_reset_work() to check for apple_rtkit_is_running() on top of the controller enable state, and then add a further direct call to nvme_disable_ctrl(..., false) later in apple_nvme_reset_work, once the firmware is back up, to ensure we can enable it after. Thoughts? Alternatively, we could just revert to the prior behavior of always issuing a disable after a shutdown. We need to disable at some point to come back anyway, so it might as well be done early (before we shut down firmware, so it works). - Hector _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice 2023-01-11 5:10 ` Hector Martin @ 2023-01-11 5:18 ` Christoph Hellwig 2023-01-11 5:44 ` Hector Martin 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2023-01-11 5:18 UTC (permalink / raw) To: Hector Martin Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg, Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel On Wed, Jan 11, 2023 at 02:10:42PM +0900, Hector Martin wrote: > OK, so the first question is who is responsible for resetting the > controller after a shutdown? The spec requires a reset in order to bring > it back up from that state. Indeed the PCIe code does an explicit > disable right now (though, judging by the comment, it probably wasn't > done with the intent of resetting after a shutdown, it just happens to > work for that too :)) We need to do the reset before banging the registers to make sure the controller is in a sane state before starting to set it up. > Right now, apple_nvme_reset_work() tries to check for the condition of > an enabled controller (under the assumption that it's coming from a live > controller, not considering shutdown/sleep) and issue an > apple_nvme_disable(). However, this doesn't work when resuming because > at that point the firmware coprocessor is shut down, so the device isn't > usable (can't even get a disable command to complete properly). Perhaps > a better conditional here would be to check for > apple_rtkit_is_running(), since apple_nvme_disable() can't work otherwise. So on a resume the controller should have previously been shutdown properly, and this shouldn't be an issue. Does the apple implementation leave some weird state after a shut down? In that case the resume callback might want to do an explicit controller disable before doing the reset. > Alternatively, we could just revert to the prior behavior of always > issuing a disable after a shutdown. We need to disable at some point to > come back anyway, so it might as well be done early (before we shut down > firmware, so it works). So the disable before shutdown doesn't really make sense from the NVMe POV - the shutdown is to cleanly bring the device into a state where it can quickly recover. While a disable is an abprupt shutdown, which can have effects on recover time, and might also use way more P/E cycles than nessecary. So if you always want to do a disable, it should be after shutdown, and in doubt in the resume / setup path instead of the remove / suspend one. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice 2023-01-11 5:18 ` Christoph Hellwig @ 2023-01-11 5:44 ` Hector Martin 2023-01-11 6:41 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Hector Martin @ 2023-01-11 5:44 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, Jens Axboe, Sagi Grimberg, Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel On 11/01/2023 14.18, Christoph Hellwig wrote: > On Wed, Jan 11, 2023 at 02:10:42PM +0900, Hector Martin wrote: >> OK, so the first question is who is responsible for resetting the >> controller after a shutdown? The spec requires a reset in order to bring >> it back up from that state. Indeed the PCIe code does an explicit >> disable right now (though, judging by the comment, it probably wasn't >> done with the intent of resetting after a shutdown, it just happens to >> work for that too :)) > > We need to do the reset before banging the registers to make sure > the controller is in a sane state before starting to set it up. > >> Right now, apple_nvme_reset_work() tries to check for the condition of >> an enabled controller (under the assumption that it's coming from a live >> controller, not considering shutdown/sleep) and issue an >> apple_nvme_disable(). However, this doesn't work when resuming because >> at that point the firmware coprocessor is shut down, so the device isn't >> usable (can't even get a disable command to complete properly). Perhaps >> a better conditional here would be to check for >> apple_rtkit_is_running(), since apple_nvme_disable() can't work otherwise. > > So on a resume the controller should have previously been shutdown > properly, and this shouldn't be an issue. Does the apple implementation > leave some weird state after a shut down? In that case the resume > callback might want to do an explicit controller disable before doing > the reset. The controller is *shut down* but it's not *disabled*, and the existing code was only checking whether the controller is enabled to decide to issue another disable. The higher-level resume path can't do a disable since the firmware isn't up at that point, and the subsequent reset (which is shared with other conditions that cause a reset) is what brings the firmware back up. So the disable has to either happen in the suspend path, or in the shared reset path after we know the firmware is running. A shutdown but enabled controller is in "limbo"; the only way to know it's nonfunctional is explicitly checking the shutdown status bits. Other than that, it looks like a live controller that plays dead. This is documented in the spec as such. >> Alternatively, we could just revert to the prior behavior of always >> issuing a disable after a shutdown. We need to disable at some point to >> come back anyway, so it might as well be done early (before we shut down >> firmware, so it works). > > So the disable before shutdown doesn't really make sense from the > NVMe POV - the shutdown is to cleanly bring the device into a state > where it can quickly recover. While a disable is an abprupt shutdown, > which can have effects on recover time, and might also use way more > P/E cycles than nessecary. That's only if you issue a disable *in lieu* of a shutdown (and in fact if you do that on Apple controllers under some conditions, they crash). Issuing a disable *after* a shutdown is required by the NVMe spec if you want to use the controller again (and should basically do nothing at that point, since the controller is already cleanly shut down, but it is required to set EN to 0 such that the subsequent 0->1 transition actually kickstarts the controller again). If you don't do that, the controller never leaves the shutdown state (how would it know?). To be clear, the sequence I was attempting to describe (which is what we were doing before the patch that regressed this) was: (on sleep) - NVMe shutdown - NVMe disable - Firmware shutdown After the firmware shutdown, we can't do anything with NVMe again until we start firmware back up, which requires going through the reset flow. Right now we're doing: (on sleep) - NVMe shutdown - Firmware shutdown (wakeup) - Oops, NVMe is enabled, let's disable it! (times out due to FW being down but failure isn't propagated) - Firmware startup - NVMe enable (thinks it succeeds but actually the controller is still in the shutdown state since it was never disabled and this persists across the firmware cycle!) - I/O (never completes) - Hector _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice 2023-01-11 5:44 ` Hector Martin @ 2023-01-11 6:41 ` Christoph Hellwig 0 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2023-01-11 6:41 UTC (permalink / raw) To: Hector Martin Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg, Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel On Wed, Jan 11, 2023 at 02:44:42PM +0900, Hector Martin wrote: > The higher-level resume path can't do a disable since the firmware isn't > up at that point, and the subsequent reset (which is shared with other > conditions that cause a reset) is what brings the firmware back up. So > the disable has to either happen in the suspend path, or in the shared > reset path after we know the firmware is running. Ok, that's the weird part where nvme-apply really isn't nvme at all. Because for actual NVMe devices the register access must work all the time. > That's only if you issue a disable *in lieu* of a shutdown (and in fact > if you do that on Apple controllers under some conditions, they crash). > Issuing a disable *after* a shutdown is required by the NVMe spec if you > want to use the controller again (and should basically do nothing at > that point, since the controller is already cleanly shut down, but it is > required to set EN to 0 such that the subsequent 0->1 transition > actually kickstarts the controller again). If you don't do that, the > controller never leaves the shutdown state (how would it know?). Yes. Although I would not call this a disable after shutdown, but a disable (or rather reset) before using it again. > To be clear, the sequence I was attempting to describe (which is what we > were doing before the patch that regressed this) was: > > (on sleep) > - NVMe shutdown > - NVMe disable > - Firmware shutdown > > After the firmware shutdown, we can't do anything with NVMe again until > we start firmware back up, which requires going through the reset flow. > > Right now we're doing: > > (on sleep) > - NVMe shutdown > - Firmware shutdown > (wakeup) > - Oops, NVMe is enabled, let's disable it! (times out due to FW being > down but failure isn't propagated) > - Firmware startup > - NVMe enable (thinks it succeeds but actually the controller is still > in the shutdown state since it was never disabled and this persists > across the firmware cycle!) > - I/O (never completes) Yes, so I guess due to the weird firmware issues doing the disable after shutdown instead of before setting up might be the right thing for nvme-apple, unlike real NVMe. So I guess we need to do that in the driver, and add a big fat comment explaining why. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] nvme: Handle shut down controllers during initialization 2023-01-11 4:36 [PATCH 0/2] nvme-apple: Fix suspend-resume regression Hector Martin 2023-01-11 4:36 ` [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice Hector Martin @ 2023-01-11 4:36 ` Hector Martin 2023-01-11 4:50 ` Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: Hector Martin @ 2023-01-11 4:36 UTC (permalink / raw) To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg Cc: Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel, Hector Martin According to the spec, controllers need an explicit reset to become active again after a controller shutdown. Check for this state in nvme_enable_ctrl and issue an explicit disable if required, which will trigger the required reset. Fixes: c76b8308e4c9 ("nvme-apple: fix controller shutdown in apple_nvme_disable") Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/nvme/host/core.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 7be562a4e1aa..84e5db192ff9 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -2410,6 +2410,19 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl) if (ret) return ret; + /* + * If the controller is enabled but shut down, we need to disable it to + * reset it and have it come up. If the controller has completed a + * shutdown and is disabled, then we need to clear the shutdown request + * and enable it in the same write to CC. + * See NVMe Base Spec 2.0c Figure 47. + */ + if (ctrl->ctrl_config & NVME_CC_SHN_MASK && ctrl->ctrl_config & NVME_CC_ENABLE) { + ret = nvme_disable_ctrl(ctrl, false); + if (ret) + return ret; + } + ctrl->ctrl_config &= ~NVME_CC_SHN_MASK; ctrl->ctrl_config |= NVME_CC_ENABLE; ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config); if (ret) -- 2.35.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] nvme: Handle shut down controllers during initialization 2023-01-11 4:36 ` [PATCH 2/2] nvme: Handle shut down controllers during initialization Hector Martin @ 2023-01-11 4:50 ` Christoph Hellwig 0 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2023-01-11 4:50 UTC (permalink / raw) To: Hector Martin Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, Eric Curtin, Janne Grunau, Sven Peter, Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-nvme, linux-kernel On Wed, Jan 11, 2023 at 01:36:14PM +0900, Hector Martin wrote: > According to the spec, controllers need an explicit reset to become > active again after a controller shutdown. Check for this state in > nvme_enable_ctrl and issue an explicit disable if required, which will > trigger the required reset. I don't think this belongs into nvme_enable_ctrl. It seems like nvme-apple is missing the equivalent to the nvme_disable_ctrl call in nvme_pci_configure_admin_queue, though. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-11 6:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-11 4:36 [PATCH 0/2] nvme-apple: Fix suspend-resume regression Hector Martin 2023-01-11 4:36 ` [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice Hector Martin 2023-01-11 4:54 ` Christoph Hellwig 2023-01-11 5:10 ` Hector Martin 2023-01-11 5:18 ` Christoph Hellwig 2023-01-11 5:44 ` Hector Martin 2023-01-11 6:41 ` Christoph Hellwig 2023-01-11 4:36 ` [PATCH 2/2] nvme: Handle shut down controllers during initialization Hector Martin 2023-01-11 4:50 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).